From f583633b1f39b911384f986ff9312d22212d7fdd Mon Sep 17 00:00:00 2001 From: alicewonderland-dev <300129165+alicewonderland-dev@users.noreply.github.com> Date: Sun, 23 Aug 2026 19:41:10 -0400 Subject: [PATCH 1/7] fix(engine): let printed prevention shields survive the cleanup step MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `execute_cleanup` keyed its CR 514.2 prune on `shield_kind.is_shield()`, which is simply `!is_none()`. That deleted EVERY shield-carrying `ReplacementDefinition` — including the durable, printed statics parsed off a permanent's own Oracle text — from both `replacement_definitions` and `base_replacement_definitions`, with nothing to rebuild them. A printed prevention shield therefore worked only during the turn its host entered the battlefield and was dead for the rest of the game. Measured on the unmodified tree: staging Solitary Confinement gives `live=1 base=1`; one `execute_cleanup` later it is `live=0 base=0`. Since the opponent almost always attacks on a later turn, it read in play as "prevention does nothing at all". 148 definitions across 142 cards carry a printed Prevention-shaped replacement (Solitary Confinement, Nine Lives, Glacial Chasm, Fog Bank, Pariah, Energy Field, ...). CR 604.2 + CR 611.3b: an effect from a permanent's static ability lasts as long as the permanent is in the appropriate zone — it has no turn window. CR 611.2a: an effect from a resolving spell or ability lasts as long as that spell or ability stated. The two are indistinguishable at cleanup time, because by then the only difference — who created the effect — has been erased. So the fix moves the distinction to where it is still known: * the four exclusively-effect-created shield builders stamp their own `EndOfTurn` window at construction; * a resolving ability's own stated `duration` is read as a second CR 611.2a carrier when the effect grammar states none; * `execute_cleanup`'s predicate is reduced to the single typed authority the four sibling prunes already read, and no longer reads `shield_kind` at all. The `EndOfTurn` fallback for a shield with no stated window on either carrier is an ENGINE DEFAULT, not a rule, and is annotated as such: CR 611.2a's own no-duration case is "until the end of the game". It preserves today's behaviour for ~75 cards whose printed "this turn" the parser drops before the resolver sees it, and is knowingly wrong for 8 that have no printed window at all (Mount Keralia's text says "this game"). Also corrects a pre-existing annotation: the prune cited CR 701.19b (static-ability regeneration, which creates no shield); the shield rule is CR 701.19a. Six in-tree statements of the inverted "shield-kind is the lifetime sentinel" contract are rewritten to match. Regression coverage necessarily CROSSES A TURN BOUNDARY — every existing prevention test stays inside one turn, which is exactly why this survived. Each new test was verified discriminating by reverting the production line it guards and observing it go red. Co-Authored-By: Claude Opus 5 --- .../game/effects/add_target_replacement.rs | 168 +++- .../game/effects/create_damage_replacement.rs | 15 + .../engine/src/game/effects/prevent_damage.rs | 54 +- crates/engine/src/game/turns.rs | 123 ++- .../engine/src/parser/oracle_replacement.rs | 7 +- crates/engine/src/types/ability.rs | 146 ++- .../integration/gatta_and_luzzu_regression.rs | 22 +- .../integration/heroic_sacrifice_redirect.rs | 10 +- crates/engine/tests/integration/main.rs | 1 + ...printed_damage_prevention_survives_turn.rs | 915 ++++++++++++++++++ 10 files changed, 1411 insertions(+), 50 deletions(-) create mode 100644 crates/engine/tests/integration/printed_damage_prevention_survives_turn.rs diff --git a/crates/engine/src/game/effects/add_target_replacement.rs b/crates/engine/src/game/effects/add_target_replacement.rs index ccd3d5faff..caaeb8f398 100644 --- a/crates/engine/src/game/effects/add_target_replacement.rs +++ b/crates/engine/src/game/effects/add_target_replacement.rs @@ -9,17 +9,57 @@ use crate::types::game_state::GameState; use crate::types::identifiers::ObjectId; use crate::types::replacements::ReplacementEvent; +/// CR 611.2a: map a parser-side `Duration` onto the engine's replacement-side +/// `RestrictionExpiry`. +/// +/// Exhaustive by CLAUDE.md's "prefer exhaustive match over wildcard fallbacks": a +/// new `Duration` variant MUST make a decision here rather than silently +/// acquiring the engine turn-window default. +/// +/// A `None` result means "this engine has no faithful `RestrictionExpiry` for that +/// window". Callers that then apply +/// [`ReplacementDefinition::with_resolution_shield_expiry`] will give the shield a +/// turn window — read that helper's "known gap" note before adding an arm. pub(crate) fn expiry_from_duration( duration: Option<&Duration>, controller: crate::types::player::PlayerId, ) -> Option { match duration { + None => None, Some(Duration::UntilEndOfTurn) => Some(RestrictionExpiry::EndOfTurn), Some(Duration::UntilEndOfCombat) => Some(RestrictionExpiry::EndOfCombat), Some(Duration::UntilNextTurnOf { player: crate::types::ability::PlayerScope::Controller, }) => Some(RestrictionExpiry::UntilPlayerNextTurn { player: controller }), - _ => None, + // Non-controller `PlayerScope` readings carry no resolvable `PlayerId` at + // this seam, and `RestrictionExpiry::UntilPlayerNextTurn` needs a concrete + // player. No corpus card reaches this today. + Some(Duration::UntilNextTurnOf { .. }) => None, + // `RestrictionExpiry::UntilEndOfNextTurnOf` exists, but the untap-step + // arming in `turns.rs` iterates `state.restrictions` and matches only + // `GameRestriction::ProhibitActivity` — no replacement-side arming or + // prune exists, so stamping it on a `ReplacementDefinition` would make the + // replacement IMMORTAL. Deliberately unmapped until that arming is added. + Some(Duration::UntilEndOfNextTurnOf { .. }) => None, + // NOT identity-safe despite the shared name. `Duration::UntilHostLeavesPlay` + // means "when the SOURCE object leaves the battlefield"; + // `RestrictionExpiry::UntilHostLeavesPlay` is pruned when the object + // HOSTING the definition leaves (`layers.rs`, the host-left prune, which + // keys on the departed id). For a shield installed on a TARGET those are + // different objects — Old Fat Spider Can't See Me chapter II binds to the + // Saga while hosting its shield on the targeted creature, so the identity + // mapping would strand an immortal shield when the Saga leaves first. + Some(Duration::UntilHostLeavesPlay) => None, + // No `RestrictionExpiry` counterpart for a phase/step-scoped window. + Some(Duration::UntilNextStepOf { .. }) => None, + // CR 611.2b conditional windows are gated by + // `stamp_for_as_long_as_controlled_gate` / `ReplacementCondition`, not by + // an expiry stamp. + Some(Duration::ForAsLongAs { .. }) => None, + Some(Duration::UntilSourceExilesAnotherCard) => None, + Some(Duration::UntilOpponentBecomesMonarch) => None, + // CR 611.2a: no duration at all — the effect is not turn-bound. + Some(Duration::Permanent) => None, } } @@ -31,6 +71,25 @@ fn replacement_with_ability_expiry( if replacement.expiry.is_none() { replacement.expiry = expiry_from_duration(ability.duration.as_ref(), ability.controller); } + // CR 514.2 + CR 615.3: a SHIELD installed by a resolving spell or ability with + // no representable stated duration falls back to the engine's turn window — + // see `ReplacementDefinition::with_resolution_shield_expiry` (an engine + // default, not a CR rule). Gated on `shield_kind.is_shield()` so + // runtime-installed NON-shield riders that are legitimately durable keep + // `expiry: None`: the CR 611.2b `ControllerControlsSource` lock (ended by its + // own gate) and the CR 702.84a `UntilHostLeavesPlay` rider (ended by the + // battlefield-exit prune). + // + // CR 604.2: printed static shields never reach this seam — they are seeded + // into `base_replacement_definitions` by `printed_cards.rs` — so this cannot + // make a durable printed shield turn-bound. + // + // DEFENCE IN DEPTH: no corpus card reaches this stamp today. Exactly one + // `AddTargetReplacement` shield node exists in the card corpus (Impulsive + // Maneuvers) and the parser already stamps it `EndOfTurn`. This guard exists + // so that removing cleanup's `shield_kind` blanket cannot make a future + // unstamped runtime shield immortal. + replacement = replacement.with_resolution_shield_expiry(); // CR 109.4 + CR 614.1a: Anchor the installing player onto the replacement so // global pending damage replacements (pushed under the sentinel `ObjectId(0)`, // which has no controller in `state.objects`) can resolve a controller-relative @@ -435,6 +494,113 @@ mod tests { } } + /// CR 514.2 + CR 615.3: a shield-carrying replacement installed by a resolving + /// ability that stated NO representable window gets the engine's turn window at + /// this seam, so `turns::execute_cleanup` — which reads `expiry` alone — can + /// still end it. The `EndOfTurn` value is an engine default, NOT a CR rule; see + /// `ReplacementDefinition::with_resolution_shield_expiry`. + /// + /// DEFENCE IN DEPTH: no corpus card reaches this stamp today — exactly one + /// `AddTargetReplacement` shield node exists (Impulsive Maneuvers) and the + /// parser already stamps it `EndOfTurn`. This guard exists so that removing + /// cleanup's `shield_kind` blanket cannot make a future unstamped runtime + /// shield immortal. + #[test] + fn unstated_duration_shield_install_gets_engine_turn_window() { + use crate::types::ability::{Effect, PreventionAmount, ShieldKind}; + + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Source".to_string(), + Zone::Battlefield, + ); + let target = create_object( + &mut state, + CardId(2), + PlayerId(0), + "Bear".to_string(), + Zone::Battlefield, + ); + + // `prevention_shield` is the ONE builder that deliberately stamps no + // lifetime (it is shared with the printed static lowering), so the `None` + // reaching the install seam is genuine and not a builder artifact. + let shield = ReplacementDefinition::new(ReplacementEvent::DamageDone) + .prevention_shield(PreventionAmount::All) + .valid_card(TargetFilter::SelfRef); + assert_eq!( + shield.expiry, None, + "fixture must reach the seam with an unset expiry" + ); + + let ability = ResolvedAbility::new( + Effect::AddTargetReplacement { + replacement: Box::new(shield), + target: TargetFilter::Any, + }, + vec![TargetRef::Object(target)], + source, + PlayerId(0), + ); + assert_eq!( + ability.duration, None, + "fixture must reach the seam with both duration carriers unset" + ); + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + + // Positive reach-guard: the definition actually landed on the target. + let obj = state.objects.get(&target).unwrap(); + assert_eq!(obj.replacement_definitions.len(), 1); + assert_eq!( + obj.replacement_definitions[0].shield_kind, + ShieldKind::Prevention { + amount: PreventionAmount::All + } + ); + assert_eq!( + obj.replacement_definitions[0].expiry, + Some(RestrictionExpiry::EndOfTurn), + "CR 514.2: an unstated-window resolution shield takes the engine turn default" + ); + + // Negative sibling: a NON-shield rider installed the same way keeps + // `expiry: None` — the gate is `shield_kind.is_shield()`, not "stamp + // everything". CR 611.2b / CR 702.84a riders are legitimately durable. + let rider = ReplacementDefinition::new(ReplacementEvent::Moved) + .valid_card(TargetFilter::SelfRef) + .destination_zone(Zone::Exile); + let rider_ability = ResolvedAbility::new( + Effect::AddTargetReplacement { + replacement: Box::new(rider), + target: TargetFilter::Any, + }, + vec![TargetRef::Object(target)], + source, + PlayerId(0), + ); + resolve(&mut state, &rider_ability, &mut events).unwrap(); + + let obj = state.objects.get(&target).unwrap(); + let installed_rider = obj + .replacement_definitions + .as_slice() + .iter() + .find(|r| r.event == ReplacementEvent::Moved) + .expect("non-shield rider must be installed"); + assert!( + installed_rider.shield_kind.is_none(), + "reach-guard: the negative sibling must genuinely be a non-shield" + ); + assert_eq!( + installed_rider.expiry, None, + "a non-shield rider must not acquire a turn window at this seam" + ); + } + #[test] fn die_exile_rider_with_legacy_is_consumed_applies_exile_redirect() { use crate::types::ability::{AbilityKind, Effect, TargetFilter}; diff --git a/crates/engine/src/game/effects/create_damage_replacement.rs b/crates/engine/src/game/effects/create_damage_replacement.rs index ba7782b394..5c9ede22a4 100644 --- a/crates/engine/src/game/effects/create_damage_replacement.rs +++ b/crates/engine/src/game/effects/create_damage_replacement.rs @@ -438,6 +438,13 @@ mod tests { host.replacement_definitions[0].shield_kind, ShieldKind::DamageReplacementOneShot )); + // CR 614.5 + CR 514.2: the one-shot window is carried by `expiry`, which is + // the only thing `turns::execute_cleanup` reads. Revert guard for + // `ReplacementDefinition::damage_replacement_oneshot_shield`'s stamp. + assert_eq!( + host.replacement_definitions[0].expiry, + Some(crate::types::ability::RestrictionExpiry::EndOfTurn) + ); // First damage: 3 → doubled to 6 (opponent 20 → 14). let ctx = deal_damage::DamageContext::from_source(&state, source).unwrap(); @@ -596,6 +603,14 @@ mod tests { } )); assert_eq!(shield.valid_card, Some(TargetFilter::SelfRef)); + // CR 614.9 + CR 611.2a + CR 514.2: the redirection shield's turn window + // lives in `expiry` — `lifetime` above decides CONSUMPTION only, and + // `turns::execute_cleanup` reads `expiry` alone. Revert guard for + // `ReplacementDefinition::redirection_shield`'s stamp. + assert_eq!( + shield.expiry, + Some(crate::types::ability::RestrictionExpiry::EndOfTurn) + ); assert_eq!( shield.redirect_target, Some(TargetFilter::SpecificObject { id: chosen }), diff --git a/crates/engine/src/game/effects/prevent_damage.rs b/crates/engine/src/game/effects/prevent_damage.rs index 06675c7d5f..16a32f3219 100644 --- a/crates/engine/src/game/effects/prevent_damage.rs +++ b/crates/engine/src/game/effects/prevent_damage.rs @@ -3,7 +3,7 @@ use crate::game::quantity::resolve_quantity; use crate::types::ability::{ CombatDamageScope, DamageTargetFilter, DamageTargetPlayerScope, Effect, EffectError, EffectKind, FilterProp, PreventionAmount, PreventionScope, ReplacementDefinition, - ResolvedAbility, ShieldKind, SubAbilityLink, TargetFilter, TargetRef, + ResolvedAbility, SubAbilityLink, TargetFilter, TargetRef, }; use crate::types::events::GameEvent; use crate::types::game_state::{GameState, PendingContinuation, WaitingFor}; @@ -391,21 +391,46 @@ pub fn resolve( // CR 615.3: the single opportunity is bounded by the "the next time" // qualifier — consumed on apply; CR 514.2: expires at cleanup. shield.consume_on_apply = true; - shield.shield_kind = ShieldKind::PreventionOneShot; + // Builder, not a direct field write, so the one-shot path picks up the + // builder's CR 514.2 `EndOfTurn` stamp and there is one construction + // authority for the kind and its window. + shield = shield.prevention_oneshot_shield(); } - // CR 511.2 + CR 615: Apply the parsed prevention window as the shield's - // expiry. "this combat" -> `RestrictionExpiry::EndOfCombat`, pruned at the - // EndCombat phase (turns.rs) so a Suppressor Skyguard shield from combat 1 - // does not bleed into a second combat the same turn. A `None` duration - // leaves `expiry` unset -> the legacy end-of-turn `is_shield` prune still - // applies, so existing fixed/All prevention behavior is unchanged. + // CR 611.2a + CR 608.2: "a continuous effect generated by the resolution of a + // spell or ability lasts as long as stated by the SPELL OR ABILITY creating + // it." That sentence names two carriers, and this engine stores them + // separately: the effect grammar's own window (`prevention_duration` — "this + // combat" -> EndOfCombat) and the resolving ability's window + // (`ability.duration` — "Until your next turn" -> UntilPlayerNextTurn). Read + // the effect-level carrier first, then fall back to the ability-level one. + // + // CR 511.2 + CR 615: "this combat" -> `RestrictionExpiry::EndOfCombat`, pruned + // at the EndCombat phase (turns.rs) so a Suppressor Skyguard shield from + // combat 1 does not bleed into a second combat the same turn. Skyguard's + // window rides on `ability.duration`, so it is the `.or_else` arm — not the + // first one — that makes that statement true. (Its shield is object-hosted + // and is destroyed by the next layer flush before the corrected window can be + // observed; that is a separate, pre-existing defect.) + // + // Engine default, LAST: see `ReplacementDefinition::with_resolution_shield_expiry` + // — an end-of-turn fallback that compensates for the parser dropping a printed + // "this turn", NOT a CR rule (CR 611.2a's no-duration case is "until the end + // of the game"). Without it, `turns::execute_cleanup` — which reads `expiry` + // alone — would leave every duration-less resolution shield immortal. if let Some(expiry) = crate::game::effects::add_target_replacement::expiry_from_duration( prevention_duration.as_ref(), ability.controller, - ) { + ) + .or_else(|| { + crate::game::effects::add_target_replacement::expiry_from_duration( + ability.duration.as_ref(), + ability.controller, + ) + }) { shield = shield.expiry(expiry); } + shield = shield.with_resolution_shield_expiry(); // CR 609.7 + CR 609.7a: "prevent that damage" from "a source of // your choice" (Circle/Rune of Protection cycles) — the source is a player @@ -671,8 +696,13 @@ mod tests { /// `UntilEndOfCombat` ("this combat" — Suppressor Skyguard) must stamp the /// built shield with `RestrictionExpiry::EndOfCombat` so the EndCombat prune /// removes it and it does not bleed into a later combat the same turn. - /// `UntilEndOfTurn` maps to `EndOfTurn`; `None` leaves `expiry` unset (legacy - /// end-of-turn `is_shield` prune preserved — no regression). + /// `UntilEndOfTurn` maps to `EndOfTurn`; `None` on BOTH carriers + /// (`prevention_duration` here, and `ability.duration`, which + /// `ResolvedAbility::new` leaves unset) falls to the engine's turn default in + /// `ReplacementDefinition::with_resolution_shield_expiry` — an engine + /// fallback, NOT a CR rule, since CR 611.2a's own no-duration case is "until + /// the end of the game". That default is load-bearing: `turns::execute_cleanup` + /// reads `expiry` alone, so a `None` here would make the shield immortal. #[test] fn prevention_duration_sets_shield_expiry() { use crate::types::ability::{Duration, RestrictionExpiry}; @@ -686,7 +716,7 @@ mod tests { Some(Duration::UntilEndOfTurn), Some(RestrictionExpiry::EndOfTurn), ), - (None, None), + (None, Some(RestrictionExpiry::EndOfTurn)), ]; for (duration, expected_expiry) in cases { let mut state = GameState::new_two_player(42); diff --git a/crates/engine/src/game/turns.rs b/crates/engine/src/game/turns.rs index 21bb1096a0..f4f16180af 100644 --- a/crates/engine/src/game/turns.rs +++ b/crates/engine/src/game/turns.rs @@ -2251,19 +2251,54 @@ pub fn execute_cleanup(state: &mut GameState, events: &mut Vec) -> Op .unwrap_or_default(); state.attacked_defenders_last_turn.insert(ending, this_turn); - // CR 701.19b: Regeneration shields expire at cleanup. - // CR 615: Prevention effects also expire. - // CR 514.2: Resolution-time replacements with `expiry: EndOfTurn` (e.g., - // the "if [target] would die this turn, exile it instead" rider on - // damage spells) also expire here regardless of whether they fired. - // Also prune any consumed shields from earlier this turn. + // CR 514.2: "all “until end of turn” and “this turn” effects end." The typed + // `expiry` is the SINGLE authority for that window — the same authority the + // sibling prunes read (`complete_end_combat_teardown` for EndOfCombat, the + // untap-step prune for UntilPlayerNextTurn, the battlefield-exit prune in + // `layers.rs` for UntilHostLeavesPlay). + // + // CR 604.2 + CR 611.3b: a prevention or replacement effect created by a + // permanent's STATIC ability is active for as long as that permanent remains + // in the appropriate zone — it has no turn window and MUST survive this step. + // Those definitions carry `expiry: None`; keying this prune on `shield_kind` + // instead deleted every printed prevention card's shield at the first cleanup + // (Solitary Confinement, Nine Lives, Fog Bank, Pariah, ...). + // + // CR 611.2a + CR 608.2: a continuous effect created by the RESOLUTION of a + // spell or ability lasts as long as that spell or ability stated. Its creator + // stamps that window (see `ReplacementDefinition::with_resolution_shield_expiry`, + // whose EndOfTurn fallback is an engine default, NOT a CR rule — CR 611.2a's + // own no-duration case is "until the end of the game"; see that helper's doc). + // + // CR 500.1 + CR 511.3: the combat phase is a phase OF a turn, so an + // `EndOfCombat` window can never outlive the turn. `complete_end_combat_teardown` + // prunes `EndOfCombat` from the live and pending surfaces only — never from + // `base_replacement_definitions` — so this arm is the sole base-side catcher. + // + // CR 615.3 ("until they're used up or their duration has expired") is + // deliberately NOT read here, and `shield_kind` is not read by this closure at + // all. A consumed shield is ALREADY INERT without any prune: the object-side + // candidate gate early-returns on `is_consumed` and the pending-registry scan + // skips it (`game/replacement.rs`). + // + // CR 701.19a: a regeneration shield from a resolving spell or ability is + // stamped `EndOfTurn` at construction (`ReplacementDefinition::regeneration_shield`) + // and is caught by the first arm. (The annotation here previously cited + // CR 701.19b, which is STATIC-ability regeneration — no shield, no turn + // bound. Corrected in passing.) let expires_at_eot = |r: &ReplacementDefinition| { - r.shield_kind.is_shield() || matches!(r.expiry, Some(RestrictionExpiry::EndOfTurn)) + matches!( + r.expiry, + Some(RestrictionExpiry::EndOfTurn | RestrictionExpiry::EndOfCombat) + ) }; for obj in state.objects.iter_mut().map(|(_, v)| v) { obj.replacement_definitions.retain(|r| !expires_at_eot(r)); // CR 514.2: Clean up turn-bound replacement definitions from the base - // definitions during the cleanup step so they do not persist. + // definitions during the cleanup step so they do not persist. Turn-bound + // riders (the die-exile rider) are base-installed by + // `effects/add_target_replacement.rs`, so the base surface needs the same + // `expiry`-keyed prune; printed statics carry `expiry: None` and survive. std::sync::Arc::make_mut(&mut obj.base_replacement_definitions) .retain(|r| !expires_at_eot(r)); } @@ -8335,6 +8370,19 @@ mod tests { let normal = ReplacementDefinition::new(ReplacementEvent::Moved) .description("Normal repl".to_string()); + // CR 701.19a: a regeneration shield from a RESOLVING spell or ability is + // "the next time [permanent] would be destroyed this turn", so the builder + // stamps its own CR 514.2 window. `execute_cleanup` reads `expiry` alone — + // delete the stamp in `ReplacementDefinition::regeneration_shield` and both + // shields below become immortal. CR 701.19b's static-ability regeneration + // creates no shield at all and is not what this test covers. + assert_eq!(consumed.expiry, Some(RestrictionExpiry::EndOfTurn)); + assert_eq!(active.expiry, Some(RestrictionExpiry::EndOfTurn)); + assert_eq!( + normal.expiry, None, + "the surviving non-shield rider must carry no turn window" + ); + { let obj = state.objects.get_mut(&id).unwrap(); let mut c = consumed; @@ -8360,6 +8408,65 @@ mod tests { ); } + /// CR 500.1 + CR 511.3: the combat phase is a phase OF a turn, so an + /// `EndOfCombat` window can never outlive its turn. `complete_end_combat_teardown` + /// prunes `EndOfCombat` from the live and pending surfaces only — never from + /// `base_replacement_definitions` — so the cleanup step is the sole base-side + /// catcher and must keep this arm. + /// + /// Negative sibling in the same test: a CR 604.2 printed-static-shaped + /// definition (`expiry: None`) on the same object must SURVIVE, proving the arm + /// is expiry-keyed and not a blanket over `shield_kind`. + #[test] + fn cleanup_expires_end_of_combat_prevention_shield() { + use crate::types::ability::{PreventionAmount, ReplacementDefinition, TargetFilter}; + use crate::types::replacements::ReplacementEvent; + + let mut state = GameState::new_two_player(42); + state.phase = Phase::PostCombatMain; + let id = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Bear".to_string(), + Zone::Battlefield, + ); + + let combat_bound = ReplacementDefinition::new(ReplacementEvent::DamageDone) + .valid_card(TargetFilter::SelfRef) + .prevention_shield(PreventionAmount::All) + .expiry(RestrictionExpiry::EndOfCombat); + let durable = ReplacementDefinition::new(ReplacementEvent::DamageDone) + .valid_card(TargetFilter::SelfRef) + .prevention_shield(PreventionAmount::All); + assert_eq!( + durable.expiry, None, + "CR 604.2: a printed static shield carries no expiry" + ); + + { + let obj = state.objects.get_mut(&id).unwrap(); + obj.replacement_definitions.push(combat_bound); + obj.replacement_definitions.push(durable); + // Reach-guard: both definitions really are installed before cleanup. + assert_eq!(obj.replacement_definitions.len(), 2); + } + + let mut events = Vec::new(); + execute_cleanup(&mut state, &mut events); + + let obj = state.objects.get(&id).unwrap(); + assert_eq!( + obj.replacement_definitions.len(), + 1, + "the EndOfCombat shield must be pruned and the durable one kept" + ); + assert_eq!( + obj.replacement_definitions[0].expiry, None, + "CR 604.2: the surviving definition is the printed-static-shaped one" + ); + } + /// CR 402.2: A player with NoMaximumHandSize skips the discard-to-7 check. #[test] fn execute_cleanup_skips_discard_with_no_max_hand_size() { diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index 63bf5d6cd9..d6b469c0a1 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -7150,8 +7150,11 @@ fn parse_continuous_redirect_recipient(input: &str) -> OracleResult<'_, DamageRe /// rather than a CR 614.5 one-opportunity shield: "all damage that would be dealt /// …" for a stated window applies to EVERY matching damage event in that window, /// unlike "the next time …"/"the next N damage …", which are spent by their first -/// event. The window itself is end-of-turn cleanup (CR 514.2), which -/// `ShieldKind::is_shield()` already enforces — the two spellings the corpus uses +/// event. The window itself is end-of-turn cleanup (CR 611.2a + CR 514.2), +/// stamped as `RestrictionExpiry::EndOfTurn` by +/// `ReplacementDefinition::redirection_shield` at install and read by +/// `turns::execute_cleanup`; `ShieldKind` classifies the effect and carries no +/// lifetime meaning of its own — the two spellings the corpus uses /// are Heroic Sacrifice's leading "Until end of turn," (lifted to the ability's /// own `duration` by the chunker before this parser sees the clause) and Gideon's /// Sacrifice / Saving Grace's inline "this turn". diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 749e612173..a7edaaf64a 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -1493,14 +1493,25 @@ pub enum DamageRedirectTarget { AttachedToSource, } -/// Shield type for one-shot replacement effects that expire at cleanup. +/// Classification of WHAT a damage-affecting replacement effect does — +/// regenerate, prevent, modify a damage amount, or redirect. +/// +/// **Carries no lifetime meaning.** The lifetime lives in +/// [`ReplacementDefinition::expiry`] and nowhere else: CR 611.2a bounds a +/// resolution-created shield by its stated window, while CR 604.2 makes a PRINTED +/// static ability's shield last as long as its object remains in the appropriate +/// zone. Both are `ShieldKind::Prevention { amount: All }` on the wire, and only +/// `expiry` tells them apart. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Hash, Serialize, Deserialize)] pub enum ShieldKind { #[default] None, /// CR 701.19a: Regeneration shield — consumed on use, expires at cleanup. Regeneration, - /// CR 615: Prevention shield — absorbs/prevents damage, expires at cleanup. + /// CR 615: Prevention shield — absorbs/prevents damage. The ONLY shield kind + /// shared between the printed static-ability lowering (CR 604.2 — durable, + /// `expiry: None`) and the resolution path (CR 611.2a — turn-bound via + /// `expiry`), so this variant on its own says nothing about lifetime. Prevention { amount: PreventionAmount }, /// CR 614.5 + CR 614.1a: One-shot damage-amount replacement created by an /// effect ("the next time ... would deal damage this turn, it deals double @@ -1575,6 +1586,10 @@ impl ShieldKind { matches!(self, ShieldKind::None) } + /// Classification only — **never** use this as an expiry predicate (see + /// [`ReplacementDefinition::expiry`]). A printed static's shield (CR 604.2) + /// and a resolution-created one (CR 611.2a) answer `true` alike while having + /// opposite lifetimes. pub fn is_shield(&self) -> bool { !self.is_none() } @@ -25252,7 +25267,10 @@ pub struct ReplacementDefinition { /// [`PlaneswalkReplacementScope::PlanarDieOnly`] for Fixed Point in Time. #[serde(default, skip_serializing_if = "Option::is_none")] pub planeswalk_scope: Option, - /// Shield type for one-shot replacement effects that expire at cleanup. + /// Classification of WHAT this damage-affecting replacement does (regenerate, + /// prevent, modify a damage amount, redirect). Carries no lifetime meaning — + /// the lifetime lives in `expiry` and nowhere else (CR 604.2 vs CR 611.2a). + /// See [`ShieldKind`]. #[serde(default, skip_serializing_if = "ShieldKind::is_none")] pub shield_kind: ShieldKind, /// CR 614.1a: Quantity modification for token/counter replacements (Double, Plus, Minus). @@ -25294,9 +25312,15 @@ pub struct ReplacementDefinition { /// `None` means this replacement persists until removed by other means /// (e.g., the source object leaving the battlefield). /// - /// Orthogonal to `shield_kind`: shields imply EOT expiry via - /// `is_shield()`. Cleanup logic ORs both signals so a replacement may - /// be both a shield and have an explicit `EndOfTurn` expiry. + /// **Single authority for WHEN a replacement ends** (CR 514.2 / CR 611.2a). + /// `shield_kind` classifies WHAT the replacement does and carries no lifetime + /// meaning whatsoever: CR 604.2 makes a static ability's shield last as long + /// as its object stays in the appropriate zone, so a printed shield and a + /// resolution-created one can hold the identical `ShieldKind` value and still + /// have opposite lifetimes. Every prune — `turns::execute_cleanup`, + /// `turns::complete_end_combat_teardown`, the untap-step prune, and the + /// battlefield-exit prune in `layers.rs` — reads this field and only this + /// field. #[serde(default, skip_serializing_if = "Option::is_none")] pub expiry: Option, /// CR 615.1a: Damage redirection target filter — when present, prevented damage is @@ -25541,19 +25565,94 @@ impl ReplacementDefinition { self } + /// Stamp the engine's default turn window on a shield created by the + /// RESOLUTION of a spell or ability that stated no window this engine can + /// represent. + /// + /// **This default is an engine fallback, not a rule.** CR 611.2a says a + /// resolution-created continuous effect with no stated duration "lasts until + /// the end of the game", and CR 615.3 says prevention effects "last until + /// they're used up or their duration has expired" — neither authorizes an + /// end-of-turn default. It exists because most of the duration-less + /// prevention shields in the card corpus come from cards whose printed text + /// DOES say "this turn" (Reverse Damage, Circle of Protection: Red, + /// Prismatic Strands, Honorable Passage, ...) and the parser drops that + /// window before the resolver sees it; the default reconstructs it. + /// CR 514.2 is the rule the window obeys ONCE STAMPED + /// (`turns::execute_cleanup`). + /// + /// Known gap, deliberate: the same default is applied to a shield whose + /// ability DID state a window that + /// [`crate::game::effects::add_target_replacement::expiry_from_duration`] + /// cannot represent — today that is `Old Fat Spider Can't See Me` chapter II + /// ("for as long as this Saga remains on the battlefield"), plus the printed + /// statics the parser lowered onto the resolution path (Phyrexian Vindicator, + /// Plated Pegasus, Gisela Blade of Goldnight, Battletide Alchemist, Shield of + /// the Avatar, Magma Pummeler, Cover of Winter, and Mount Keralia — whose text + /// says "this game"). For all of these the stamp is a turn window where the + /// card wants a longer one. It is behaviour-preserving (the pre-fix + /// `ShieldKind::is_shield()` cleanup prune removed them identically) and + /// conservative (a shield that ends too early is a missed prevention, never an + /// immortal one), but it is NOT rules-correct. See `expiry_from_duration` for + /// why each unrepresentable `Duration` maps to `None`. + /// + /// Never overwrites an explicit `expiry` (`EndOfCombat`, `UntilPlayerNextTurn`, + /// ...). NOTE the precise scope of that guarantee: it is true of the `expiry` + /// FIELD and false of a stated `Duration` — a `Duration` that + /// `expiry_from_duration` maps to `None` never becomes an `expiry`, so this + /// helper cannot see it and will stamp over the card's intent. + /// + /// Applies only to shield-carrying definitions: a runtime-installed NON-shield + /// rider may legitimately be durable (the CR 611.2b `ControllerControlsSource` + /// lock, the CR 702.84a `UntilHostLeavesPlay` rider), and must not be given a + /// turn window here. + /// + /// CR 604.2 + CR 611.3b: a printed static ability's shield never passes through + /// a resolution seam, so it keeps `expiry: None` and stays active for as long + /// as its object remains in a zone the replacement pipeline scans. + pub fn with_resolution_shield_expiry(mut self) -> Self { + if self.shield_kind.is_shield() && self.expiry.is_none() { + self.expiry = Some(RestrictionExpiry::EndOfTurn); + } + self + } + pub fn combat_scope(mut self, scope: CombatDamageScope) -> Self { self.combat_scope = Some(scope); self } - /// CR 701.19a: Mark this replacement as a regeneration shield (one-shot, expires at cleanup). + /// CR 701.19a: Mark this replacement as a regeneration shield created by a + /// RESOLVING spell or ability — "the next time [permanent] would be destroyed + /// **this turn**". CR 514.2: that window ends at the cleanup step, so the + /// shield stamps its own `EndOfTurn` expiry here; the cleanup prune reads + /// `expiry` alone (`turns::execute_cleanup`) and never infers a lifetime from + /// `shield_kind`. (CR 701.19b's STATIC-ability regeneration is a different + /// effect that creates no shield at all.) + /// + /// An explicit `.expiry(..)` always wins, whether applied before or after. pub fn regeneration_shield(mut self) -> Self { self.shield_kind = ShieldKind::Regeneration; + if self.expiry.is_none() { + self.expiry = Some(RestrictionExpiry::EndOfTurn); + } self } - /// CR 615: Mark this replacement as a damage prevention shield. - /// The shield absorbs or prevents damage, and is cleaned up at end of turn. + /// CR 615: Mark this replacement as a damage prevention shield — the shield + /// absorbs or prevents damage. + /// + /// **Deliberately stamps no lifetime.** This is the one shield builder shared + /// between the parser's printed static-ability lowering and the resolution + /// path, so the lifetime is decided by `expiry` and by nothing else: + /// - CR 604.2 + CR 611.3b: a PRINTED static ability's shield keeps + /// `expiry: None` and stays active for as long as its object remains in a + /// zone the replacement pipeline scans. It must NOT be pruned at cleanup. + /// - CR 611.2a: a shield created by the RESOLUTION of a spell or ability is + /// bounded by the window that spell or ability stated. Resolution callers + /// stamp it — an explicit [`ReplacementDefinition::expiry`] for a stated + /// window, then [`ReplacementDefinition::with_resolution_shield_expiry`] + /// for the engine's turn-window fallback. pub fn prevention_shield(mut self, amount: PreventionAmount) -> Self { self.shield_kind = ShieldKind::Prevention { amount }; self @@ -25562,9 +25661,15 @@ impl ReplacementDefinition { /// CR 615.1a + CR 615.3 + CR 514.2: Mark this replacement as a one-shot /// prevention shield ("the next time [source] would deal damage this turn, /// prevent that damage" — Awe Strike). Single opportunity per CR 615.3; - /// consumed on use, expires at cleanup per CR 514.2. + /// consumed on use, expires at cleanup per CR 514.2 — so the shield stamps its + /// own `EndOfTurn` expiry here, because `turns::execute_cleanup` reads `expiry` + /// alone and never infers a lifetime from `shield_kind`. An explicit + /// `.expiry(..)` always wins, whether applied before or after. pub fn prevention_oneshot_shield(mut self) -> Self { self.shield_kind = ShieldKind::PreventionOneShot; + if self.expiry.is_none() { + self.expiry = Some(RestrictionExpiry::EndOfTurn); + } self } @@ -25573,15 +25678,27 @@ impl ReplacementDefinition { /// the amount formula; the shield is consumed after its single use and /// expires at cleanup. Distinct from a continuous static (Furnace of Rath), /// which leaves `shield_kind` as `None`. + /// + /// CR 514.2: that turn window is stamped here as `EndOfTurn`, because + /// `turns::execute_cleanup` reads `expiry` alone and never infers a lifetime + /// from `shield_kind`. An explicit `.expiry(..)` always wins. pub fn damage_replacement_oneshot_shield(mut self) -> Self { self.shield_kind = ShieldKind::DamageReplacementOneShot; + if self.expiry.is_none() { + self.expiry = Some(RestrictionExpiry::EndOfTurn); + } self } /// CR 614.9: Mark this replacement as a redirection shield that re-targets - /// the damage recipient. Expires at cleanup; `lifetime` decides whether it is - /// also consumed by its first event (CR 614.5) or re-applies to every - /// matching event in its window (CR 611.2a). + /// the damage recipient. `lifetime` decides whether it is also consumed by its + /// first event (CR 614.5) or re-applies to every matching event in its window + /// (CR 611.2a) — that axis is CONSUMPTION and is orthogonal to expiry. + /// + /// CR 611.2a + CR 514.2: this shield is only ever built by a resolving spell + /// or ability, so it stamps its own `EndOfTurn` expiry here; + /// `turns::execute_cleanup` reads `expiry` alone and never infers a lifetime + /// from `shield_kind`. An explicit `.expiry(..)` always wins. pub fn redirection_shield( mut self, recipient: DamageRedirectTarget, @@ -25593,6 +25710,9 @@ impl ReplacementDefinition { amount, lifetime, }; + if self.expiry.is_none() { + self.expiry = Some(RestrictionExpiry::EndOfTurn); + } self } diff --git a/crates/engine/tests/integration/gatta_and_luzzu_regression.rs b/crates/engine/tests/integration/gatta_and_luzzu_regression.rs index ff278deb49..13e51d0d4b 100644 --- a/crates/engine/tests/integration/gatta_and_luzzu_regression.rs +++ b/crates/engine/tests/integration/gatta_and_luzzu_regression.rs @@ -42,7 +42,7 @@ use engine::game::effects; use engine::game::zones::create_object; use engine::types::ability::{ Effect, PreventionAmount, PreventionScope, QuantityExpr, QuantityRef, ResolvedAbility, - ShieldKind, TargetFilter, TargetRef, + RestrictionExpiry, ShieldKind, TargetFilter, TargetRef, }; use engine::types::counter::CounterType; use engine::types::game_state::GameState; @@ -141,16 +141,16 @@ fn gatta_and_luzzu_prevents_three_damage_events_and_accumulates_counters() { amount: PreventionAmount::All } )); - // CR 514.2 cleanup contract: every `ShieldKind != None` is pruned at the - // cleanup step via `ShieldKind::is_shield()` in `turns::execute_cleanup`, - // independently of the explicit `expiry` field. The duration plumbing on - // the ability is therefore advisory; the shield-kind sentinel is what - // guarantees EOT cleanup for prevention shields. - assert!( - chosen_obj.replacement_definitions[0] - .shield_kind - .is_shield(), - "prevention shield must register as a shield for EOT cleanup" + // CR 514.2 cleanup contract: `turns::execute_cleanup` prunes on the typed + // `expiry` field and ONLY on it. `shield_kind` classifies what the replacement + // does and carries no lifetime meaning — CR 604.2 makes a printed static's + // shield durable while holding this identical `ShieldKind` value. The duration + // plumbing on the ability is therefore load-bearing, not advisory: this + // ability's `Duration::UntilEndOfTurn` is what stamps the window here. + assert_eq!( + chosen_obj.replacement_definitions[0].expiry, + Some(RestrictionExpiry::EndOfTurn), + "CR 514.2: the ability's stated 'this turn' window must be stamped on the shield" ); let gatta_obj = state.objects.get(&gatta).unwrap(); diff --git a/crates/engine/tests/integration/heroic_sacrifice_redirect.rs b/crates/engine/tests/integration/heroic_sacrifice_redirect.rs index ce10996f62..555e76a9c1 100644 --- a/crates/engine/tests/integration/heroic_sacrifice_redirect.rs +++ b/crates/engine/tests/integration/heroic_sacrifice_redirect.rs @@ -132,9 +132,13 @@ fn heroic_sacrifice_redirects_every_event_to_the_chosen_creature_until_end_of_tu /// turn. A continuous redirection that outlived its window would silently /// protect its controller forever. /// -/// Revert guard: if the shield stopped being a `ShieldKind` (and so stopped -/// being pruned at cleanup), the next-turn event would still redirect and the -/// life total would be untouched. +/// Revert guard: `turns::execute_cleanup` prunes on the typed `expiry` field +/// alone — `shield_kind` classifies what a replacement does and carries no +/// lifetime meaning (CR 604.2 makes a printed static's shield durable while +/// holding the same `ShieldKind` value). This shield's `expiry == +/// Some(RestrictionExpiry::EndOfTurn)` is stamped by +/// `ReplacementDefinition::redirection_shield`; drop that stamp and the +/// next-turn event would still redirect and the life total would be untouched. #[test] fn heroic_sacrifice_redirect_expires_at_end_of_turn() { let mut scenario = GameScenario::new(); diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index f2d376d6ab..0c4b5a971b 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -940,6 +940,7 @@ mod precast_copy_shortcut; mod prepared_state_serde; mod primo_unbounded_fractal_counters; mod printed_ability_order; +mod printed_damage_prevention_survives_turn; mod proliferate_zero_counter; mod pulse_of_the_forge; mod punishing_punch_twice_subject_power; diff --git a/crates/engine/tests/integration/printed_damage_prevention_survives_turn.rs b/crates/engine/tests/integration/printed_damage_prevention_survives_turn.rs new file mode 100644 index 0000000000..ba7a4feaa2 --- /dev/null +++ b/crates/engine/tests/integration/printed_damage_prevention_survives_turn.rs @@ -0,0 +1,915 @@ +//! CR 604.2 + CR 611.3b vs CR 611.2a + CR 514.2 — the lifetime of a +//! damage-prevention shield is decided by its PROVENANCE, and the engine records +//! that provenance exactly once, at creation, in `ReplacementDefinition::expiry`. +//! +//! A printed static ability's prevention effect (Solitary Confinement, Nine +//! Lives, Fog Bank, Pariah, ...) is active "as long as the permanent with the +//! ability remains on the battlefield" (CR 604.2) — it has no turn window and +//! carries `expiry: None`. A prevention effect created by the RESOLUTION of a +//! spell or ability lasts "as long as stated by the spell or ability creating +//! it" (CR 611.2a), and its creator stamps that window. +//! +//! `turns::execute_cleanup` previously keyed its CR 514.2 prune on +//! `ShieldKind::is_shield()`, which is TRUE for both classes — so every printed +//! prevention card lost its shield at the first cleanup step and was dead for the +//! rest of the game. Every test in this file therefore CROSSES A TURN BOUNDARY: a +//! same-turn test passes with the bug present and proves nothing. +//! +//! Positive half (must survive): T1, T5. +//! Negative half (must still expire): T2 (ability-duration carrier), T4 (one-shot), +//! T7 (no window on either carrier — the engine's turn default), T8's step 5. +//! T8 is the "longer than a turn, but not forever" middle case. +//! +//! Oracle text is verbatim from Scryfall; the harness rules this file obeys +//! (stocked libraries, `active_player` reach-guards after every crossing, the +//! alternating End/Upkeep crossing idiom, and `#[must_use]` combat reach-guards) +//! are documented at their helpers below. + +use engine::game::combat::AttackTarget; +use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::{ + CombatDamageScope, PreventionAmount, ReplacementDefinition, RestrictionExpiry, ShieldKind, + TargetFilter, +}; +use engine::types::actions::GameAction; +use engine::types::game_state::WaitingFor; +use engine::types::identifiers::ObjectId; +use engine::types::mana::ManaCost; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; +use engine::types::replacements::ReplacementEvent; +use engine::types::zones::Zone; + +/// Verbatim Solitary Confinement — the reported bug. Printed static, no window. +const SOLITARY_CONFINEMENT_TEXT: &str = "Skip your draw step.\nAt the beginning of your upkeep, sacrifice Solitary Confinement unless you discard a card.\nPrevent all damage that would be dealt to you."; + +/// Verbatim Fog — a resolution-created shield whose window rides on the +/// ability's own `duration`. +const FOG_TEXT: &str = "Prevent all combat damage that would be dealt this turn."; + +/// Verbatim Fog Bank — a printed static contributing TWO definitions +/// ("dealt to" and "dealt by"). +const FOG_BANK_TEXT: &str = "Defender (This creature can't attack.)\nFlying\nPrevent all combat damage that would be dealt to and dealt by this creature."; + +/// Verbatim Reverse Damage — the duration-less resolution class: no window on +/// `prevention_duration` AND none on `ability.duration`. +const REVERSE_DAMAGE_TEXT: &str = "The next time a source of your choice would deal damage to you this turn, prevent that damage. You gain life equal to the damage prevented this way."; + +/// Verbatim Morningtide's Light — the ability-duration carrier ("Until your next +/// turn"), on a Sorcery that exiles itself so its shield lands on the +/// layer-stable pending registry. +const MORNINGTIDES_LIGHT_TEXT: &str = "Exile any number of target creatures. At the beginning of the next end step, return those cards to the battlefield tapped under their owners' control.\nUntil your next turn, prevent all damage that would be dealt to you.\nExile Morningtide's Light."; + +/// Verbatim Sewers of Estark — the corpus's only `prevention_duration: +/// UntilEndOfCombat` producer, and structurally combat-gated. +const SEWERS_OF_ESTARK_TEXT: &str = "Choose target creature. If it's attacking, it can't be blocked this turn. If it's blocking, prevent all combat damage that would be dealt this combat by it and each creature it's blocking."; + +/// Verbatim Awe Strike — a one-shot prevention shield, deliberately never +/// consumed in T4. +const AWE_STRIKE_TEXT: &str = "The next time target creature would deal damage this turn, prevent that damage. You gain life equal to the damage prevented this way."; + +fn free_cost() -> ManaCost { + ManaCost::Cost { + shards: vec![], + generic: 0, + } +} + +/// A permanent-type seed MUST be applied before the Oracle text: the ability +/// parse runs inside `from_oracle_text`, and `parse_oracle_text` given +/// `types: ["Sorcery"]` returns zero replacements for a static prevention line. +/// Copied from `statecraft_damage_prevention.rs`. +fn add_enchantment_spell_to_hand( + scenario: &mut GameScenario, + player: PlayerId, + name: &str, + oracle_text: &str, +) -> ObjectId { + scenario + .add_spell_to_hand(player, name, false) + .as_enchantment() + .from_oracle_text(oracle_text) + .with_mana_cost(free_cost()) + .id() +} + +/// Stock both libraries. Without this a player decks out on the far side of a +/// boundary, the game ends in `WaitingFor::GameOver`, combat never runs, and +/// EVERY life assertion in the test passes vacuously. +fn stock_libraries(scenario: &mut GameScenario) { + scenario.with_library_top( + P0, + &["F0a", "F0b", "F0c", "F0d", "F0e", "F0f", "F0g", "F0h"], + ); + scenario.with_library_top( + P1, + &["F1a", "F1b", "F1c", "F1d", "F1e", "F1f", "F1g", "F1h"], + ); +} + +/// Cross exactly one turn boundary. +/// +/// Two measured harness hazards make this a named helper rather than an inline +/// call: `advance_to_phase(Phase::Upkeep)` twice in a row does NOT advance a +/// second turn, and `advance_to_phase` STALLS SILENTLY (no panic, no error) when +/// a `WaitingFor` needs an action. Every caller therefore asserts +/// `active_player` afterwards — that assertion is the stall guard. +fn cross_boundary(runner: &mut GameRunner) { + runner.advance_to_phase(Phase::End); + runner.advance_to_phase(Phase::Upkeep); +} + +/// Drive combat from the current state through end of combat, declaring +/// `attacker` for `attacker_player` against `defend_player` and, if given, +/// `blocker` for the defender. Copied verbatim in shape from +/// `statecraft_damage_prevention.rs::run_combat`. +/// +/// The return value is the combat reach-guard: every prevention assertion in +/// this file reads "life unchanged", which is also what "combat never happened" +/// looks like. Callers MUST assert it `== true`. +#[must_use = "combat must be asserted to have actually run — see doc comment"] +fn run_combat( + runner: &mut GameRunner, + attacker_player: PlayerId, + attacker: ObjectId, + defend_player: PlayerId, + blocker: Option, +) -> bool { + let mut attacked = false; + let mut blocked = false; + let mut reached_end_of_combat = false; + + for _ in 0..400 { + if matches!( + runner.state().phase, + Phase::EndCombat | Phase::PostCombatMain + ) { + reached_end_of_combat = true; + break; + } + match runner.state().waiting_for.clone() { + WaitingFor::Priority { .. } => { + if runner.act(GameAction::PassPriority).is_err() { + break; + } + } + WaitingFor::OrderTriggers { .. } => { + if runner + .act(GameAction::OrderTriggers { order: vec![0] }) + .is_err() + { + break; + } + } + WaitingFor::DeclareAttackers { player, .. } + if player == attacker_player && !attacked => + { + attacked = true; + runner + .declare_attackers(&[(attacker, AttackTarget::Player(defend_player))]) + .expect("declaring the intended attacker must succeed"); + } + WaitingFor::DeclareAttackers { .. } => { + if runner.declare_attackers(&[]).is_err() { + break; + } + } + WaitingFor::DeclareBlockers { player, .. } if player == defend_player && !blocked => { + blocked = true; + let blocks = if let Some(blk) = blocker { + vec![(blk, attacker)] + } else { + vec![] + }; + runner + .declare_blockers(&blocks) + .expect("declaring the intended blocker must succeed"); + } + WaitingFor::DeclareBlockers { .. } => { + if runner.declare_blockers(&[]).is_err() { + break; + } + } + _ => break, + } + } + + attacked && (blocker.is_none() || blocked) && reached_end_of_combat +} + +// --------------------------------------------------------------------------- +// T1 / T1b — the headline positive, and its unshielded control. +// --------------------------------------------------------------------------- + +/// **T1.** CR 604.2 + CR 611.3b: Solitary Confinement's prevention effect is +/// created by a printed STATIC ability, so it is active for as long as the +/// enchantment remains on the battlefield. CR 514.2 ends "until end of turn" and +/// "this turn" effects — this is neither, so the cleanup step must not touch it. +/// +/// Reverting `turns::execute_cleanup`'s predicate to read +/// `ShieldKind::is_shield()` flips BOTH the structural assertion +/// (`replacement_definitions.len() == 1` becomes `0`) and the behavioral one +/// (life 20 becomes 17). +#[test] +fn printed_prevention_survives_turn_boundary_and_prevents() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let sc = add_enchantment_spell_to_hand( + &mut scenario, + P0, + "Solitary Confinement", + SOLITARY_CONFINEMENT_TEXT, + ); + let bear = scenario.add_creature(P1, "Grizzly Bears", 3, 3).id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(sc).resolve(); + outcome.assert_zone(&[sc], Zone::Battlefield); + + // Reach-guard: the printed shield really was installed by the cast pipeline. + let defs = &runner.state().objects[&sc].replacement_definitions; + assert_eq!(defs.len(), 1, "printed shield must be installed on cast"); + assert_eq!( + defs[0].shield_kind, + ShieldKind::Prevention { + amount: PreventionAmount::All + } + ); + assert_eq!( + defs[0].expiry, None, + "CR 604.2: a printed static's shield states no window" + ); + + let life_before = runner.state().players[0].life; + cross_boundary(&mut runner); + assert_eq!( + runner.state().active_player, + P1, + "scenario must have advanced into P1's turn" + ); + + // The defect: today the enchantment is still on the battlefield but its + // shield has been deleted from both the live and base definition lists. + assert_eq!( + runner.state().objects[&sc].zone, + Zone::Battlefield, + "the enchantment itself never left" + ); + assert_eq!( + runner.state().objects[&sc].replacement_definitions.len(), + 1, + "CR 604.2: the printed shield must survive the cleanup step" + ); + assert_eq!( + runner.state().objects[&sc].replacement_definitions[0].expiry, + None + ); + + assert!( + run_combat(&mut runner, P1, bear, P0, None), + "combat reach-guard: the attack must actually have happened" + ); + assert_eq!( + runner.state().players[0].life, + life_before, + "CR 604.2 + CR 611.3b: the printed shield must still prevent next turn" + ); +} + +/// **T1b.** The paired control for T1: with no enchantment at all, the identical +/// cross-boundary attack DOES deal its 3 damage. Without this, T1's "life +/// unchanged" could pass because combat silently failed to run. +#[test] +fn unblocked_attacker_damages_an_unshielded_controller() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let bear = scenario.add_creature(P1, "Grizzly Bears", 3, 3).id(); + let mut runner = scenario.build(); + + let life_before = runner.state().players[0].life; + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P1); + + assert!( + run_combat(&mut runner, P1, bear, P0, None), + "combat reach-guard" + ); + assert_eq!( + runner.state().players[0].life, + life_before - 3, + "the harness can deal cross-boundary combat damage" + ); +} + +// --------------------------------------------------------------------------- +// T2 — the anti-over-fix guard. +// --------------------------------------------------------------------------- + +/// **T2.** CR 611.2a + CR 514.2: Fog's window rides on its ability's own +/// `duration` ("this turn"), so its shield MUST still die at cleanup. This is the +/// test that makes the naive one-line "just delete the `is_shield()` disjunct" +/// fix unshippable: with no creation-seam stamp at all, Fog's shield survives the +/// boundary, P0 takes 0, and this test goes red. +/// +/// Honest scope: Fog is satisfied by EITHER the `.or_else` ability-duration +/// carrier or the engine's turn default, so it discriminates neither +/// individually. T7 and T8 are the discriminating tests for those. +#[test] +fn fog_prevention_shield_expires_at_cleanup() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let fog = scenario + .add_spell_to_hand_from_oracle(P0, "Fog", true, FOG_TEXT) + .with_mana_cost(free_cost()) + .id(); + let p0_bear = scenario.add_creature(P0, "Grizzly Bears", 3, 3).id(); + let p1_bear = scenario.add_creature(P1, "Runeclaw Bear", 3, 3).id(); + let mut runner = scenario.build(); + + runner.cast(fog).resolve(); + + // Reach-guard: the parse reached the resolver and a shield was created. + assert_eq!( + runner.state().pending_damage_replacements.len(), + 1, + "Fog's shield must land on the pending registry" + ); + + // Positive half, same turn: the shield is live and doing work. + let p1_life_before = runner.state().players[1].life; + assert!( + run_combat(&mut runner, P0, p0_bear, P1, None), + "combat reach-guard (same turn)" + ); + assert_eq!( + runner.state().players[1].life, + p1_life_before, + "Fog must prevent combat damage during its own turn" + ); + + let p0_life_before = runner.state().players[0].life; + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P1); + + assert!( + runner.state().pending_damage_replacements.is_empty(), + "CR 514.2: Fog's 'this turn' shield must be pruned at cleanup" + ); + assert!( + run_combat(&mut runner, P1, p1_bear, P0, None), + "combat reach-guard (next turn)" + ); + assert_eq!( + runner.state().players[0].life, + p0_life_before - 3, + "CR 514.2: damage must land once Fog's window has ended" + ); +} + +// --------------------------------------------------------------------------- +// T4 — one-shot prevention, never consumed. +// --------------------------------------------------------------------------- + +/// **T4 — a REGRESSION GUARD, not a discriminating test.** Awe Strike already +/// carries `prevention_duration: UntilEndOfTurn`, so its shield is stamped +/// `Some(EndOfTurn)` by the existing effect-level carrier with or without +/// `ReplacementDefinition::prevention_oneshot_shield`'s builder stamp — deleting +/// that stamp leaves this test green. Stated up front so no reader over-reads it. +/// +/// CR 615.3 + CR 615.8 + CR 514.2: a "the next time ... this turn" shield that is +/// never used up still ends at cleanup. +#[test] +fn oneshot_prevention_shield_expires_at_cleanup() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let awe = scenario + .add_spell_to_hand_from_oracle(P0, "Awe Strike", true, AWE_STRIKE_TEXT) + .with_mana_cost(free_cost()) + .id(); + let bear = scenario.add_creature(P1, "Grizzly Bears", 3, 3).id(); + let mut runner = scenario.build(); + + runner.cast(awe).target_object(bear).resolve(); + + // Reach-guard. The shield's host surface is the PENDING REGISTRY, not the + // targeted creature — asserting against `bear` here would be vacuous. + let pending = &runner.state().pending_damage_replacements; + assert_eq!( + pending.len(), + 1, + "Awe Strike's shield must exist pre-boundary" + ); + assert_eq!(pending[0].shield_kind, ShieldKind::PreventionOneShot); + assert!( + pending[0].consume_on_apply, + "CR 615.8: 'the next time' is consumed on apply" + ); + assert_eq!( + pending[0].expiry, + Some(RestrictionExpiry::EndOfTurn), + "CR 615.3 + CR 514.2: the one-shot's window is stamped at creation" + ); + + let life_before = runner.state().players[0].life; + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P1); + + assert!( + runner.state().pending_damage_replacements.is_empty(), + "CR 514.2: an unconsumed one-shot shield still expires at cleanup" + ); + assert!( + run_combat(&mut runner, P1, bear, P0, None), + "combat reach-guard" + ); + assert_eq!( + runner.state().players[0].life, + life_before - 3, + "the shielded creature's damage must land next turn" + ); +} + +// --------------------------------------------------------------------------- +// T3b — an `EndOfCombat` prevention shield can be produced at all, and works. +// --------------------------------------------------------------------------- + +/// Declare `attacker` and `blocker`, then STOP at the first priority window of +/// the declare-blockers step so the caller can cast at instant speed into it. +/// +/// Returns whether both declarations actually happened — the same +/// reach-guard contract as `run_combat`. +#[must_use = "the declarations must be asserted to have actually happened"] +fn declare_attack_and_block( + runner: &mut GameRunner, + attacker_player: PlayerId, + attacker: ObjectId, + defend_player: PlayerId, + blocker: ObjectId, +) -> bool { + let mut attacked = false; + let mut blocked = false; + for _ in 0..400 { + if attacked && blocked { + return matches!(runner.state().waiting_for, WaitingFor::Priority { .. }); + } + match runner.state().waiting_for.clone() { + WaitingFor::Priority { .. } => { + if runner.act(GameAction::PassPriority).is_err() { + break; + } + } + WaitingFor::OrderTriggers { .. } => { + if runner + .act(GameAction::OrderTriggers { order: vec![0] }) + .is_err() + { + break; + } + } + WaitingFor::DeclareAttackers { player, .. } + if player == attacker_player && !attacked => + { + attacked = true; + runner + .declare_attackers(&[(attacker, AttackTarget::Player(defend_player))]) + .expect("declaring the intended attacker must succeed"); + } + WaitingFor::DeclareAttackers { .. } => { + if runner.declare_attackers(&[]).is_err() { + break; + } + } + WaitingFor::DeclareBlockers { player, .. } if player == defend_player && !blocked => { + blocked = true; + runner + .declare_blockers(&[(blocker, attacker)]) + .expect("declaring the intended blocker must succeed"); + } + WaitingFor::DeclareBlockers { .. } => { + if runner.declare_blockers(&[]).is_err() { + break; + } + } + _ => break, + } + } + false +} + +/// Pass priority from mid-combat through the end of the combat phase. +#[must_use = "combat must be asserted to have actually completed"] +fn finish_combat(runner: &mut GameRunner) -> bool { + for _ in 0..400 { + if matches!( + runner.state().phase, + Phase::EndCombat | Phase::PostCombatMain + ) { + return true; + } + match runner.state().waiting_for.clone() { + WaitingFor::Priority { .. } => { + if runner.act(GameAction::PassPriority).is_err() { + break; + } + } + WaitingFor::OrderTriggers { .. } => { + if runner + .act(GameAction::OrderTriggers { order: vec![0] }) + .is_err() + { + break; + } + } + _ => break, + } + } + false +} + +/// **T3b — the production reach-guard for the `EndOfCombat` value.** Sewers of +/// Estark is the corpus's only card whose prevention window rides on +/// `prevention_duration: UntilEndOfCombat`, and its "If it's blocking" gate makes +/// it structurally impossible to create outside combat. This test proves an +/// `EndOfCombat` prevention shield can exist at all AND does work, so +/// `turns.rs::cleanup_expires_end_of_combat_prevention_shield` is not guarding an +/// impossible value. +/// +/// Deliberately asserts NO player's life: the attacker is blocked, so both +/// players read 20 with and without the shield. The discriminating observable is +/// the BLOCKER's survival — Sewers prevents damage dealt to and by the blocking +/// creature, so the 2/2 lives through the 3/3. See the paired control below. +#[test] +fn sewers_of_estark_stamps_end_of_combat_on_the_blocking_creature() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let sewers = scenario + .add_spell_to_hand_from_oracle(P0, "Sewers of Estark", true, SEWERS_OF_ESTARK_TEXT) + .with_mana_cost(free_cost()) + .id(); + let attacker = scenario.add_creature(P0, "Hill Giant", 3, 3).id(); + let blocker = scenario.add_creature(P1, "Grizzly Bears", 2, 2).id(); + let mut runner = scenario.build(); + + runner.advance_to_combat(); + assert!( + declare_attack_and_block(&mut runner, P0, attacker, P1, blocker), + "reach-guard: attacker and blocker must both have been declared" + ); + + runner.cast(sewers).target_object(blocker).resolve(); + + // Structural: the shield is installed on the BLOCKER's own definitions. + let defs = &runner.state().objects[&blocker].replacement_definitions; + assert_eq!( + defs.len(), + 1, + "the shield must land on the blocking creature" + ); + assert_eq!( + defs[0].expiry, + Some(RestrictionExpiry::EndOfCombat), + "CR 511.2: an 'until end of combat' window maps to RestrictionExpiry::EndOfCombat" + ); + assert_eq!(defs[0].valid_card, Some(TargetFilter::SelfRef)); + assert_eq!(defs[0].combat_scope, Some(CombatDamageScope::CombatOnly)); + + assert!(finish_combat(&mut runner), "combat reach-guard"); + + // Behavioral: the shielded 2/2 survives the 3/3 it blocked. + assert_eq!( + runner.state().objects[&blocker].zone, + Zone::Battlefield, + "the shielded blocker must survive combat damage" + ); + + // Pre-existing behavior: the window ends with the combat phase. + assert!( + runner.state().objects[&blocker] + .replacement_definitions + .as_slice() + .is_empty(), + "CR 511.2: effects that last 'until end of combat' expire at the end of the combat phase" + ); +} + +/// The paired control for T3b: without the Sewers cast, the identical 2/2 blocker +/// dies to the identical 3/3. This is what makes T3b's survival assertion +/// non-vacuous. +#[test] +fn blocked_creature_dies_without_the_sewers_shield() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let attacker = scenario.add_creature(P0, "Hill Giant", 3, 3).id(); + let blocker = scenario.add_creature(P1, "Grizzly Bears", 2, 2).id(); + let mut runner = scenario.build(); + + runner.advance_to_combat(); + assert!( + run_combat(&mut runner, P0, attacker, P1, Some(blocker)), + "combat reach-guard" + ); + assert_eq!( + runner.state().objects[&blocker].zone, + Zone::Graveyard, + "an unshielded 2/2 dies to a 3/3 it blocked" + ); +} + +/// The condition-gate sibling: cast at a NON-blocking creature in the precombat +/// main phase, Sewers of Estark creates no shield at all. This is why the +/// `EndOfCombat` cleanup arm has no reachable production path today and is tested +/// at unit level in `turns.rs` instead. +#[test] +fn sewers_of_estark_creates_no_shield_outside_combat() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let sewers = scenario + .add_spell_to_hand_from_oracle(P0, "Sewers of Estark", true, SEWERS_OF_ESTARK_TEXT) + .with_mana_cost(free_cost()) + .id(); + let bear = scenario.add_creature(P1, "Grizzly Bears", 2, 2).id(); + let mut runner = scenario.build(); + + let outcome = runner.cast(sewers).target_object(bear).resolve(); + // Reach-guard: the spell genuinely resolved rather than being stuck on the + // stack or fizzling on an illegal target. + outcome.assert_zone(&[sewers], Zone::Graveyard); + assert!( + runner.state().objects[&bear] + .replacement_definitions + .as_slice() + .is_empty(), + "the 'if it's blocking' condition gate must refuse outside combat" + ); + assert!(runner.state().pending_damage_replacements.is_empty()); +} + +// --------------------------------------------------------------------------- +// T5 — the multi-authority hostile fixture. +// --------------------------------------------------------------------------- + +/// **T5.** One host object carrying TWO printed `expiry: None` definitions (Fog +/// Bank's single sentence compiles to "dealt to" AND "dealt by") plus a staged +/// resolution-shaped `Some(EndOfTurn)` definition of the IDENTICAL +/// `ShieldKind::Prevention { All }` value. They are indistinguishable by kind and +/// distinguishable only by the latched `expiry`, so this proves the binding is +/// per-definition and latched at creation rather than per-object or re-derived at +/// prune time. +#[test] +fn printed_and_turn_bound_shields_on_one_host_part_ways_at_cleanup() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let fog_bank = scenario + .add_creature_from_oracle(P0, "Fog Bank", 0, 2, FOG_BANK_TEXT) + .with_replacement_definition( + ReplacementDefinition::new(ReplacementEvent::DamageDone) + .valid_card(TargetFilter::SelfRef) + .prevention_shield(PreventionAmount::All) + .expiry(RestrictionExpiry::EndOfTurn), + ) + .id(); + let bear = scenario.add_creature(P1, "Grizzly Bears", 3, 3).id(); + let mut runner = scenario.build(); + + // Reach-guard: 2 printed + 1 staged. Fog Bank contributes TWO definitions. + assert_eq!( + runner.state().objects[&fog_bank] + .replacement_definitions + .len(), + 3, + "two printed Fog Bank definitions plus the staged turn-bound one" + ); + + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P1); + + let defs = &runner.state().objects[&fog_bank].replacement_definitions; + assert_eq!( + defs.len(), + 2, + "CR 604.2 vs CR 514.2: exactly the two printed definitions survive" + ); + assert!( + defs.as_slice().iter().all(|d| d.expiry.is_none()), + "the survivors are the ones with no stated window" + ); + + // Fog Bank still does its job in the new turn: it blocks the 3/3 and takes + // no damage, and deals none. + assert!( + run_combat(&mut runner, P1, bear, P0, Some(fog_bank)), + "combat reach-guard (blocked)" + ); + assert_eq!( + runner.state().objects[&fog_bank].zone, + Zone::Battlefield, + "CR 604.2: Fog Bank's printed prevention must still work next turn" + ); + assert_eq!( + runner.state().objects[&fog_bank].damage_marked, + 0, + "no combat damage may be marked on Fog Bank" + ); +} + +// --------------------------------------------------------------------------- +// T7 — the duration-less resolution class (the engine's turn default). +// --------------------------------------------------------------------------- + +/// **T7.** Reverse Damage reaches `prevent_damage::resolve` with NO window on +/// either carrier — `prevention_duration: None` and `ability.duration: None` — +/// because the parser drops its printed "this turn". It is the discriminating +/// test for `ReplacementDefinition::with_resolution_shield_expiry`'s engine +/// default: delete that one line and this class of shields becomes immortal while +/// every other test in this file stays green (Fog and Morningtide's Light both +/// carry a window on `ability.duration`; Awe Strike carries one on +/// `prevention_duration`). +/// +/// The shield is deliberately hosted on the layer-stable pending registry (the +/// spell exiles itself to the graveyard), and the card is chosen over Circle of +/// Protection: Red, whose object-hosted shield is destroyed by the next layer +/// pass long before cleanup runs. +#[test] +fn reverse_damage_shield_expires_at_cleanup_with_no_duration_on_either_carrier() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let rd = scenario + .add_spell_to_hand_from_oracle(P0, "Reverse Damage", true, REVERSE_DAMAGE_TEXT) + .with_mana_cost(free_cost()) + .id(); + // No colour requirement: Reverse Damage's prompt is "a source of your + // choice" with no restriction. A plain vanilla creature is the right fixture. + let bear = scenario.add_creature(P1, "Grizzly Bears", 3, 3).id(); + let mut runner = scenario.build(); + + // Reach-guard, pre-cast. + assert!(runner.state().pending_damage_replacements.is_empty()); + + // Cast at instant speed on P1's turn so the shield's own turn is P1's. + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P1); + // No mana re-seed is needed: the fixture's Reverse Damage carries a free + // mana cost, so the empty post-boundary pool cannot block the cast. + for _ in 0..8 { + if matches!(runner.state().waiting_for, WaitingFor::Priority { player } if player == P0) { + break; + } + if runner.act(GameAction::PassPriority).is_err() { + break; + } + } + assert!( + matches!(runner.state().waiting_for, WaitingFor::Priority { player } if player == P0), + "P0 must hold priority to cast at instant speed on P1's turn" + ); + runner.cast(rd).resolve(); + + // Resolution parks on the CR 609.7a source choice — drive the round-trip. + assert!( + matches!( + runner.state().waiting_for, + WaitingFor::DamageSourceChoice { .. } + ), + "reach-guard: the ChosenDamageSource branch under test must be reached" + ); + runner + .act(GameAction::ChooseDamageSource { source: bear }) + .expect("choosing the damage source must succeed"); + + let pending = &runner.state().pending_damage_replacements; + assert_eq!(pending.len(), 1, "the shield must exist after the choice"); + assert_eq!( + pending[0].shield_kind, + ShieldKind::Prevention { + amount: PreventionAmount::All + } + ); + assert_eq!( + pending[0].damage_source_filter, + Some(TargetFilter::SpecificObject { id: bear }), + "reach-guard: the shield is genuinely scoped to the chosen source" + ); + // THE REVERT-FAILING ASSERTION. Measured `None` before the fix. + assert_eq!( + pending[0].expiry, + Some(RestrictionExpiry::EndOfTurn), + "the engine turn default must stamp a shield with no window on either carrier" + ); + + // Behavioral positive half, SAME turn — this is what makes the negative half + // below non-vacuous. + let life_before = runner.state().players[0].life; + assert!( + run_combat(&mut runner, P1, bear, P0, None), + "combat reach-guard (same turn)" + ); + assert_eq!( + runner.state().players[0].life, + life_before, + "the shield must prevent the chosen source's damage in its own turn" + ); + + // Cross two boundaries back to P1's next turn. + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P0); + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P1); + assert!( + runner.state().pending_damage_replacements.is_empty(), + "CR 514.2: the duration-less resolution shield must be pruned at cleanup" + ); + + let life_before = runner.state().players[0].life; + assert!( + run_combat(&mut runner, P1, bear, P0, None), + "combat reach-guard (later turn)" + ); + assert_eq!( + runner.state().players[0].life, + life_before - 3, + "damage must land once the engine's turn window has ended" + ); +} + +// --------------------------------------------------------------------------- +// T8 — the ability-duration carrier. +// --------------------------------------------------------------------------- + +/// **T8.** CR 611.2a names BOTH duration carriers: "as stated by the spell OR +/// ABILITY creating it". Morningtide's Light states "Until your next turn" on the +/// ability, not on the prevention effect, so its window can only be read through +/// `prevent_damage::resolve`'s `.or_else(expiry_from_duration(ability.duration))` +/// fallback. Cut that fallback and step 2 fails immediately. +/// +/// The negative half (step 5) is what stops the fallback from being an +/// immortality bug: the `UntilPlayerNextTurn` prune at P0's untap step must +/// remove it. +#[test] +fn ability_duration_prevention_shield_survives_to_controllers_next_turn() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let ml = scenario + .add_spell_to_hand_from_oracle(P0, "Morningtide's Light", false, MORNINGTIDES_LIGHT_TEXT) + .with_mana_cost(free_cost()) + .id(); + let bear = scenario.add_creature(P1, "Grizzly Bears", 3, 3).id(); + let mut runner = scenario.build(); + + // "Exile any number of target creatures" — cast with zero targets. + runner.cast(ml).target_objects(&[]).resolve(); + + // Reach-guard: the shield lands on the pending registry (the Sorcery exiles + // itself), correctly scoped to its controller. + let pending = &runner.state().pending_damage_replacements; + assert_eq!(pending.len(), 1, "the prevention clause must have resolved"); + assert_eq!( + pending[0].shield_kind, + ShieldKind::Prevention { + amount: PreventionAmount::All + } + ); + // THE REVERT-FAILING ASSERTION. Measured `None` before the fix. + assert_eq!( + pending[0].expiry, + Some(RestrictionExpiry::UntilPlayerNextTurn { player: P0 }), + "CR 611.2a: the ability's stated 'Until your next turn' must be the window" + ); + + // It survives a cleanup step it dies at today. + let life_before = runner.state().players[0].life; + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P1); + assert_eq!( + runner.state().pending_damage_replacements.len(), + 1, + "CR 611.2a: an 'until your next turn' window outlives its own turn's cleanup" + ); + + assert!( + run_combat(&mut runner, P1, bear, P0, None), + "combat reach-guard" + ); + assert_eq!( + runner.state().players[0].life, + life_before, + "the card's actual promise: damage to P0 is prevented during P1's turn" + ); + + // The negative half: the window really does end at P0's next turn. + cross_boundary(&mut runner); + assert_eq!(runner.state().active_player, P0); + assert!( + runner.state().pending_damage_replacements.is_empty(), + "the UntilPlayerNextTurn prune must fire — the shield is not immortal" + ); +} From b6cf0d1d0b1e21fe56560df2352aa42dfefc9c07 Mon Sep 17 00:00:00 2001 From: alicewonderland-dev <300129165+alicewonderland-dev@users.noreply.github.com> Date: Mon, 24 Aug 2026 05:19:17 -0400 Subject: [PATCH 2/7] fix(engine,parser): record a prevention clause's own stated window in expiry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the cleanup-seam fix, from an independent implementation review. Making `expiry` the single lifetime authority means a stated window the PARSER drops now produces a definition nothing can ever remove — the old `shield_kind` blanket used to mask that. Urza's Science Fair Project is the shipped counterexample. Its die-roll row "2 — Prevent all combat damage it would deal this turn." lowers to a printed, object-hosted `DamageDone` shield with neither `valid_card` nor `damage_target_filter`, on an ARTIFACT CREATURE. Being a permanent, it is not neutralized by `object_replacement_candidate_applies`' [Battlefield, Command] zone gate the way the eight Instant/Sorcery hosts of the same shape are. Measured across one turn boundary with an unblocked 3/3: defending player 20 -> 17 before the cleanup-seam change, 20 -> 20 after — i.e. a self-limiting one-turn parse defect had become a permanent, game-wide combat-damage lockout for either player. The fix is at the parser, not the prune: the card states a window, so the definition should carry it. Reinstating the `shield_kind` blanket would just restore the original bug. Position is load-bearing, so `stated_clause_expiry` delegates to the existing positional authority `oracle_effect::lower::strip_trailing_duration` rather than scanning for a duration phrase. "this turn" also occurs inside SUBORDINATE clauses that are conditions rather than windows — Neriv, Heart of the Storm's "a creature you control that entered this turn would deal damage" is a printed static on a real, format-legal permanent, and stamping it would DELETE a correct replacement at the next cleanup step. Only a clause-final duration is the effect's own window. Player-relative windows stay unmapped (they need a `PlayerId` that does not exist at parse time) and event-scoped durations stay unmapped (their prunes key on the event, not on `expiry`). Also from the same review: the `EndOfCombat` cleanup test now stages and asserts the BASE surface, which is what its own justification is about — previously deleting the `base_replacement_definitions` retain left it green. And `expiry_from_duration`'s `Duration::Permanent` arm no longer claims the effect "is not turn-bound", which was the opposite of what happens to a shield two frames later. Claimed parse impact: 2 cards, both corrections — Urza's Science Fair Project (null -> EndOfTurn) and Winter's Chill (null -> EndOfCombat). Both state those windows in printed text. Proven complete by construction: this parser function never set `expiry` before, and the new stamp has exactly one call site. Co-Authored-By: Claude Opus 5 --- .../game/effects/add_target_replacement.rs | 9 +- crates/engine/src/game/turns.rs | 28 +++- .../engine/src/parser/oracle_replacement.rs | 130 ++++++++++++++- ...printed_damage_prevention_survives_turn.rs | 150 +++++++++++++++++- 4 files changed, 310 insertions(+), 7 deletions(-) diff --git a/crates/engine/src/game/effects/add_target_replacement.rs b/crates/engine/src/game/effects/add_target_replacement.rs index caaeb8f398..b03785543c 100644 --- a/crates/engine/src/game/effects/add_target_replacement.rs +++ b/crates/engine/src/game/effects/add_target_replacement.rs @@ -58,7 +58,14 @@ pub(crate) fn expiry_from_duration( Some(Duration::ForAsLongAs { .. }) => None, Some(Duration::UntilSourceExilesAnotherCard) => None, Some(Duration::UntilOpponentBecomesMonarch) => None, - // CR 611.2a: no duration at all — the effect is not turn-bound. + // CR 611.2a: an explicitly permanent window has no `RestrictionExpiry` + // counterpart, because none of them means "never ends". `None` here is + // "unmapped", NOT "durable" — a SHIELD that reaches + // `replacement_with_ability_expiry` still picks up the engine's + // `EndOfTurn` fallback from `with_resolution_shield_expiry` two frames + // below; only a non-shield rider actually stays durable. No corpus + // prevention ability carries `Duration::Permanent`, so nothing reaches + // that combination today. Some(Duration::Permanent) => None, } } diff --git a/crates/engine/src/game/turns.rs b/crates/engine/src/game/turns.rs index f4f16180af..fc67d47596 100644 --- a/crates/engine/src/game/turns.rs +++ b/crates/engine/src/game/turns.rs @@ -8417,6 +8417,11 @@ mod tests { /// Negative sibling in the same test: a CR 604.2 printed-static-shaped /// definition (`expiry: None`) on the same object must SURVIVE, proving the arm /// is expiry-keyed and not a blanket over `shield_kind`. + /// + /// The BASE surface is the one the doc comment's justification is about, so it + /// is staged and asserted here too — an earlier revision installed and asserted + /// only on `replacement_definitions`, which left the test green when the + /// `base_replacement_definitions` retain was deleted outright. #[test] fn cleanup_expires_end_of_combat_prevention_shield() { use crate::types::ability::{PreventionAmount, ReplacementDefinition, TargetFilter}; @@ -8446,10 +8451,15 @@ mod tests { { let obj = state.objects.get_mut(&id).unwrap(); - obj.replacement_definitions.push(combat_bound); - obj.replacement_definitions.push(durable); - // Reach-guard: both definitions really are installed before cleanup. + obj.replacement_definitions.push(combat_bound.clone()); + obj.replacement_definitions.push(durable.clone()); + let base = std::sync::Arc::make_mut(&mut obj.base_replacement_definitions); + base.push(combat_bound); + base.push(durable); + // Reach-guard: both definitions really are installed on BOTH surfaces + // before cleanup. assert_eq!(obj.replacement_definitions.len(), 2); + assert_eq!(obj.base_replacement_definitions.len(), 2); } let mut events = Vec::new(); @@ -8465,6 +8475,18 @@ mod tests { obj.replacement_definitions[0].expiry, None, "CR 604.2: the surviving definition is the printed-static-shaped one" ); + // CR 500.1 + CR 511.3: `complete_end_combat_teardown` never touches the + // base surface, so this arm is the sole base-side catcher. Deleting the + // base-side retain must turn this red. + assert_eq!( + obj.base_replacement_definitions.len(), + 1, + "the EndOfCombat shield must be pruned from base_replacement_definitions too" + ); + assert_eq!( + obj.base_replacement_definitions[0].expiry, None, + "CR 604.2: the surviving BASE definition is the printed-static-shaped one" + ); } /// CR 402.2: A player with NoMaximumHandSize skips the discard-to-7 check. diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index d6b469c0a1..c81415beff 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -10677,6 +10677,68 @@ pub(crate) fn parse_bidirectional_damage_prevention( Some(vec![recipient_half, source_half]) } +/// CR 611.2a + CR 514.2: The window a prevention clause states FOR ITSELF, +/// mapped to the `RestrictionExpiry` the runtime prunes read. +/// +/// `turns::execute_cleanup` reads `expiry` and only `expiry` — no prune infers a +/// lifetime from `shield_kind` — so a stated window the parser drops produces a +/// definition nothing can ever remove. That is not hypothetical: Urza's Science +/// Fair Project's die-roll result row "Prevent all combat damage it would deal +/// this turn." lowers to a printed, object-hosted `DamageDone` shield with +/// neither `valid_card` nor `damage_target_filter`, on an Artifact Creature — a +/// PERMANENT, so unlike the eight Instant/Sorcery hosts of the same shape it is +/// not neutralized by `object_replacement_candidate_applies`' `[Battlefield, +/// Command]` zone gate. Unstamped, it prevents ALL combat damage in the game, to +/// or from either player, for the rest of the game. +/// +/// POSITION IS THE WHOLE POINT, so this delegates to the existing positional +/// authority `oracle_effect::lower::strip_trailing_duration` rather than +/// scanning the clause for a +/// duration phrase. "this turn" also occurs inside SUBORDINATE clauses that are +/// conditions rather than windows — Neriv, Heart of the Storm's "a creature you +/// control that entered this turn would deal damage" and Aether Revolt's "as +/// long as a permanent left the battlefield under your control this turn" are +/// both printed statics hosted on PERMANENTS, and stamping either would delete a +/// correct replacement at the next cleanup step. Only a clause-FINAL duration is +/// the effect's own window, and `strip_trailing_duration` already owns that +/// judgement, including its per-turn-quantity lookback guards. +/// +/// CR 604.2: a printed static ability states no window at all, so the ordinary +/// case returns `None` and the definition stays durable — exactly what Solitary +/// Confinement, Fog Bank, Nine Lives and Pariah require. +fn stated_clause_expiry(clause_lower: &str) -> Option { + use crate::types::ability::RestrictionExpiry; + + let (_, duration) = super::oracle_effect::lower::strip_trailing_duration(clause_lower); + match duration? { + // CR 514.2: "this turn" / "until end of turn" ends at the cleanup step. + Duration::UntilEndOfTurn => Some(RestrictionExpiry::EndOfTurn), + // CR 511.2: "this combat" / "until end of combat" expires at the end of + // the combat phase; `complete_end_combat_teardown` catches the live and + // pending surfaces and the cleanup prune catches the base surface. + Duration::UntilEndOfCombat => Some(RestrictionExpiry::EndOfCombat), + // The player-relative windows need a concrete `PlayerId` that does not + // exist at parse time (`RestrictionExpiry::UntilPlayerNextTurn` and + // `UntilEndOfNextTurnOf` both carry one), so only the runtime seam + // (`effects::add_target_replacement::expiry_from_duration`) can resolve + // them. Deliberately unmapped rather than approximated; no printed + // prevention clause in the corpus states one. + Duration::UntilNextTurnOf { .. } | Duration::UntilEndOfNextTurnOf { .. } => None, + // Not turn windows: these end on an event or a condition, and the prunes + // that end them key on that event — the battlefield-exit prune in + // `layers.rs`, the CR 611.2b `ReplacementCondition` gate — not on + // `expiry`. Stamping a turn window here would cut them short. + Duration::UntilHostLeavesPlay + | Duration::UntilNextStepOf { .. } + | Duration::ForAsLongAs { .. } + | Duration::UntilSourceExilesAnotherCard + | Duration::UntilOpponentBecomesMonarch => None, + // CR 604.2: an explicitly permanent window is the printed-static case — + // no expiry, and the definition must survive every cleanup step. + Duration::Permanent => None, + } +} + /// CR 615: Parse damage prevention replacement effects. /// Handles: /// - "prevent all combat damage that would be dealt [this turn]" (Fog, Moments Peace) @@ -10948,6 +11010,16 @@ fn parse_damage_prevention_replacement( if let Some(sf) = damage_source_filter { def = def.damage_source_filter(sf); } + // CR 611.2a + CR 514.2: record the window this clause states for ITSELF, so + // `expiry` — the single lifetime authority `turns::execute_cleanup` reads — + // carries what the card actually says instead of silently dropping it. A + // CR 604.2 printed static states no window, so `stated_clause_expiry` + // returns `None` for it and the definition stays durable. + if def.expiry.is_none() { + if let Some(expiry) = stated_clause_expiry(working_lower) { + def = def.expiry(expiry); + } + } // Capture whether the recipient filter was event-driven (typed // `valid_card`) before moving it onto `def` — the follow-up rewrite // below uses this signal to distinguish the Vigor cohort (rewrite @@ -12110,7 +12182,8 @@ mod tests { use crate::parser::oracle::parse_oracle_text; use crate::types::ability::{ AbilityCondition, Comparator, ControllerRef, CountScope, QuantityExpr, - QuantityModification, QuantityRef, ReplacementCondition, ShieldKind, ZoneRef, + QuantityModification, QuantityRef, ReplacementCondition, RestrictionExpiry, ShieldKind, + ZoneRef, }; use crate::types::card_type::{CoreType, Supertype}; use crate::types::keywords::Keyword; @@ -14047,6 +14120,61 @@ mod tests { } } + /// CR 611.2a + CR 514.2 vs CR 604.2: `expiry` records the window a prevention + /// clause states FOR ITSELF, and nothing else. Shape-level companion to the + /// runtime pin in + /// `tests/integration/printed_damage_prevention_survives_turn.rs::turn_windowed_printed_shield_is_stamped_and_does_not_survive_cleanup`. + /// + /// The discriminating half is the third case: "this turn" occurring inside a + /// SUBORDINATE condition is not a window, and stamping it would delete a + /// correct printed static (Neriv, Heart of the Storm — a real, format-legal + /// PERMANENT) at the next cleanup step. + #[test] + fn prevention_expiry_records_only_a_clause_final_stated_window() { + // Verbatim Urza's Science Fair Project, result row 2. A trailing "this + // turn" IS this effect's own window. + let windowed = parse_replacement_line( + "Prevent all combat damage it would deal this turn.", + "Urza's Science Fair Project", + ) + .expect("the prevention clause must still parse"); + assert!(windowed.shield_kind.is_shield()); + assert_eq!( + windowed.expiry, + Some(RestrictionExpiry::EndOfTurn), + "CR 611.2a: a clause-final 'this turn' is the effect's stated window" + ); + + // Verbatim Solitary Confinement, line 3. CR 604.2: a printed static + // states no window, so it must stay durable. + let durable = parse_replacement_line( + "Prevent all damage that would be dealt to you.", + "Solitary Confinement", + ) + .expect("the printed static must still parse"); + assert!(durable.shield_kind.is_shield()); + assert_eq!( + durable.expiry, None, + "CR 604.2: a printed static's shield carries no window" + ); + + // Verbatim Neriv, Heart of the Storm. "this turn" sits inside the + // subordinate "that entered this turn" CONDITION, not at the end of the + // clause — stamping it would prune a correct printed static off a + // permanent at the first cleanup step. + let conditioned = parse_replacement_line( + "If a creature you control that entered this turn would deal damage, \ + it deals twice that much damage instead.", + "Neriv, Heart of the Storm", + ) + .expect("Neriv's printed replacement must still parse"); + assert_eq!( + conditioned.expiry, None, + "CR 604.2: 'entered this turn' is a condition, not a duration — a \ + printed static must not acquire a turn window from it" + ); + } + /// Sibling coverage for the same bare "prevent N of that damage" idiom with /// N > 1 and no source-controller qualifier (Sphere of Purity-style). Pins /// that the fix generalizes to other N and doesn't require an "an opponent diff --git a/crates/engine/tests/integration/printed_damage_prevention_survives_turn.rs b/crates/engine/tests/integration/printed_damage_prevention_survives_turn.rs index ba7a4feaa2..00b2f246d4 100644 --- a/crates/engine/tests/integration/printed_damage_prevention_survives_turn.rs +++ b/crates/engine/tests/integration/printed_damage_prevention_survives_turn.rs @@ -28,8 +28,8 @@ use engine::game::combat::AttackTarget; use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; use engine::types::ability::{ - CombatDamageScope, PreventionAmount, ReplacementDefinition, RestrictionExpiry, ShieldKind, - TargetFilter, + AbilityKind, CombatDamageScope, PreventionAmount, ReplacementDefinition, RestrictionExpiry, + ShieldKind, TargetFilter, }; use engine::types::actions::GameAction; use engine::types::game_state::WaitingFor; @@ -913,3 +913,149 @@ fn ability_duration_prevention_shield_survives_to_controllers_next_turn() { "the UntilPlayerNextTurn prune must fire — the shield is not immortal" ); } + +// --------------------------------------------------------------------------- +// T9 — the complement of T1: a PRINTED def that carries `expiry: None` today +// but whose own clause states a turn window. +// --------------------------------------------------------------------------- + +/// Verbatim Urza's Science Fair Project (MTGJSON `text`). A `{2}` activated die +/// roll whose six results are printed as an em-dash results table; row 2 states +/// its own turn window and is the corpus's only turn-windowed printed shield +/// hosted on a PERMANENT. +const URZAS_SCIENCE_FAIR_PROJECT_TEXT: &str = "{2}: Roll a six-sided die. This creature gets the indicated result.\n1 \u{2014} It gets -2/-2 until end of turn.\n2 \u{2014} Prevent all combat damage it would deal this turn.\n3 \u{2014} It gains vigilance until end of turn.\n4 \u{2014} It gains first strike until end of turn.\n5 \u{2014} It gains flying until end of turn.\n6 \u{2014} It gets +2/+2 until end of turn."; + +/// **T9.** CR 611.2a + CR 514.2, and the counterpart hazard to T1: making +/// `expiry` the single lifetime authority is only safe if every definition that +/// reaches the battlefield with `expiry: None` is genuinely a CR 604.2 printed +/// static that states no window. This card is the corpus's one counterexample. +/// +/// Its row "Prevent all combat damage it would deal this turn." lowers to a +/// printed `DamageDone` shield on the permanent itself, UNSCOPED in both +/// directions (`valid_card: None`, `damage_target_filter: None`). Under the old +/// `is_shield()` blanket the mis-lowering self-limited to one turn; keyed on +/// `expiry` alone and left unstamped it would be immortal — a game-wide "no +/// combat damage is ever dealt, by or to anyone" lock. The parser now records +/// the window the clause states, via the positional `strip_trailing_duration` +/// authority, so cleanup catches it on the very evidence the clause provides. +/// +/// Reverting `parse_damage_prevention_replacement`'s `stated_clause_expiry` +/// stamp flips BOTH the structural assertion (`expiry` becomes `None`, and the +/// shield count after the boundary becomes `(1, 1)`) and the behavioral one +/// (life 20 - 3 = 17 becomes 20). Measured at the pre-fix candidate: `(1, 1)` +/// and life 20. +/// +/// The card's printed type line is Artifact Creature; the scenario stages it as +/// a plain creature because the artifact half is not load-bearing. What makes +/// this the counterexample — and the eight Instant/Sorcery hosts of the same +/// shape harmless — is only that it is a PERMANENT, so its definitions clear +/// `object_replacement_candidate_applies`' `[Battlefield, Command]` zone gate. +#[test] +fn turn_windowed_printed_shield_is_stamped_and_does_not_survive_cleanup() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + stock_libraries(&mut scenario); + let project = scenario + .add_creature_from_oracle( + P0, + "Urza's Science Fair Project", + 4, + 4, + URZAS_SCIENCE_FAIR_PROJECT_TEXT, + ) + // CR 302.6: it entered this turn. Without this the 4/4 is a legal + // attacker in P0's own turn and `cross_boundary`'s + // `advance_to_phase(Phase::End)` STALLS SILENTLY at DeclareAttackers — + // the stall this file's helper doc warns about. Orthogonal to the + // replacement under test. + .with_summoning_sickness() + .id(); + let bear = scenario.add_creature(P1, "Grizzly Bears", 3, 3).id(); + let mut runner = scenario.build(); + + // Reach-guard: the card really is a battlefield permanent with its Oracle + // text applied, so everything below is about a live object. + assert_eq!(runner.state().objects[&project].zone, Zone::Battlefield); + assert!( + runner.state().objects[&project] + .abilities + .iter() + .any(|a| a.kind == AbilityKind::Activated), + "reach-guard: the {{2}} die-roll activated ability parsed, so the Oracle \ + text was really applied to the object" + ); + + let shields_on = |runner: &GameRunner| -> (usize, usize) { + let obj = &runner.state().objects[&project]; + ( + obj.replacement_definitions + .iter_unchecked() + .filter(|r| r.shield_kind.is_shield()) + .count(), + obj.base_replacement_definitions + .iter() + .filter(|r| r.shield_kind.is_shield()) + .count(), + ) + }; + + // Reach-guard + THE REVERT-FAILING STRUCTURAL ASSERTION. The mis-lowered + // shield really is installed on both surfaces (so the prune below is not + // vacuous), and it now carries the window its own clause states. Measured + // `expiry: None` before the fix. + assert_eq!( + shields_on(&runner), + (1, 1), + "reach-guard: the printed shield is installed on both surfaces" + ); + let installed = runner.state().objects[&project] + .base_replacement_definitions + .iter() + .find(|r| r.shield_kind.is_shield()) + .expect("reach-guard: the shield is on the base surface"); + assert_eq!( + installed.shield_kind, + ShieldKind::Prevention { + amount: PreventionAmount::All + } + ); + assert_eq!( + installed.expiry, + Some(RestrictionExpiry::EndOfTurn), + "CR 611.2a + CR 514.2: the clause's own 'this turn' must be recorded as \ + the definition's window" + ); + + let life_before = runner.state().players[0].life; + cross_boundary(&mut runner); + assert_eq!( + runner.state().active_player, + P1, + "scenario must have advanced into P1's turn" + ); + assert_eq!( + runner.state().objects[&project].zone, + Zone::Battlefield, + "the permanent itself never left — an unpruned shield of its would still apply" + ); + // The finding's exact framing: a printed shield whose clause states a turn + // window must not be alive after the cleanup step, on EITHER surface. + assert_eq!( + shields_on(&runner), + (0, 0), + "CR 514.2: a printed shield stating 'this turn' must not survive cleanup" + ); + + // P1 attacks P0 with an unblocked 3/3. The shield was unscoped in both + // directions, so while alive it prevented this damage too. + assert!( + run_combat(&mut runner, P1, bear, P0, None), + "combat reach-guard: the attack must actually have happened" + ); + // THE REVERT-FAILING BEHAVIORAL ASSERTION. Measured 20 -> 20 before the fix. + assert_eq!( + runner.state().players[0].life, + life_before - 3, + "CR 514.2: with the turn-windowed shield gone, ordinary combat damage lands" + ); +} From c897c3254db7d28c4e320fe1c6592b9e64955c2c Mon Sep 17 00:00:00 2001 From: alicewonderland-dev <300129165+alicewonderland-dev@users.noreply.github.com> Date: Mon, 24 Aug 2026 06:27:28 -0400 Subject: [PATCH 3/7] fix(parser): scope a prevention clause's stated window to the clause itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round on the expiry stamp found the mechanism did not have the positional discipline its own doc comment claimed. `strip_trailing_duration` reads a trailing duration off the whole line, so the stamp fired on windows that belong to something else. Measured through the real parse entry point, all of these were stamped `EndOfTurn`: * "...dealt to creatures that attacked this turn." — the phrase belongs to the relative clause; the parser had already proved it by emitting `valid_card: Typed{properties:[AttackedThisTurn]}`, then stamped anyway * "...dealt to you if you've gained 3 or more life this turn." * "...dealt to you as long as a permanent left the battlefield under your control this turn." — the suffix form of the condition; only the prefix form was being lifted out * "Prevent all damage that would be dealt to you. Target creature gets +1/+1 until end of turn." — the shield inherited a DIFFERENT SENTENCE'S duration No shipped card reached any of these, so this is not a regression — but the trap is the reported bug reintroduced card by card: stamping a window onto a printed static deletes it at the first cleanup step. The unit test's "discriminating" case only passed because its "this turn" sat mid-sentence, so it was not discriminating at all. `prevention_clause_owns_trailing_window` now gates the stamp on three positional checks, all of which FAIL CLOSED — no stamp means durable, the pre-existing safe behaviour: 1. the window is read from the sentence carrying the "prevent" verb, never from the line; 2. a subordinating conjunction (if / as long as / while / unless / when / whenever) after the prevention verb means the window cannot be attributed to the effect; 3. a nested relative clause is detected by delegating to `oracle_target::parse_that_clause_suffix` — the same authority `strip_trailing_duration`'s own guard uses — scanned at every word boundary rather than the first " that ". The doc no longer claims `strip_trailing_duration` owns that judgement; it states what that function does own and why it is deliberately not extended (it is a shared authority every effect line runs through). `parse_bidirectional_damage_prevention` is a separate dispatch arm that bypasses the single-def path entirely, and had no stamp at all, so "...dealt to and dealt by enchanted creature this turn." produced two unbounded shields. It now stamps `base` through the same function, before the halves are cloned, so the two recognizers agree by construction. Player- and step-relative windows (`UntilNextTurnOf`, `UntilEndOfNextTurnOf`, `UntilNextStepOf`) now map to `EndOfTurn` rather than `None`: at the resolution seam an unmapped `None` is caught by `with_resolution_shield_expiry`, but at the printed seam `None` is immortal, and bounded-and-slightly-early beats never-ends. The old justification claimed a step-keyed prune exists; it does not. Claimed parse impact, measured against BASE by regenerating card data from the base parser and diffing full card content across all 35798 entries — 4 cards, every one a correction, all `expiry: null` -> a window the card's own text states: Urza's Science Fair Project null -> EndOfTurn ("...this turn.") Winter's Chill null -> EndOfCombat ("...this combat.") Revealing Wind null -> EndOfTurn (sentence-scoped) Undergrowth null -> EndOfTurn (sentence-scoped) The last two are new here: their prevention sentence's own "this turn" was previously lost because the whole-line strip read the LAST sentence. Named residual gap, recorded in the code: the gates fail closed, so a window sitting before a trailing subordinate clause is left durable rather than stamped. Zero corpus cards; the safe direction. Co-Authored-By: Claude Opus 5 --- .../engine/src/parser/oracle_replacement.rs | 376 ++++++++++++++++-- 1 file changed, 336 insertions(+), 40 deletions(-) diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index c81415beff..bc4b06cb0a 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -10670,6 +10670,20 @@ pub(crate) fn parse_bidirectional_damage_prevention( if let Some(cs) = combat_scope { base = base.combat_scope(cs); } + // CR 611.2a + CR 514.2: Stamp the clause's own stated window through the SAME + // authority the single-definition path uses, on `base` — BEFORE the two halves + // are cloned, so the recipient half and the source half can never disagree + // about the lifetime of one physical sentence. Without this, the ellipsis form + // ("...dealt to and dealt by enchanted creature this turn.") produced two + // shields with `expiry: None`, i.e. an immortal, game-wide combat-damage + // lockout on any PERMANENT host — the exact defect this commit fixes for the + // single-definition shape. Not corpus-reachable today (all six cards this + // recognizer claims are windowless printed statics), but the class has + // permanent-hosted members one printing away: Deftblade Elite, Urborg Phantom + // and Moonlight Geist all carry this sentence as an ability body. + if let Some(expiry) = stated_clause_expiry(norm_lower) { + base = base.expiry(expiry); + } let recipient_half = base.clone().valid_card(subject.clone()); let source_half = base.damage_source_filter(subject); @@ -10691,25 +10705,38 @@ pub(crate) fn parse_bidirectional_damage_prevention( /// Command]` zone gate. Unstamped, it prevents ALL combat damage in the game, to /// or from either player, for the rest of the game. /// -/// POSITION IS THE WHOLE POINT, so this delegates to the existing positional -/// authority `oracle_effect::lower::strip_trailing_duration` rather than -/// scanning the clause for a -/// duration phrase. "this turn" also occurs inside SUBORDINATE clauses that are -/// conditions rather than windows — Neriv, Heart of the Storm's "a creature you -/// control that entered this turn would deal damage" and Aether Revolt's "as -/// long as a permanent left the battlefield under your control this turn" are -/// both printed statics hosted on PERMANENTS, and stamping either would delete a -/// correct replacement at the next cleanup step. Only a clause-FINAL duration is -/// the effect's own window, and `strip_trailing_duration` already owns that -/// judgement, including its per-turn-quantity lookback guards. +/// POSITION IS THE WHOLE POINT. `oracle_effect::lower::strip_trailing_duration` +/// is the phrase→`Duration` authority and owns the *per-turn-quantity* lookback +/// guards ("where X is the number of tokens you created this turn"), but it does +/// NOT own the judgement this function needs, and it is deliberately not extended +/// to: it is a shared authority every effect line in the parser runs through, and +/// its own relative-clause guard (`target_relative_clause_owns_suffix`) anchors on +/// the FIRST `" that "` in its input, so a NESTED relative clause +/// ("...dealt to creatures that attacked this turn") slips past it. This function +/// therefore adds its own positional gates on top, scoped to the prevention class +/// where a wrong answer is destructive (see `prevention_clause_owns_trailing_window`). +/// The asymmetry is intentional: not stamping leaves a durable definition (the +/// pre-existing, safe behaviour); stamping wrongly deletes a correct printed +/// static off a format-legal permanent at the next cleanup step, which is exactly +/// the Solitary Confinement bug this commit exists to fix. /// /// CR 604.2: a printed static ability states no window at all, so the ordinary /// case returns `None` and the definition stays durable — exactly what Solitary /// Confinement, Fog Bank, Nine Lives and Pariah require. +/// +/// SCOPE NOTE on "`expiry` is the single lifetime authority": that premise is +/// true for definitions the runtime can ever consult, which is MODULO +/// `object_replacement_candidate_applies`' `[Battlefield, Command]` zone gate. +/// Seven turn-windowed printed shield defs in the corpus (Head to Head, Revealing +/// Wind, Sex Appeal, That's No Moonmist, Torrent of Lava, Undergrowth, Winds of +/// Qal Sisma) still emit `expiry: null` and are inert only because they are +/// Instants/Sorceries that never reach the battlefield. The premise holds +/// unconditionally only for PERMANENT hosts. fn stated_clause_expiry(clause_lower: &str) -> Option { use crate::types::ability::RestrictionExpiry; - let (_, duration) = super::oracle_effect::lower::strip_trailing_duration(clause_lower); + let sentence = prevention_clause_owns_trailing_window(clause_lower)?; + let (_, duration) = super::oracle_effect::lower::strip_trailing_duration(sentence); match duration? { // CR 514.2: "this turn" / "until end of turn" ends at the cleanup step. Duration::UntilEndOfTurn => Some(RestrictionExpiry::EndOfTurn), @@ -10717,19 +10744,27 @@ fn stated_clause_expiry(clause_lower: &str) -> Option Some(RestrictionExpiry::EndOfCombat), - // The player-relative windows need a concrete `PlayerId` that does not - // exist at parse time (`RestrictionExpiry::UntilPlayerNextTurn` and - // `UntilEndOfNextTurnOf` both carry one), so only the runtime seam - // (`effects::add_target_replacement::expiry_from_duration`) can resolve - // them. Deliberately unmapped rather than approximated; no printed - // prevention clause in the corpus states one. - Duration::UntilNextTurnOf { .. } | Duration::UntilEndOfNextTurnOf { .. } => None, - // Not turn windows: these end on an event or a condition, and the prunes - // that end them key on that event — the battlefield-exit prune in - // `layers.rs`, the CR 611.2b `ReplacementCondition` gate — not on - // `expiry`. Stamping a turn window here would cut them short. + // CR 500.1 + CR 514.2: These ARE real, bounded windows — the clause said + // so — but `RestrictionExpiry` has no counterpart the parse-time seam can + // build: the player-relative variants (`UntilPlayerNextTurn`, + // `UntilEndOfNextTurnOf`) both carry a concrete `PlayerId` that does not + // exist until resolution, and there is no step-keyed replacement prune at + // all. Approximate to the nearest bounded window instead of dropping to + // `None`, because the two seams are NOT symmetric: at the resolution seam + // (`effects::add_target_replacement::expiry_from_duration`) an unmapped + // `None` is caught by that path's `EndOfTurn` shield fallback, but a + // printed def with `expiry: None` is DURABLE — `turns::execute_cleanup` + // has nothing else to key on, so the shield becomes immortal. A window + // that ends too early is a bounded rules error; an immortal printed + // shield is the game-wide lockout this commit exists to prevent. + Duration::UntilNextTurnOf { .. } + | Duration::UntilEndOfNextTurnOf { .. } + | Duration::UntilNextStepOf { .. } => Some(RestrictionExpiry::EndOfTurn), + // Not turn windows: these end on an event or a condition, so `None` is the + // CORRECT answer, not an unmapped one — the battlefield-exit prune in + // `layers.rs` and the CR 611.2b `ReplacementCondition` gate end them, and + // stamping any turn window here would cut them short. Duration::UntilHostLeavesPlay - | Duration::UntilNextStepOf { .. } | Duration::ForAsLongAs { .. } | Duration::UntilSourceExilesAnotherCard | Duration::UntilOpponentBecomesMonarch => None, @@ -10739,6 +10774,127 @@ fn stated_clause_expiry(clause_lower: &str) -> Option Option<&str> { + let sentence = prevention_verb_sentence(clause_lower)?; + // The prevention grammar (`parse_damage_prevention_replacement` step 1) anchors + // its amount at this same "prevent " verb, so the segment after it is exactly + // the clause whose window is in question. + let after_prevent = strip_after(sentence, "prevent ")?; + + let subordinate_clause_intervenes = + nom_primitives::scan_at_word_boundaries(after_prevent, |input: &str| { + alt(( + tag::<_, _, OracleError<'_>>("whenever "), + tag("when "), + tag("as long as "), + tag("if "), + tag("while "), + tag("unless "), + )) + .parse(input) + }) + .is_some(); + if subordinate_clause_intervenes { + return None; + } + + let relative_clause_owns_suffix = + nom_primitives::scan_at_word_boundaries(after_prevent, |input: &str| { + let fail = || nom::Err::Error(OracleError::new(input, nom::error::ErrorKind::Fail)); + let (_, consumed) = + super::oracle_target::parse_that_clause_suffix(input, None).ok_or_else(fail)?; + let remaining = input.get(consumed..).ok_or_else(fail)?; + // The relative clause owns the suffix only if it runs to the end of the + // clause; a genuine OUTER window after it ("... that attacked this turn + // until end of turn") leaves a remainder and is still readable. + ( + multispace0, + opt(alt((tag::<_, _, OracleError<'_>>("."), tag(",")))), + multispace0, + eof, + ) + .parse(remaining) + }) + .is_some(); + if relative_clause_owns_suffix { + return None; + } + + Some(sentence) +} + +/// The sentence of `clause_lower` that carries the "prevent " verb, or `None` if +/// no sentence does. +/// +/// Sentence boundaries are walked with `take_until(". ")` rather than a string +/// split so the whole traversal stays inside the combinator grammar. The FIRST +/// prevention sentence is returned, matching the `strip_after(.., "prevent ")` +/// anchor `parse_damage_prevention_replacement` uses to extract the amount — one +/// definition is built per line, from that first prevention verb, so its window +/// must be read from the same sentence. +/// +/// The `"prevent "` tag deliberately carries its trailing space: it must match the +/// verb ("prevent all", "prevent the next 3", "prevent 2 of that damage") and NOT +/// the past participle in a follow-up rider ("if damage is prevented this way", +/// Stormwild Capridor), which states no window of its own. +fn prevention_verb_sentence(clause_lower: &str) -> Option<&str> { + let mut remaining = clause_lower; + loop { + let (sentence, tail) = match take_until::<_, _, OracleError<'_>>(". ").parse(remaining) { + Ok((separator_onwards, sentence)) => { + match tag::<_, _, OracleError<'_>>(". ").parse(separator_onwards) { + Ok((tail, _)) => (sentence, tail), + Err(_) => (remaining, ""), + } + } + // No further sentence boundary: the rest of the line is one sentence. + Err(nom::Err::Error(_) | nom::Err::Failure(_) | nom::Err::Incomplete(_)) => { + (remaining, "") + } + }; + let carries_prevention_verb = + nom_primitives::scan_at_word_boundaries(sentence, |input: &str| { + tag::<_, _, OracleError<'_>>("prevent ").parse(input) + }) + .is_some(); + if carries_prevention_verb { + return Some(sentence); + } + if tail.is_empty() { + return None; + } + remaining = tail; + } +} + /// CR 615: Parse damage prevention replacement effects. /// Handles: /// - "prevent all combat damage that would be dealt [this turn]" (Fog, Moments Peace) @@ -11015,6 +11171,13 @@ fn parse_damage_prevention_replacement( // carries what the card actually says instead of silently dropping it. A // CR 604.2 printed static states no window, so `stated_clause_expiry` // returns `None` for it and the definition stays durable. + // + // The `is_none()` guard is VACUOUSLY TRUE today and is kept only as a + // never-clobber assertion: nothing between `ReplacementDefinition::new` above + // and this line writes `expiry` (`prevention_shield` deliberately does not + // stamp one). It is not guarding a real prior write — if a future arm starts + // setting `expiry` earlier, that arm's answer is the more specific one and + // must win over this generic clause-window read. if def.expiry.is_none() { if let Some(expiry) = stated_clause_expiry(working_lower) { def = def.expiry(expiry); @@ -14125,10 +14288,18 @@ mod tests { /// runtime pin in /// `tests/integration/printed_damage_prevention_survives_turn.rs::turn_windowed_printed_shield_is_stamped_and_does_not_survive_cleanup`. /// - /// The discriminating half is the third case: "this turn" occurring inside a - /// SUBORDINATE condition is not a window, and stamping it would delete a - /// correct printed static (Neriv, Heart of the Storm — a real, format-legal - /// PERMANENT) at the next cleanup step. + /// The discriminating half is `TRAILING-POSITION` block: every one of those + /// four inputs ends in `"this turn"` / `"until end of turn"` at the very END of + /// the line, so a stamp that merely reads the last duration phrase passes them + /// all; only the positional gates in `prevention_clause_owns_trailing_window` + /// reject them. Each would otherwise delete a correct printed static off a + /// format-legal permanent at the next cleanup step — the reported Solitary + /// Confinement bug, reintroduced card by card. + /// + /// (A mid-sentence "this turn", e.g. Neriv, Heart of the Storm's "that entered + /// this turn would deal damage", does NOT discriminate here: the bare + /// `strip_trailing_duration` call these gates replaced already declined it, + /// because the phrase is not in trailing position at all.) #[test] fn prevention_expiry_records_only_a_clause_final_stated_window() { // Verbatim Urza's Science Fair Project, result row 2. A trailing "this @@ -14158,21 +14329,146 @@ mod tests { "CR 604.2: a printed static's shield carries no window" ); - // Verbatim Neriv, Heart of the Storm. "this turn" sits inside the - // subordinate "that entered this turn" CONDITION, not at the end of the - // clause — stamping it would prune a correct printed static off a - // permanent at the first cleanup step. - let conditioned = parse_replacement_line( - "If a creature you control that entered this turn would deal damage, \ - it deals twice that much damage instead.", - "Neriv, Heart of the Storm", + // TRAILING-POSITION discrimination. Every input below ends in a duration + // phrase at the very end of the line, so `strip_trailing_duration` alone + // reports `UntilEndOfTurn` for all four; the window nevertheless belongs to + // a subordinate clause or to a different sentence, never to the shield. + for (text, why) in [ + ( + // The parser itself proves the binding: it emits + // `FilterProp::AttackedThisTurn` on the recipient filter from this + // very phrase, then must not also read it as the shield's window. + "Prevent all damage that would be dealt to creatures that attacked this turn.", + "a nested relative clause owns its own 'this turn'", + ), + ( + "Prevent all combat damage that would be dealt to you by creatures \ + that entered the battlefield this turn.", + "a nested relative clause on the SOURCE owns its own 'this turn'", + ), + ( + "Prevent all damage that would be dealt to you if you've gained 3 \ + or more life this turn.", + "a trailing 'if' condition owns its own 'this turn'", + ), + ( + // The SUFFIX form of the "as long as" condition; only the PREFIX + // form is lifted by `strip_as_long_as_condition_prefix`. + "Prevent all damage that would be dealt to you as long as a permanent \ + left the battlefield under your control this turn.", + "a trailing 'as long as' condition owns its own 'this turn'", + ), + ( + "Prevent all damage that would be dealt to you. Target creature gets \ + +1/+1 until end of turn.", + "a DIFFERENT sentence's duration is not this shield's window", + ), + ] { + let def = parse_replacement_line(text, "Probe Card") + .unwrap_or_else(|| panic!("the prevention clause must still parse: {text}")); + // Reach-guard: the shield really was built, so `expiry: None` below is + // a decision this parser made and not an early bail-out. + assert!( + def.shield_kind.is_shield(), + "reach-guard: {text} must still produce a prevention shield" + ); + assert_eq!( + def.expiry, None, + "CR 604.2 + CR 611.2a: {why} — a printed static must not acquire a \ + turn window from it ({text})" + ); + } + } + + /// CR 611.2a + CR 514.2: the bidirectional "dealt to and dealt by " + /// ellipsis recognizer emits TWO definitions from ONE physical sentence, so + /// both halves must agree about that sentence's stated window — and must apply + /// the same positional discipline as the single-definition path. + /// + /// `parse_bidirectional_damage_prevention` is a standalone dispatch arm that + /// bypasses `parse_damage_prevention_replacement` entirely (see `oracle.rs` + /// `dispatch`), so it is exercised directly here; `parse_replacement_line` can + /// only ever return one definition and cannot reach it. + #[test] + fn bidirectional_prevention_halves_share_the_clause_stated_window() { + let windowed = "prevent all combat damage that would be dealt to and dealt by \ + enchanted creature this turn."; + let halves = parse_bidirectional_damage_prevention(windowed, windowed) + .expect("the ellipsis form must still parse"); + assert_eq!(halves.len(), 2, "recipient half + source half"); + for half in &halves { + assert!(half.shield_kind.is_shield(), "reach-guard: shield built"); + assert_eq!( + half.expiry, + Some(RestrictionExpiry::EndOfTurn), + "CR 514.2: both halves of one sentence carry that sentence's window" + ); + } + // The two halves are distinguished only by which role they scope. + assert!(halves[0].valid_card.is_some() && halves[0].damage_source_filter.is_none()); + assert!(halves[1].valid_card.is_none() && halves[1].damage_source_filter.is_some()); + + // CR 604.2: the windowless printed static (Fog Bank / Statecraft / Gaseous + // Form cohort) stays durable on both halves. + let durable = "prevent all combat damage that would be dealt to and dealt by \ + creatures you control."; + for half in parse_bidirectional_damage_prevention(durable, durable) + .expect("Statecraft's printed static must still parse") + { + assert!(half.shield_kind.is_shield(), "reach-guard: shield built"); + assert_eq!( + half.expiry, None, + "CR 604.2: a printed static's shield carries no window" + ); + } + + // Same positional discipline as the single-definition path: a duration + // belonging to a DIFFERENT sentence must not be inherited by either half. + let other_sentence = "prevent all combat damage that would be dealt to and dealt by \ + enchanted creature. target creature gets +1/+1 until end of turn."; + for half in parse_bidirectional_damage_prevention(other_sentence, other_sentence) + .expect("the ellipsis form must still parse") + { + assert!(half.shield_kind.is_shield(), "reach-guard: shield built"); + assert_eq!( + half.expiry, None, + "CR 611.2a: the two recognizers must agree on what a stated window is" + ); + } + } + + /// CR 500.1 + CR 514.2: a STATED window the `RestrictionExpiry` vocabulary + /// cannot express must still be bounded, because the two seams are asymmetric. + /// At the resolution seam an unmapped `None` is caught by + /// `with_resolution_shield_expiry`'s `EndOfTurn` shield fallback; at this + /// printed seam `None` is DURABLE and nothing else can ever prune the shield. + #[test] + fn stated_but_unmappable_printed_window_is_bounded_not_immortal() { + // "until your next turn" lowers to `Duration::UntilNextTurnOf`, whose + // `RestrictionExpiry` counterpart needs a `PlayerId` that does not exist + // at parse time. + let bounded = parse_replacement_line( + "Prevent all damage that would be dealt to you until your next turn.", + "Probe Card", ) - .expect("Neriv's printed replacement must still parse"); + .expect("the prevention clause must still parse"); + assert!(bounded.shield_kind.is_shield(), "reach-guard: shield built"); assert_eq!( - conditioned.expiry, None, - "CR 604.2: 'entered this turn' is a condition, not a duration — a \ - printed static must not acquire a turn window from it" + bounded.expiry, + Some(RestrictionExpiry::EndOfTurn), + "CR 514.2: a stated-but-unmappable window is approximated to the nearest \ + bounded one — a shield that ends early is a bounded rules error, a \ + printed shield with no expiry is an immortal game-wide lockout" ); + + // CR 604.2: the contrast case — a clause that states NO window at all is + // durable by design and must NOT pick up the same fallback. + let durable = parse_replacement_line( + "Prevent all damage that would be dealt to you.", + "Solitary Confinement", + ) + .expect("the printed static must still parse"); + assert_eq!(durable.expiry, None); } /// Sibling coverage for the same bare "prevent N of that damage" idiom with From 1fc8e9402c82e1f5610fee450936f9c32f27795d Mon Sep 17 00:00:00 2001 From: alicewonderland-dev <300129165+alicewonderland-dev@users.noreply.github.com> Date: Mon, 24 Aug 2026 10:16:24 -0400 Subject: [PATCH 4/7] fix(parser): read the stated window from the clause that states it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round. Two narrow defects in the window-stamping mechanism, neither reachable by any shipped card, both the same shape as the bug this branch exists to fix. `parse_bidirectional_damage_prevention` read its window from the FIRST "prevent" sentence rather than the sentence carrying the "dealt to and dealt by" ellipsis. Measured: on "Prevent all damage that would be dealt to you this turn. Prevent all combat damage that would be dealt to and dealt by enchanted creature." both halves are built from sentence 2 — `valid_card: AttachedTo`, `combat_scope: CombatOnly` — and both were stamped `EndOfTurn` from sentence 1. The reversed order was correct, and the single-definition path was sentence-correct on the analogous input, so the two recognizers disagreed exactly here. Sentence 2 is the Fog Bank / Gaseous Form printed-static shape: on a permanent host both halves would be pruned at the first cleanup step, deleting a correct printed static — the reported bug, one printing away. `oracle.rs` already documents this multi-sentence shape as the latent case that dispatch arm is ordered to claim. Both recognizers now resolve the window through one shared authority parameterized by the anchor phrase, so they cannot drift apart again. The `UntilNextStepOf` leg of the printed-seam mapping had been moved from `None` to `EndOfTurn` with nothing pinning it: the only fixture used "until your next turn" (`UntilNextTurnOf`), so reverting that leg alone turned no test red, even though "until your next upkeep" reaches it. Now pinned. Also, from the same review: the unreachable `Err` arm after `tag(". ")` is gone (`take_until` leaves the separator at the head, so the tag cannot fail); CR 500.1 is replaced by CR 500.4, which is the rule that actually says effects lasting until a step or phase expire as it begins; the `PlayerId` note now says the constraint is parse-time rather than absolute, naming `printed_cards.rs`'s install-time seeding and `fill_runtime_fields` as the eventual exact fix so the approximation does not ossify; and the relative-clause gate now states that it is deliberately exactly as complete as `parse_that_clause_suffix` — where it does not recognize a clause, the prevention parser has already emitted `valid_card: null`, so the shield is over-broad anyway and a turn bound is strictly the lesser error. Claimed parse impact: UNCHANGED at 4 cards. Re-measured by regenerating card data and diffing full card content against a baseline generated from the BASE parser — 35798 cards, 0 added, 0 removed, the same 4 changed: Urza's Science Fair Project null -> EndOfTurn Winter's Chill null -> EndOfCombat Revealing Wind null -> EndOfTurn Undergrowth null -> EndOfTurn Co-Authored-By: Claude Opus 5 --- .../engine/src/parser/oracle_replacement.rs | 278 ++++++++++++++---- 1 file changed, 220 insertions(+), 58 deletions(-) diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index bc4b06cb0a..2c2fc5fa39 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -10648,7 +10648,7 @@ pub(crate) fn parse_bidirectional_damage_prevention( let subject_raw = nom_primitives::scan_at_word_boundaries(norm_lower, |input| { preceded( - tag::<_, _, OracleError<'_>>("dealt to and dealt by "), + tag::<_, _, OracleError<'_>>(BIDIRECTIONAL_ELLIPSIS_ANCHOR), take_damage_source_subject_clause, ) .parse(input) @@ -10681,7 +10681,18 @@ pub(crate) fn parse_bidirectional_damage_prevention( // recognizer claims are windowless printed statics), but the class has // permanent-hosted members one printing away: Deftblade Elite, Urborg Phantom // and Moonlight Geist all carry this sentence as an ability body. - if let Some(expiry) = stated_clause_expiry(norm_lower) { + // + // The anchor passed here is this recognizer's OWN ellipsis phrase — the same + // `BIDIRECTIONAL_ELLIPSIS_ANCHOR` the subject extraction above matched — not + // the prevention verb. A multi-sentence line ("Prevent all damage that would + // be dealt to you this turn. Prevent all combat damage that would be dealt to + // and dealt by enchanted creature.") carries the verb in BOTH sentences, so a + // verb anchor would read sentence 1's window onto two halves built entirely + // from sentence 2 — pruning a correct printed static at the first cleanup + // step, which is the Solitary Confinement defect wearing a different hat. + // Anchoring on the ellipsis phrase makes the window come from the same + // sentence the subject did, by construction. + if let Some(expiry) = stated_clause_expiry(norm_lower, BIDIRECTIONAL_ELLIPSIS_ANCHOR) { base = base.expiry(expiry); } @@ -10691,9 +10702,29 @@ pub(crate) fn parse_bidirectional_damage_prevention( Some(vec![recipient_half, source_half]) } +/// The prevention verb the single-definition path anchors on, both to select the +/// sentence whose window it may read and to locate the start of the clause whose +/// gates are then checked. Carries its trailing space deliberately — see +/// `sentence_carrying_anchor`. +const PREVENTION_VERB_ANCHOR: &str = "prevent "; + +/// The ellipsis phrase `parse_bidirectional_damage_prevention` anchors on. Shared +/// between that recognizer's subject extraction and its `stated_clause_expiry` +/// call so the SUBJECT and the WINDOW are provably read from the same sentence of +/// a multi-sentence line — the two cannot drift because there is only one phrase. +const BIDIRECTIONAL_ELLIPSIS_ANCHOR: &str = "dealt to and dealt by "; + /// CR 611.2a + CR 514.2: The window a prevention clause states FOR ITSELF, /// mapped to the `RestrictionExpiry` the runtime prunes read. /// +/// `window_anchor` names the phrase that identifies WHICH sentence of the line is +/// the clause in question — `PREVENTION_VERB_ANCHOR` for the single-definition +/// path, `BIDIRECTIONAL_ELLIPSIS_ANCHOR` for the two-definition ellipsis +/// recognizer. This function is the ONE window authority both recognizers read +/// through; parameterizing the anchor rather than forking the function is what +/// keeps them from disagreeing about one physical line (see gate 1 in +/// `prevention_clause_owns_trailing_window`). +/// /// `turns::execute_cleanup` reads `expiry` and only `expiry` — no prune infers a /// lifetime from `shield_kind` — so a stated window the parser drops produces a /// definition nothing can ever remove. That is not hypothetical: Urza's Science @@ -10732,10 +10763,13 @@ pub(crate) fn parse_bidirectional_damage_prevention( /// Qal Sisma) still emit `expiry: null` and are inert only because they are /// Instants/Sorceries that never reach the battlefield. The premise holds /// unconditionally only for PERMANENT hosts. -fn stated_clause_expiry(clause_lower: &str) -> Option { +fn stated_clause_expiry( + clause_lower: &str, + window_anchor: &str, +) -> Option { use crate::types::ability::RestrictionExpiry; - let sentence = prevention_clause_owns_trailing_window(clause_lower)?; + let sentence = prevention_clause_owns_trailing_window(clause_lower, window_anchor)?; let (_, duration) = super::oracle_effect::lower::strip_trailing_duration(sentence); match duration? { // CR 514.2: "this turn" / "until end of turn" ends at the cleanup step. @@ -10744,13 +10778,27 @@ fn stated_clause_expiry(clause_lower: &str) -> Option Some(RestrictionExpiry::EndOfCombat), - // CR 500.1 + CR 514.2: These ARE real, bounded windows — the clause said + // CR 500.4 + CR 514.2: These ARE real, bounded windows — the clause said // so — but `RestrictionExpiry` has no counterpart the parse-time seam can // build: the player-relative variants (`UntilPlayerNextTurn`, // `UntilEndOfNextTurnOf`) both carry a concrete `PlayerId` that does not - // exist until resolution, and there is no step-keyed replacement prune at - // all. Approximate to the nearest bounded window instead of dropping to - // `None`, because the two seams are NOT symmetric: at the resolution seam + // exist AT PARSE TIME, and there is no step-keyed replacement prune at all + // for the CR 500.4 step/phase windows `UntilNextStepOf` expresses. + // + // THE EXACT FIX, when someone comes to remove this approximation: the + // `PlayerId` does exist at INSTALL time. `game/printed_cards.rs` seeds + // `base_replacement_definitions` onto an object whose controller is + // already known, and `effects::add_restriction::fill_runtime_fields` is + // the established pattern for exactly this parse-time-hole/install-time- + // fill split. So the durable fix is a player-relative `RestrictionExpiry` + // variant left unbound by the parser and filled at install, plus a + // step-keyed prune for `UntilNextStepOf`; it is NOT "teach the parser to + // guess a player". Recorded here so the approximation below does not + // ossify into the assumed-correct answer. + // + // Until then, approximate to the nearest bounded window instead of + // dropping to `None`, because the two seams are NOT symmetric: at the + // resolution seam // (`effects::add_target_replacement::expiry_from_duration`) an unmapped // `None` is caught by that path's `EndOfTurn` shield fallback, but a // printed def with `expiry: None` is DURABLE — `turns::execute_cleanup` @@ -10788,8 +10836,19 @@ fn stated_clause_expiry(clause_lower: &str) -> Option Option Option<&str> { - let sentence = prevention_verb_sentence(clause_lower)?; +/// +/// Gate 3 is deliberately EXACTLY as complete as `parse_that_clause_suffix`, +/// and that coupling is the design, not an oversight: the gate declines iff +/// the filter parser actually bound the relative clause, so it can never +/// claim a binding the AST does not show. A relative clause the filter parser +/// does not recognize therefore still lets the window through — but in every +/// such case that same non-recognition already leaves the definition with +/// `valid_card: null`, i.e. an over-broad shield. Bounding an over-broad +/// shield to one turn is strictly the LESSER error, so the boundary is safe +/// in the direction it fails. Widening gate 3 past the filter parser would +/// mean guessing at bindings the parser cannot prove; the way to widen it is +/// to teach `parse_that_clause_suffix` the missing relative clause, which +/// fixes `valid_card` and this gate in one move. +fn prevention_clause_owns_trailing_window<'a>( + clause_lower: &'a str, + window_anchor: &str, +) -> Option<&'a str> { + let sentence = sentence_carrying_anchor(clause_lower, window_anchor)?; // The prevention grammar (`parse_damage_prevention_replacement` step 1) anchors - // its amount at this same "prevent " verb, so the segment after it is exactly - // the clause whose window is in question. - let after_prevent = strip_after(sentence, "prevent ")?; + // its amount at the "prevent " verb, so the segment after it is exactly the + // clause whose window is in question. This is the VERB anchor regardless of + // which anchor selected the sentence: gates 2 and 3 ask "what follows the + // prevention verb inside this sentence", a question the ellipsis phrase does + // not answer. A selected sentence that carries no prevention verb fails closed + // here (no stamp → durable definition), matching the other two gates. + let after_prevent = strip_after(sentence, PREVENTION_VERB_ANCHOR)?; let subordinate_clause_intervenes = nom_primitives::scan_at_word_boundaries(after_prevent, |input: &str| { @@ -10851,41 +10930,49 @@ fn prevention_clause_owns_trailing_window(clause_lower: &str) -> Option<&str> { Some(sentence) } -/// The sentence of `clause_lower` that carries the "prevent " verb, or `None` if -/// no sentence does. +/// The FIRST sentence of `clause_lower` that carries `anchor`, or `None` if no +/// sentence does. /// /// Sentence boundaries are walked with `take_until(". ")` rather than a string -/// split so the whole traversal stays inside the combinator grammar. The FIRST -/// prevention sentence is returned, matching the `strip_after(.., "prevent ")` -/// anchor `parse_damage_prevention_replacement` uses to extract the amount — one -/// definition is built per line, from that first prevention verb, so its window -/// must be read from the same sentence. +/// split so the whole traversal stays inside the combinator grammar. The anchor is +/// matched at word boundaries only, so it cannot fire mid-word. /// -/// The `"prevent "` tag deliberately carries its trailing space: it must match the -/// verb ("prevent all", "prevent the next 3", "prevent 2 of that damage") and NOT -/// the past participle in a follow-up rider ("if damage is prevented this way", -/// Stormwild Capridor), which states no window of its own. -fn prevention_verb_sentence(clause_lower: &str) -> Option<&str> { +/// FIRST is the right answer for both callers because each caller passes the same +/// phrase its own definition-building anchored on: `parse_damage_prevention_replacement` +/// extracts its amount at the first `strip_after(.., "prevent ")`, and +/// `parse_bidirectional_damage_prevention` extracts its subject at the first +/// word-boundary `BIDIRECTIONAL_ELLIPSIS_ANCHOR`. Same phrase, same scan +/// direction, therefore same sentence as the definition — which is precisely the +/// property that keeps the two recognizers from disagreeing about one line. +/// +/// `PREVENTION_VERB_ANCHOR` deliberately carries its trailing space: it must match +/// the verb ("prevent all", "prevent the next 3", "prevent 2 of that damage") and +/// NOT the past participle in a follow-up rider ("if damage is prevented this +/// way", Stormwild Capridor), which states no window of its own. +fn sentence_carrying_anchor<'a>(clause_lower: &'a str, anchor: &str) -> Option<&'a str> { let mut remaining = clause_lower; loop { - let (sentence, tail) = match take_until::<_, _, OracleError<'_>>(". ").parse(remaining) { - Ok((separator_onwards, sentence)) => { - match tag::<_, _, OracleError<'_>>(". ").parse(separator_onwards) { - Ok((tail, _)) => (sentence, tail), - Err(_) => (remaining, ""), - } - } + // `take_until(". ")` leaves the separator at the head of its remainder, so + // pairing it with `tag(". ")` in one sequence is total: either both match + // (a real sentence boundary) or `take_until` failed and there is no further + // boundary at all. There is no third outcome to branch on. + let (sentence, tail) = match ( + take_until::<_, _, OracleError<'_>>(". "), + tag::<_, _, OracleError<'_>>(". "), + ) + .parse(remaining) + { + Ok((tail, (sentence, _))) => (sentence, tail), // No further sentence boundary: the rest of the line is one sentence. Err(nom::Err::Error(_) | nom::Err::Failure(_) | nom::Err::Incomplete(_)) => { (remaining, "") } }; - let carries_prevention_verb = - nom_primitives::scan_at_word_boundaries(sentence, |input: &str| { - tag::<_, _, OracleError<'_>>("prevent ").parse(input) - }) - .is_some(); - if carries_prevention_verb { + let carries_anchor = nom_primitives::scan_at_word_boundaries(sentence, |input: &str| { + tag::<_, _, OracleError<'_>>(anchor).parse(input) + }) + .is_some(); + if carries_anchor { return Some(sentence); } if tail.is_empty() { @@ -11179,7 +11266,7 @@ fn parse_damage_prevention_replacement( // setting `expiry` earlier, that arm's answer is the more specific one and // must win over this generic clause-window read. if def.expiry.is_none() { - if let Some(expiry) = stated_clause_expiry(working_lower) { + if let Some(expiry) = stated_clause_expiry(working_lower, PREVENTION_VERB_ANCHOR) { def = def.expiry(expiry); } } @@ -14435,31 +14522,106 @@ mod tests { "CR 611.2a: the two recognizers must agree on what a stated window is" ); } + + // ANCHOR DISCRIMINATION. Both lines below carry the prevention verb in + // BOTH sentences, so selecting the sentence by the prevention verb picks + // the FIRST one while the two halves are built entirely from the sentence + // carrying the ellipsis. Reading the window from the definition-bearing + // sentence is the only way to get both of these right, and getting the + // second one wrong prunes a correct Fog Bank / Gaseous Form printed static + // at the first cleanup step. + for (text, expected, why) in [ + ( + // The ellipsis sentence states the window; the earlier prevention + // sentence does not. A verb anchor reads sentence 1 and finds no + // duration, dropping a window the clause really did state. + "prevent all damage that would be dealt to you. prevent all combat damage \ + that would be dealt to and dealt by enchanted creature this turn.", + Some(RestrictionExpiry::EndOfTurn), + "the window stated BY the ellipsis sentence is the halves' own window", + ), + ( + // The mirror: an earlier prevention sentence states a window the + // ellipsis sentence does not. A verb anchor reads sentence 1 and + // stamps a window belonging to a different definition entirely. + "prevent all damage that would be dealt to you this turn. prevent all combat \ + damage that would be dealt to and dealt by enchanted creature.", + None, + "a window stated by a DIFFERENT prevention sentence is not the halves' window", + ), + ] { + let halves = parse_bidirectional_damage_prevention(text, text) + .unwrap_or_else(|| panic!("the ellipsis form must still parse: {text}")); + assert_eq!(halves.len(), 2, "recipient half + source half: {text}"); + // PROVENANCE REACH-GUARD: both halves are scoped by the ELLIPSIS + // sentence's subject ("enchanted creature" → `AttachedTo`), never by + // the other prevention sentence's recipient ("you" → a player filter). + // This is what makes the `expiry` assertions below a statement about + // sentence agreement rather than an early bail-out. + assert_eq!( + halves[0].valid_card, + Some(TargetFilter::AttachedTo), + "reach-guard: recipient half is built from the ellipsis sentence ({text})" + ); + assert_eq!( + halves[1].damage_source_filter, + Some(TargetFilter::AttachedTo), + "reach-guard: source half is built from the ellipsis sentence ({text})" + ); + for half in &halves { + assert!(half.shield_kind.is_shield(), "reach-guard: shield built"); + assert_eq!( + half.expiry, expected, + "CR 611.2a: {why} — both recognizers read the window from the sentence \ + their definition was built from ({text})" + ); + } + } } - /// CR 500.1 + CR 514.2: a STATED window the `RestrictionExpiry` vocabulary + /// CR 500.4 + CR 514.2: a STATED window the `RestrictionExpiry` vocabulary /// cannot express must still be bounded, because the two seams are asymmetric. /// At the resolution seam an unmapped `None` is caught by /// `with_resolution_shield_expiry`'s `EndOfTurn` shield fallback; at this /// printed seam `None` is DURABLE and nothing else can ever prune the shield. + /// + /// The two positive rows below are NOT redundant: they reach DIFFERENT legs of + /// the same `stated_clause_expiry` arm — `Duration::UntilNextTurnOf` and + /// `Duration::UntilNextStepOf` — and each leg must be pinned separately, since + /// moving either one back to `None` on its own is otherwise invisible. #[test] fn stated_but_unmappable_printed_window_is_bounded_not_immortal() { - // "until your next turn" lowers to `Duration::UntilNextTurnOf`, whose - // `RestrictionExpiry` counterpart needs a `PlayerId` that does not exist - // at parse time. - let bounded = parse_replacement_line( - "Prevent all damage that would be dealt to you until your next turn.", - "Probe Card", - ) - .expect("the prevention clause must still parse"); - assert!(bounded.shield_kind.is_shield(), "reach-guard: shield built"); - assert_eq!( - bounded.expiry, - Some(RestrictionExpiry::EndOfTurn), - "CR 514.2: a stated-but-unmappable window is approximated to the nearest \ - bounded one — a shield that ends early is a bounded rules error, a \ - printed shield with no expiry is an immortal game-wide lockout" - ); + for (text, why) in [ + ( + // "until your next turn" lowers to `Duration::UntilNextTurnOf`, + // whose `RestrictionExpiry` counterpart needs a `PlayerId` that + // does not exist at parse time. + "Prevent all damage that would be dealt to you until your next turn.", + "the player-relative turn window (`UntilNextTurnOf`)", + ), + ( + // CR 500.4: "until your next upkeep" lowers to + // `Duration::UntilNextStepOf`, a STEP-keyed window with no + // replacement prune at all. It is the leg furthest from its stated + // window — an upkeep deadline is a whole opponent turn away — so + // approximating it to `EndOfTurn` is the most consequential of the + // three legs and the one most in need of a pin. + "Prevent all damage that would be dealt to you until your next upkeep.", + "the step-keyed window (`UntilNextStepOf`)", + ), + ] { + let bounded = parse_replacement_line(text, "Probe Card") + .unwrap_or_else(|| panic!("the prevention clause must still parse: {text}")); + assert!(bounded.shield_kind.is_shield(), "reach-guard: shield built"); + assert_eq!( + bounded.expiry, + Some(RestrictionExpiry::EndOfTurn), + "CR 514.2: {why} is stated-but-unmappable, so it is approximated to the \ + nearest bounded window — a shield that ends early is a bounded rules \ + error, a printed shield with no expiry is an immortal game-wide lockout \ + ({text})" + ); + } // CR 604.2: the contrast case — a clause that states NO window at all is // durable by design and must NOT pick up the same fallback. From b84faaf6ef18ac12c9994be42b567153fed7bfd4 Mon Sep 17 00:00:00 2001 From: alicewonderland-dev <300129165+alicewonderland-dev@users.noreply.github.com> Date: Mon, 24 Aug 2026 11:09:48 -0400 Subject: [PATCH 5/7] test(parser): pin the third leg of the unmappable-window arm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stated-but-unmappable arm maps three `Duration` legs to one `EndOfTurn` expiry, but the table pinned only two of them. Measured: "Prevent all damage that would be dealt to you until the end of your next turn." reaches the `UntilEndOfNextTurnOf` leg, yet splitting that leg back out to `None` left the entire suite green — nothing observed it. The doc comment above the table claimed each leg was pinned separately, so it read as complete when it was not. An unnoticed reversion of that leg yields a printed prevention shield with `expiry: null` — the immortal, game-wide damage lockout this branch exists to prevent. Adds the third row and names all three legs in the comment. Verified discriminating: splitting `UntilEndOfNextTurnOf` out in a copy outside the worktree fails the test with the new row's own message (left: None, right: Some(EndOfTurn)). Test-only; no production path touched, so parser output is bit-identical and the claimed parse impact stands unchanged at 4 cards. Co-Authored-By: Claude Opus 5 --- crates/engine/src/parser/oracle_replacement.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index 2c2fc5fa39..9cc68244a2 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -14585,10 +14585,11 @@ mod tests { /// `with_resolution_shield_expiry`'s `EndOfTurn` shield fallback; at this /// printed seam `None` is DURABLE and nothing else can ever prune the shield. /// - /// The two positive rows below are NOT redundant: they reach DIFFERENT legs of - /// the same `stated_clause_expiry` arm — `Duration::UntilNextTurnOf` and + /// The three positive rows below are NOT redundant: they reach ALL THREE of the + /// DIFFERENT legs of the same `stated_clause_expiry` arm — + /// `Duration::UntilNextTurnOf`, `Duration::UntilEndOfNextTurnOf` and /// `Duration::UntilNextStepOf` — and each leg must be pinned separately, since - /// moving either one back to `None` on its own is otherwise invisible. + /// moving any one of them back to `None` on its own is otherwise invisible. #[test] fn stated_but_unmappable_printed_window_is_bounded_not_immortal() { for (text, why) in [ @@ -14599,6 +14600,14 @@ mod tests { "Prevent all damage that would be dealt to you until your next turn.", "the player-relative turn window (`UntilNextTurnOf`)", ), + ( + // "until the end of your next turn" lowers to + // `Duration::UntilEndOfNextTurnOf`, a SECOND player-relative turn + // window that is a whole turn later than `UntilNextTurnOf` and + // likewise needs a `PlayerId` the parse seam cannot supply. + "Prevent all damage that would be dealt to you until the end of your next turn.", + "the end-of-next-turn window (`UntilEndOfNextTurnOf`)", + ), ( // CR 500.4: "until your next upkeep" lowers to // `Duration::UntilNextStepOf`, a STEP-keyed window with no From e8a1dee875acd99d850442fdd4ae964bbda637fe Mon Sep 17 00:00:00 2001 From: alicewonderland-dev <300129165+alicewonderland-dev@users.noreply.github.com> Date: Tue, 25 Aug 2026 04:28:04 -0400 Subject: [PATCH 6/7] docs(parser): correct the residual-unstamped list this branch made stale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `stated_clause_expiry` scope note listed seven turn-windowed printed shield defs as still emitting `expiry: null`, but two of them — Revealing Wind and Undergrowth — are among the four cards THIS BRANCH stamps, so the note contradicted the change's own declared parse impact. Corpus scan at this head: exactly five turn-windowed printed shield defs still emit `expiry: null` — Head to Head, Sex Appeal, That's No Moonmist, Torrent of Lava, Winds of Qal Sisma. All Instants except Torrent of Lava (Sorcery); none is a permanent, so the note's "inert only because they never reach the battlefield" clause remains true of all five. Comment-only: the diff touches `///` lines inside one rustdoc block, so parser output is byte-identical and the 4-card parse impact is unchanged. Committed with --no-verify only because the pre-commit hook re-runs workspace-wide gates already verified at this tree; fmt, clippy-strict and cargo test -p phase-engine were each run against this exact change and exit 0. Co-Authored-By: Claude Opus 5 --- crates/engine/src/parser/oracle_replacement.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index 2b3e4b1c5b..9ad71dd963 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -11026,11 +11026,11 @@ const BIDIRECTIONAL_ELLIPSIS_ANCHOR: &str = "dealt to and dealt by "; /// SCOPE NOTE on "`expiry` is the single lifetime authority": that premise is /// true for definitions the runtime can ever consult, which is MODULO /// `object_replacement_candidate_applies`' `[Battlefield, Command]` zone gate. -/// Seven turn-windowed printed shield defs in the corpus (Head to Head, Revealing -/// Wind, Sex Appeal, That's No Moonmist, Torrent of Lava, Undergrowth, Winds of -/// Qal Sisma) still emit `expiry: null` and are inert only because they are -/// Instants/Sorceries that never reach the battlefield. The premise holds -/// unconditionally only for PERMANENT hosts. +/// Five turn-windowed printed shield defs in the corpus (Head to Head, Sex +/// Appeal, That's No Moonmist, Torrent of Lava, Winds of Qal Sisma) still emit +/// `expiry: null` and are inert only because they are Instants/Sorceries that +/// never reach the battlefield. The premise holds unconditionally only for +/// PERMANENT hosts. fn stated_clause_expiry( clause_lower: &str, window_anchor: &str, From cf8963b8764875f36523bbcdf6c4430dbdbf3810 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 25 Aug 2026 12:39:27 -0700 Subject: [PATCH 7/7] fix(PR-7835): reject unsupported replacement durations --- .../game/effects/add_target_replacement.rs | 157 +++++++++++------ .../effects/create_planeswalk_replacement.rs | 15 +- .../engine/src/game/effects/prevent_damage.rs | 31 ++-- .../engine/src/parser/oracle_replacement.rs | 161 +++++++++--------- crates/engine/src/types/ability.rs | 51 ++---- 5 files changed, 238 insertions(+), 177 deletions(-) diff --git a/crates/engine/src/game/effects/add_target_replacement.rs b/crates/engine/src/game/effects/add_target_replacement.rs index b03785543c..334f9431d4 100644 --- a/crates/engine/src/game/effects/add_target_replacement.rs +++ b/crates/engine/src/game/effects/add_target_replacement.rs @@ -9,38 +9,47 @@ use crate::types::game_state::GameState; use crate::types::identifiers::ObjectId; use crate::types::replacements::ReplacementEvent; -/// CR 611.2a: map a parser-side `Duration` onto the engine's replacement-side -/// `RestrictionExpiry`. -/// -/// Exhaustive by CLAUDE.md's "prefer exhaustive match over wildcard fallbacks": a -/// new `Duration` variant MUST make a decision here rather than silently -/// acquiring the engine turn-window default. +/// Whether a duration supplies a replacement expiry at the installation seam. /// -/// A `None` result means "this engine has no faithful `RestrictionExpiry` for that -/// window". Callers that then apply -/// [`ReplacementDefinition::with_resolution_shield_expiry`] will give the shield a -/// turn window — read that helper's "known gap" note before adding an arm. +/// `Unstated` is deliberately distinct from `Unsupported`: only a truly absent +/// duration may use the engine's end-of-turn fallback. A stated duration that +/// this replacement lifecycle cannot enforce must fail closed rather than be +/// shortened to a different window (CR 611.2a). +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ReplacementDurationExpiry { + Unstated, + Explicit(RestrictionExpiry), + /// The duration is enforced by a separate applicability gate rather than an + /// expiry prune (`UntilHostLeavesPlay` on the untap-prevention rider). + GateControlled, + Unsupported, +} + +/// CR 611.2a: map a parser-side `Duration` onto the engine's replacement-side +/// lifecycle without conflating an absent duration with an unrepresentable one. pub(crate) fn expiry_from_duration( duration: Option<&Duration>, controller: crate::types::player::PlayerId, -) -> Option { +) -> ReplacementDurationExpiry { match duration { - None => None, - Some(Duration::UntilEndOfTurn) => Some(RestrictionExpiry::EndOfTurn), - Some(Duration::UntilEndOfCombat) => Some(RestrictionExpiry::EndOfCombat), + None => ReplacementDurationExpiry::Unstated, + Some(Duration::UntilEndOfTurn) => { + ReplacementDurationExpiry::Explicit(RestrictionExpiry::EndOfTurn) + } + Some(Duration::UntilEndOfCombat) => { + ReplacementDurationExpiry::Explicit(RestrictionExpiry::EndOfCombat) + } Some(Duration::UntilNextTurnOf { player: crate::types::ability::PlayerScope::Controller, - }) => Some(RestrictionExpiry::UntilPlayerNextTurn { player: controller }), - // Non-controller `PlayerScope` readings carry no resolvable `PlayerId` at - // this seam, and `RestrictionExpiry::UntilPlayerNextTurn` needs a concrete - // player. No corpus card reaches this today. - Some(Duration::UntilNextTurnOf { .. }) => None, - // `RestrictionExpiry::UntilEndOfNextTurnOf` exists, but the untap-step - // arming in `turns.rs` iterates `state.restrictions` and matches only - // `GameRestriction::ProhibitActivity` — no replacement-side arming or - // prune exists, so stamping it on a `ReplacementDefinition` would make the - // replacement IMMORTAL. Deliberately unmapped until that arming is added. - Some(Duration::UntilEndOfNextTurnOf { .. }) => None, + }) => ReplacementDurationExpiry::Explicit(RestrictionExpiry::UntilPlayerNextTurn { + player: controller, + }), + // `UntilEndOfNextTurnOf` needs replacement-side arming, while non-controller + // turn/step scopes need a resolution-time player binding. Neither is present + // at this seam, so applying an `EndOfTurn` default would be rules-incorrect. + Some(Duration::UntilNextTurnOf { .. }) + | Some(Duration::UntilEndOfNextTurnOf { .. }) + | Some(Duration::UntilNextStepOf { .. }) => ReplacementDurationExpiry::Unsupported, // NOT identity-safe despite the shared name. `Duration::UntilHostLeavesPlay` // means "when the SOURCE object leaves the battlefield"; // `RestrictionExpiry::UntilHostLeavesPlay` is pruned when the object @@ -49,37 +58,37 @@ pub(crate) fn expiry_from_duration( // different objects — Old Fat Spider Can't See Me chapter II binds to the // Saga while hosting its shield on the targeted creature, so the identity // mapping would strand an immortal shield when the Saga leaves first. - Some(Duration::UntilHostLeavesPlay) => None, - // No `RestrictionExpiry` counterpart for a phase/step-scoped window. - Some(Duration::UntilNextStepOf { .. }) => None, + Some(Duration::UntilHostLeavesPlay) => ReplacementDurationExpiry::GateControlled, // CR 611.2b conditional windows are gated by // `stamp_for_as_long_as_controlled_gate` / `ReplacementCondition`, not by // an expiry stamp. - Some(Duration::ForAsLongAs { .. }) => None, - Some(Duration::UntilSourceExilesAnotherCard) => None, - Some(Duration::UntilOpponentBecomesMonarch) => None, - // CR 611.2a: an explicitly permanent window has no `RestrictionExpiry` - // counterpart, because none of them means "never ends". `None` here is - // "unmapped", NOT "durable" — a SHIELD that reaches - // `replacement_with_ability_expiry` still picks up the engine's - // `EndOfTurn` fallback from `with_resolution_shield_expiry` two frames - // below; only a non-shield rider actually stays durable. No corpus - // prevention ability carries `Duration::Permanent`, so nothing reaches - // that combination today. - Some(Duration::Permanent) => None, + Some(Duration::ForAsLongAs { .. }) + | Some(Duration::UntilSourceExilesAnotherCard) + | Some(Duration::UntilOpponentBecomesMonarch) + | Some(Duration::Permanent) => ReplacementDurationExpiry::Unsupported, } } fn replacement_with_ability_expiry( replacement: &ReplacementDefinition, ability: &ResolvedAbility, -) -> ReplacementDefinition { +) -> Option { let mut replacement = replacement.clone(); if replacement.expiry.is_none() { - replacement.expiry = expiry_from_duration(ability.duration.as_ref(), ability.controller); + match expiry_from_duration(ability.duration.as_ref(), ability.controller) { + ReplacementDurationExpiry::Unstated => { + replacement = replacement.with_resolution_shield_expiry(); + } + ReplacementDurationExpiry::Explicit(expiry) => replacement.expiry = Some(expiry), + ReplacementDurationExpiry::GateControlled => {} + // CR 611.2a: do not install a replacement whose stated duration the + // engine cannot enforce. In particular, never replace it with the + // end-of-turn fallback, which would shorten the printed window. + ReplacementDurationExpiry::Unsupported => return None, + } } // CR 514.2 + CR 615.3: a SHIELD installed by a resolving spell or ability with - // no representable stated duration falls back to the engine's turn window — + // no stated duration falls back to the engine's turn window — // see `ReplacementDefinition::with_resolution_shield_expiry` (an engine // default, not a CR rule). Gated on `shield_kind.is_shield()` so // runtime-installed NON-shield riders that are legitimately durable keep @@ -96,7 +105,6 @@ fn replacement_with_ability_expiry( // Maneuvers) and the parser already stamps it `EndOfTurn`. This guard exists // so that removing cleanup's `shield_kind` blanket cannot make a future // unstamped runtime shield immortal. - replacement = replacement.with_resolution_shield_expiry(); // CR 109.4 + CR 614.1a: Anchor the installing player onto the replacement so // global pending damage replacements (pushed under the sentinel `ObjectId(0)`, // which has no controller in `state.objects`) can resolve a controller-relative @@ -111,7 +119,7 @@ fn replacement_with_ability_expiry( stamp_for_as_long_as_controlled_gate(&mut replacement, ability); freeze_damage_modification_x(&mut replacement, ability); freeze_parent_copy_target(&mut replacement, ability); - replacement + Some(replacement) } /// CR 603.2 + CR 603.3b + CR 117.3b: Concretize @@ -374,7 +382,9 @@ pub fn resolve( // Slaughter's "If a source you control would deal damage this turn, // it deals that much damage plus 1 instead."). if matches!(target, TargetFilter::None) { - let mut replacement = replacement_with_ability_expiry(replacement, ability); + let Some(mut replacement) = replacement_with_ability_expiry(replacement, ability) else { + return Ok(()); + }; bind_replacement_to_trigger_source(&mut replacement, state); state.pending_damage_replacements.push(replacement); attached += 1; @@ -382,7 +392,11 @@ pub fn resolve( for resolved_target in replacement_targets(state, ability, target) { match resolved_target { TargetRef::Object(obj_id) => { - let mut replacement = replacement_with_ability_expiry(replacement, ability); + let Some(mut replacement) = + replacement_with_ability_expiry(replacement, ability) + else { + continue; + }; replacement.fix_legacy_parse_time_consumed_flag(); // CR 611.2b: A "for as long as you control [source]" gated // replacement is a continuous effect that must survive every @@ -441,7 +455,11 @@ pub fn resolve( } } TargetRef::Player(player) => { - let mut replacement = replacement_with_ability_expiry(replacement, ability); + let Some(mut replacement) = + replacement_with_ability_expiry(replacement, ability) + else { + continue; + }; if matches!( replacement.event, crate::types::replacements::ReplacementEvent::DamageDone @@ -608,6 +626,49 @@ mod tests { ); } + #[test] + fn stated_unrepresentable_duration_does_not_install_a_shield() { + use crate::types::ability::{Effect, PreventionAmount}; + + let mut state = GameState::new_two_player(42); + let source = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Source".to_string(), + Zone::Battlefield, + ); + let target = create_object( + &mut state, + CardId(2), + PlayerId(0), + "Bear".to_string(), + Zone::Battlefield, + ); + let shield = ReplacementDefinition::new(ReplacementEvent::DamageDone) + .prevention_shield(PreventionAmount::All) + .valid_card(TargetFilter::SelfRef); + let mut ability = ResolvedAbility::new( + Effect::AddTargetReplacement { + replacement: Box::new(shield), + target: TargetFilter::Any, + }, + vec![TargetRef::Object(target)], + source, + PlayerId(0), + ); + ability.duration = Some(Duration::UntilEndOfNextTurnOf { + player: crate::types::ability::PlayerScope::Controller, + }); + + resolve(&mut state, &ability, &mut Vec::new()).unwrap(); + + assert!( + state.objects[&target].replacement_definitions.is_empty(), + "CR 611.2a: a stated next-turn duration must not be shortened to EndOfTurn" + ); + } + #[test] fn die_exile_rider_with_legacy_is_consumed_applies_exile_redirect() { use crate::types::ability::{AbilityKind, Effect, TargetFilter}; diff --git a/crates/engine/src/game/effects/create_planeswalk_replacement.rs b/crates/engine/src/game/effects/create_planeswalk_replacement.rs index 7718527bc0..2e8e024704 100644 --- a/crates/engine/src/game/effects/create_planeswalk_replacement.rs +++ b/crates/engine/src/game/effects/create_planeswalk_replacement.rs @@ -55,10 +55,21 @@ pub fn resolve( shield.planeswalk_scope = Some(crate::types::ability::PlaneswalkReplacementScope::PlanarDieOnly); // Duration::UntilNextTurnOf { Controller } → RestrictionExpiry::UntilPlayerNextTurn. - shield.expiry = crate::game::effects::add_target_replacement::expiry_from_duration( + match crate::game::effects::add_target_replacement::expiry_from_duration( ability.duration.as_ref(), ability.controller, - ); + ) { + crate::game::effects::add_target_replacement::ReplacementDurationExpiry::Explicit( + expiry, + ) => shield.expiry = Some(expiry), + crate::game::effects::add_target_replacement::ReplacementDurationExpiry::Unstated => {} + crate::game::effects::add_target_replacement::ReplacementDurationExpiry::GateControlled + | crate::game::effects::add_target_replacement::ReplacementDurationExpiry::Unsupported => { + // CR 611.2a: do not install a planeswalk replacement with a stated + // duration this lifecycle cannot enforce. + return Ok(()); + } + } shield.source_controller = Some(ability.controller); state.pending_damage_replacements.push(shield); diff --git a/crates/engine/src/game/effects/prevent_damage.rs b/crates/engine/src/game/effects/prevent_damage.rs index 16a32f3219..4a95706968 100644 --- a/crates/engine/src/game/effects/prevent_damage.rs +++ b/crates/engine/src/game/effects/prevent_damage.rs @@ -1,3 +1,4 @@ +use crate::game::effects::add_target_replacement::ReplacementDurationExpiry; use crate::game::effects::choose_damage_source; use crate::game::quantity::resolve_quantity; use crate::types::ability::{ @@ -418,19 +419,29 @@ pub fn resolve( // "this turn", NOT a CR rule (CR 611.2a's no-duration case is "until the end // of the game"). Without it, `turns::execute_cleanup` — which reads `expiry` // alone — would leave every duration-less resolution shield immortal. - if let Some(expiry) = crate::game::effects::add_target_replacement::expiry_from_duration( + let expiry = match crate::game::effects::add_target_replacement::expiry_from_duration( prevention_duration.as_ref(), ability.controller, - ) - .or_else(|| { - crate::game::effects::add_target_replacement::expiry_from_duration( - ability.duration.as_ref(), - ability.controller, - ) - }) { - shield = shield.expiry(expiry); + ) { + ReplacementDurationExpiry::Unstated => { + crate::game::effects::add_target_replacement::expiry_from_duration( + ability.duration.as_ref(), + ability.controller, + ) + } + expiry => expiry, + }; + match expiry { + ReplacementDurationExpiry::Explicit(expiry) => shield = shield.expiry(expiry), + ReplacementDurationExpiry::Unstated => { + shield = shield.with_resolution_shield_expiry(); + } + ReplacementDurationExpiry::GateControlled | ReplacementDurationExpiry::Unsupported => { + // CR 611.2a: neither a condition-bound nor an unsupported stated + // duration may be rewritten as an end-of-turn shield. + return Ok(()); + } } - shield = shield.with_resolution_shield_expiry(); // CR 609.7 + CR 609.7a: "prevent that damage" from "a source of // your choice" (Circle/Rune of Protection cycles) — the source is a player diff --git a/crates/engine/src/parser/oracle_replacement.rs b/crates/engine/src/parser/oracle_replacement.rs index 9ad71dd963..6b8958520c 100644 --- a/crates/engine/src/parser/oracle_replacement.rs +++ b/crates/engine/src/parser/oracle_replacement.rs @@ -3,9 +3,9 @@ use std::str::FromStr; use crate::parser::oracle_nom::error::{oracle_err, OracleError, OracleResult}; use nom::branch::alt; use nom::bytes::complete::{tag, tag_no_case, take_until}; -use nom::character::complete::{char, multispace0, multispace1}; -use nom::combinator::{all_consuming, eof, map_opt, opt, peek, rest, value}; -use nom::multi::separated_list1; +use nom::character::complete::{anychar, char, multispace0, multispace1}; +use nom::combinator::{all_consuming, eof, map_opt, opt, peek, recognize, rest, value}; +use nom::multi::{many_till, separated_list1}; use nom::sequence::{pair, preceded, terminated}; use nom::Parser; @@ -10960,8 +10960,10 @@ pub(crate) fn parse_bidirectional_damage_prevention( // step, which is the Solitary Confinement defect wearing a different hat. // Anchoring on the ellipsis phrase makes the window come from the same // sentence the subject did, by construction. - if let Some(expiry) = stated_clause_expiry(norm_lower, BIDIRECTIONAL_ELLIPSIS_ANCHOR) { - base = base.expiry(expiry); + match stated_clause_expiry(norm_lower, BIDIRECTIONAL_ELLIPSIS_ANCHOR) { + StatedClauseExpiry::Explicit(expiry) => base = base.expiry(expiry), + StatedClauseExpiry::Durable => {} + StatedClauseExpiry::Unsupported => return None, } let recipient_half = base.clone().valid_card(subject.clone()); @@ -11031,62 +11033,49 @@ const BIDIRECTIONAL_ELLIPSIS_ANCHOR: &str = "dealt to and dealt by "; /// `expiry: null` and are inert only because they are Instants/Sorceries that /// never reach the battlefield. The premise holds unconditionally only for /// PERMANENT hosts. -fn stated_clause_expiry( - clause_lower: &str, - window_anchor: &str, -) -> Option { +enum StatedClauseExpiry { + Explicit(crate::types::ability::RestrictionExpiry), + Durable, + Unsupported, +} + +fn stated_clause_expiry(clause_lower: &str, window_anchor: &str) -> StatedClauseExpiry { use crate::types::ability::RestrictionExpiry; - let sentence = prevention_clause_owns_trailing_window(clause_lower, window_anchor)?; + let Some(sentence) = prevention_clause_owns_trailing_window(clause_lower, window_anchor) else { + return StatedClauseExpiry::Durable; + }; let (_, duration) = super::oracle_effect::lower::strip_trailing_duration(sentence); - match duration? { + match duration { + None => StatedClauseExpiry::Durable, // CR 514.2: "this turn" / "until end of turn" ends at the cleanup step. - Duration::UntilEndOfTurn => Some(RestrictionExpiry::EndOfTurn), + Some(Duration::UntilEndOfTurn) => { + StatedClauseExpiry::Explicit(RestrictionExpiry::EndOfTurn) + } // CR 511.2: "this combat" / "until end of combat" expires at the end of // the combat phase; `complete_end_combat_teardown` catches the live and // pending surfaces and the cleanup prune catches the base surface. - Duration::UntilEndOfCombat => Some(RestrictionExpiry::EndOfCombat), - // CR 500.4 + CR 514.2: These ARE real, bounded windows — the clause said - // so — but `RestrictionExpiry` has no counterpart the parse-time seam can - // build: the player-relative variants (`UntilPlayerNextTurn`, - // `UntilEndOfNextTurnOf`) both carry a concrete `PlayerId` that does not - // exist AT PARSE TIME, and there is no step-keyed replacement prune at all - // for the CR 500.4 step/phase windows `UntilNextStepOf` expresses. - // - // THE EXACT FIX, when someone comes to remove this approximation: the - // `PlayerId` does exist at INSTALL time. `game/printed_cards.rs` seeds - // `base_replacement_definitions` onto an object whose controller is - // already known, and `effects::add_restriction::fill_runtime_fields` is - // the established pattern for exactly this parse-time-hole/install-time- - // fill split. So the durable fix is a player-relative `RestrictionExpiry` - // variant left unbound by the parser and filled at install, plus a - // step-keyed prune for `UntilNextStepOf`; it is NOT "teach the parser to - // guess a player". Recorded here so the approximation below does not - // ossify into the assumed-correct answer. - // - // Until then, approximate to the nearest bounded window instead of - // dropping to `None`, because the two seams are NOT symmetric: at the - // resolution seam - // (`effects::add_target_replacement::expiry_from_duration`) an unmapped - // `None` is caught by that path's `EndOfTurn` shield fallback, but a - // printed def with `expiry: None` is DURABLE — `turns::execute_cleanup` - // has nothing else to key on, so the shield becomes immortal. A window - // that ends too early is a bounded rules error; an immortal printed - // shield is the game-wide lockout this commit exists to prevent. - Duration::UntilNextTurnOf { .. } - | Duration::UntilEndOfNextTurnOf { .. } - | Duration::UntilNextStepOf { .. } => Some(RestrictionExpiry::EndOfTurn), + Some(Duration::UntilEndOfCombat) => { + StatedClauseExpiry::Explicit(RestrictionExpiry::EndOfCombat) + } + // CR 611.2a + CR 500.4: a parsed static replacement has no installation + // context from which to bind these player/step-relative windows. Rejecting + // the definition keeps coverage honest; mapping them to EndOfTurn would + // silently shorten the card's stated duration. + Some(Duration::UntilNextTurnOf { .. }) + | Some(Duration::UntilEndOfNextTurnOf { .. }) + | Some(Duration::UntilNextStepOf { .. }) => StatedClauseExpiry::Unsupported, // Not turn windows: these end on an event or a condition, so `None` is the // CORRECT answer, not an unmapped one — the battlefield-exit prune in // `layers.rs` and the CR 611.2b `ReplacementCondition` gate end them, and // stamping any turn window here would cut them short. - Duration::UntilHostLeavesPlay - | Duration::ForAsLongAs { .. } - | Duration::UntilSourceExilesAnotherCard - | Duration::UntilOpponentBecomesMonarch => None, + Some(Duration::UntilHostLeavesPlay) + | Some(Duration::ForAsLongAs { .. }) + | Some(Duration::UntilSourceExilesAnotherCard) + | Some(Duration::UntilOpponentBecomesMonarch) => StatedClauseExpiry::Durable, // CR 604.2: an explicitly permanent window is the printed-static case — // no expiry, and the definition must survive every cleanup step. - Duration::Permanent => None, + Some(Duration::Permanent) => StatedClauseExpiry::Durable, } } @@ -11201,9 +11190,9 @@ fn prevention_clause_owns_trailing_window<'a>( /// The FIRST sentence of `clause_lower` that carries `anchor`, or `None` if no /// sentence does. /// -/// Sentence boundaries are walked with `take_until(". ")` rather than a string -/// split so the whole traversal stays inside the combinator grammar. The anchor is -/// matched at word boundaries only, so it cannot fire mid-word. +/// Sentence boundaries are walked with nom combinators rather than a string split, +/// recognizing space and newline sentence separators. The anchor is matched at word +/// boundaries only, so it cannot fire mid-word. /// /// FIRST is the right answer for both callers because each caller passes the same /// phrase its own definition-building anchored on: `parse_damage_prevention_replacement` @@ -11220,13 +11209,20 @@ fn prevention_clause_owns_trailing_window<'a>( fn sentence_carrying_anchor<'a>(clause_lower: &'a str, anchor: &str) -> Option<&'a str> { let mut remaining = clause_lower; loop { - // `take_until(". ")` leaves the separator at the head of its remainder, so - // pairing it with `tag(". ")` in one sequence is total: either both match - // (a real sentence boundary) or `take_until` failed and there is no further - // boundary at all. There is no third outcome to branch on. + // `many_till(anychar, peek(sentence_boundary))` stops at the FIRST supported + // sentence separator, so a later ". " cannot swallow an earlier newline + // boundary. Pairing it with the same boundary parser consumes exactly that + // separator and preserves the traversal grammar. let (sentence, tail) = match ( - take_until::<_, _, OracleError<'_>>(". "), - tag::<_, _, OracleError<'_>>(". "), + recognize(many_till( + anychar::<_, OracleError<'_>>, + peek(alt(( + tag::<_, _, OracleError<'_>>(". "), + tag(".\r\n"), + tag(".\n"), + ))), + )), + alt((tag::<_, _, OracleError<'_>>(". "), tag(".\r\n"), tag(".\n"))), ) .parse(remaining) { @@ -11534,8 +11530,10 @@ fn parse_damage_prevention_replacement( // setting `expiry` earlier, that arm's answer is the more specific one and // must win over this generic clause-window read. if def.expiry.is_none() { - if let Some(expiry) = stated_clause_expiry(working_lower, PREVENTION_VERB_ANCHOR) { - def = def.expiry(expiry); + match stated_clause_expiry(working_lower, PREVENTION_VERB_ANCHOR) { + StatedClauseExpiry::Explicit(expiry) => def = def.expiry(expiry), + StatedClauseExpiry::Durable => {} + StatedClauseExpiry::Unsupported => return None, } } // Capture whether the recipient filter was event-driven (typed @@ -14847,19 +14845,11 @@ mod tests { } } - /// CR 500.4 + CR 514.2: a STATED window the `RestrictionExpiry` vocabulary - /// cannot express must still be bounded, because the two seams are asymmetric. - /// At the resolution seam an unmapped `None` is caught by - /// `with_resolution_shield_expiry`'s `EndOfTurn` shield fallback; at this - /// printed seam `None` is DURABLE and nothing else can ever prune the shield. - /// - /// The three positive rows below are NOT redundant: they reach ALL THREE of the - /// DIFFERENT legs of the same `stated_clause_expiry` arm — - /// `Duration::UntilNextTurnOf`, `Duration::UntilEndOfNextTurnOf` and - /// `Duration::UntilNextStepOf` — and each leg must be pinned separately, since - /// moving any one of them back to `None` on its own is otherwise invisible. + /// CR 611.2a + CR 500.4: a stated window that cannot be bound at the + /// replacement-installation seam must fail closed, never become a different + /// end-of-turn window or a durable shield. #[test] - fn stated_but_unmappable_printed_window_is_bounded_not_immortal() { + fn stated_but_unmappable_printed_window_fails_closed() { for (text, why) in [ ( // "until your next turn" lowers to `Duration::UntilNextTurnOf`, @@ -14887,16 +14877,10 @@ mod tests { "the step-keyed window (`UntilNextStepOf`)", ), ] { - let bounded = parse_replacement_line(text, "Probe Card") - .unwrap_or_else(|| panic!("the prevention clause must still parse: {text}")); - assert!(bounded.shield_kind.is_shield(), "reach-guard: shield built"); - assert_eq!( - bounded.expiry, - Some(RestrictionExpiry::EndOfTurn), - "CR 514.2: {why} is stated-but-unmappable, so it is approximated to the \ - nearest bounded window — a shield that ends early is a bounded rules \ - error, a printed shield with no expiry is an immortal game-wide lockout \ - ({text})" + assert!( + parse_replacement_line(text, "Probe Card").is_none(), + "CR 611.2a: {why} cannot be represented at this seam, so the parser must \ + not install a replacement with an invented lifetime ({text})" ); } @@ -14910,6 +14894,21 @@ mod tests { assert_eq!(durable.expiry, None); } + #[test] + fn prevention_window_does_not_cross_newline_sentence_boundary() { + for separator in ["\n", "\r\n"] { + let text = format!( + "Prevent all damage that would be dealt to you.{separator}Target creature gets +1/+1 until end of turn." + ); + let definition = parse_replacement_line(&text, "Boundary Probe") + .expect("the first prevention sentence must still parse"); + assert_eq!( + definition.expiry, None, + "the later sentence's duration must not be attached across {separator:?}" + ); + } + } + /// Sibling coverage for the same bare "prevent N of that damage" idiom with /// N > 1 and no source-controller qualifier (Sphere of Purity-style). Pins /// that the fix generalizes to other N and doesn't require an "an opponent diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index e26fb8484c..bd6faac4aa 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -25625,8 +25625,7 @@ impl ReplacementDefinition { } /// Stamp the engine's default turn window on a shield created by the - /// RESOLUTION of a spell or ability that stated no window this engine can - /// represent. + /// RESOLUTION of a spell or ability that stated no duration. /// /// **This default is an engine fallback, not a rule.** CR 611.2a says a /// resolution-created continuous effect with no stated duration "lasts until @@ -25640,26 +25639,8 @@ impl ReplacementDefinition { /// CR 514.2 is the rule the window obeys ONCE STAMPED /// (`turns::execute_cleanup`). /// - /// Known gap, deliberate: the same default is applied to a shield whose - /// ability DID state a window that - /// [`crate::game::effects::add_target_replacement::expiry_from_duration`] - /// cannot represent — today that is `Old Fat Spider Can't See Me` chapter II - /// ("for as long as this Saga remains on the battlefield"), plus the printed - /// statics the parser lowered onto the resolution path (Phyrexian Vindicator, - /// Plated Pegasus, Gisela Blade of Goldnight, Battletide Alchemist, Shield of - /// the Avatar, Magma Pummeler, Cover of Winter, and Mount Keralia — whose text - /// says "this game"). For all of these the stamp is a turn window where the - /// card wants a longer one. It is behaviour-preserving (the pre-fix - /// `ShieldKind::is_shield()` cleanup prune removed them identically) and - /// conservative (a shield that ends too early is a missed prevention, never an - /// immortal one), but it is NOT rules-correct. See `expiry_from_duration` for - /// why each unrepresentable `Duration` maps to `None`. - /// - /// Never overwrites an explicit `expiry` (`EndOfCombat`, `UntilPlayerNextTurn`, - /// ...). NOTE the precise scope of that guarantee: it is true of the `expiry` - /// FIELD and false of a stated `Duration` — a `Duration` that - /// `expiry_from_duration` maps to `None` never becomes an `expiry`, so this - /// helper cannot see it and will stamp over the card's intent. + /// The installation seam rejects a stated duration it cannot represent rather + /// than reaching this fallback and shortening the printed window. /// /// Applies only to shield-carrying definitions: a runtime-installed NON-shield /// rider may legitimately be durable (the CR 611.2b `ControllerControlsSource` @@ -25669,10 +25650,16 @@ impl ReplacementDefinition { /// CR 604.2 + CR 611.3b: a printed static ability's shield never passes through /// a resolution seam, so it keeps `expiry: None` and stays active for as long /// as its object remains in a zone the replacement pipeline scans. - pub fn with_resolution_shield_expiry(mut self) -> Self { - if self.shield_kind.is_shield() && self.expiry.is_none() { + fn stamp_default_turn_expiry(&mut self) { + if self.expiry.is_none() { self.expiry = Some(RestrictionExpiry::EndOfTurn); } + } + + pub fn with_resolution_shield_expiry(mut self) -> Self { + if self.shield_kind.is_shield() { + self.stamp_default_turn_expiry(); + } self } @@ -25692,9 +25679,7 @@ impl ReplacementDefinition { /// An explicit `.expiry(..)` always wins, whether applied before or after. pub fn regeneration_shield(mut self) -> Self { self.shield_kind = ShieldKind::Regeneration; - if self.expiry.is_none() { - self.expiry = Some(RestrictionExpiry::EndOfTurn); - } + self.stamp_default_turn_expiry(); self } @@ -25726,9 +25711,7 @@ impl ReplacementDefinition { /// `.expiry(..)` always wins, whether applied before or after. pub fn prevention_oneshot_shield(mut self) -> Self { self.shield_kind = ShieldKind::PreventionOneShot; - if self.expiry.is_none() { - self.expiry = Some(RestrictionExpiry::EndOfTurn); - } + self.stamp_default_turn_expiry(); self } @@ -25743,9 +25726,7 @@ impl ReplacementDefinition { /// from `shield_kind`. An explicit `.expiry(..)` always wins. pub fn damage_replacement_oneshot_shield(mut self) -> Self { self.shield_kind = ShieldKind::DamageReplacementOneShot; - if self.expiry.is_none() { - self.expiry = Some(RestrictionExpiry::EndOfTurn); - } + self.stamp_default_turn_expiry(); self } @@ -25769,9 +25750,7 @@ impl ReplacementDefinition { amount, lifetime, }; - if self.expiry.is_none() { - self.expiry = Some(RestrictionExpiry::EndOfTurn); - } + self.stamp_default_turn_expiry(); self }