From 0ae10ab74363cbe075a57d9341bd557ad46df2b8 Mon Sep 17 00:00:00 2001 From: AlexD-Richardson Date: Sun, 26 Apr 2026 22:04:38 -0500 Subject: [PATCH 1/6] Add Winds of Abandon --- Cargo.lock | 254 +-------- crates/engine/src/game/effects/mod.rs | 538 +++++++++++++++++- crates/engine/src/game/effects/overload.rs | 69 +++ crates/engine/src/parser/oracle_effect/mod.rs | 133 +++++ .../src/parser/oracle_effect/sequence.rs | 62 +- crates/engine/src/types/game_state.rs | 46 ++ 6 files changed, 836 insertions(+), 266 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b31b7ba3c0..283c45dd0b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -23,15 +23,6 @@ dependencies = [ "memchr", ] -[[package]] -name = "android_system_properties" -version = "0.1.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "819e7219dbd41043ac279b19830f2efc897156490d7fd6ea916720117ee66311" -dependencies = [ - "libc", -] - [[package]] name = "anstream" version = "1.0.0" @@ -240,18 +231,6 @@ version = "1.0.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9330f8b2ff13f34540b44e946ef35111825727b38d33286ef986142615121801" -[[package]] -name = "chrono" -version = "0.4.44" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c673075a2e0e5f4a1dde27ce9dee1ea4558c7ffe648f576438a20ca1d2acc4b0" -dependencies = [ - "iana-time-zone", - "num-traits", - "serde", - "windows-link", -] - [[package]] name = "clap" version = "4.6.0" @@ -422,40 +401,6 @@ dependencies = [ "syn", ] -[[package]] -name = "darling" -version = "0.23.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "25ae13da2f202d56bd7f91c25fba009e7717a1e4a1cc98a76d844b65ae912e9d" -dependencies = [ - "darling_core", - "darling_macro", -] - -[[package]] -name = "darling_core" -version = "0.23.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9865a50f7c335f53564bb694ef660825eb8610e0a53d3e11bf1b0d3df31e03b0" -dependencies = [ - "ident_case", - "proc-macro2", - "quote", - "strsim", - "syn", -] - -[[package]] -name = "darling_macro" -version = "0.23.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ac3984ec7bd6cfa798e62b4a642426a5be0e68f9401cfc2a01e3fa9ea2fcdb8d" -dependencies = [ - "darling_core", - "quote", - "syn", -] - [[package]] name = "data-encoding" version = "2.10.0" @@ -469,7 +414,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7cd812cc2bc1d69d4764bd80df88b4317eaef9e773c75226407d9bc0876b211c" dependencies = [ "powerfmt", - "serde_core", ] [[package]] @@ -525,12 +469,6 @@ dependencies = [ "dtoa", ] -[[package]] -name = "dyn-clone" -version = "1.0.20" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d0881ea181b1df73ff77ffaaf9c7544ecc11e82fba9b5f27b262a3c73a332555" - [[package]] name = "ego-tree" version = "0.10.0" @@ -564,7 +502,7 @@ version = "0.1.19" dependencies = [ "assert_matches", "im", - "indexmap 2.13.0", + "indexmap", "insta", "nom", "nom-language", @@ -832,19 +770,13 @@ dependencies = [ "futures-core", "futures-sink", "http", - "indexmap 2.13.0", + "indexmap", "slab", "tokio", "tokio-util", "tracing", ] -[[package]] -name = "hashbrown" -version = "0.12.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8a9ee70c43aaf417c914396645a0fa852624801b24ebb7ae78fe8272889ac888" - [[package]] name = "hashbrown" version = "0.14.5" @@ -884,12 +816,6 @@ version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2304e00983f87ffb38b55b444b5e3b60a884b5d30c0fca7d82fe33449bbe55ea" -[[package]] -name = "hex" -version = "0.4.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" - [[package]] name = "html5ever" version = "0.29.1" @@ -1027,30 +953,6 @@ dependencies = [ "windows-registry", ] -[[package]] -name = "iana-time-zone" -version = "0.1.65" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e31bc9ad994ba00e440a8aa5c9ef0ec67d5cb5e5cb0cc7f8b744a35b389cc470" -dependencies = [ - "android_system_properties", - "core-foundation-sys", - "iana-time-zone-haiku", - "js-sys", - "log", - "wasm-bindgen", - "windows-core", -] - -[[package]] -name = "iana-time-zone-haiku" -version = "0.1.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f31827a206f56af32e590ba56d5d2d085f558508192593743f16b2306495269f" -dependencies = [ - "cc", -] - [[package]] name = "icu_collections" version = "2.1.1" @@ -1138,12 +1040,6 @@ version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3d3067d79b975e8844ca9eb072e16b31c3c1c36928edf9c6789548c524d0d954" -[[package]] -name = "ident_case" -version = "1.0.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b9e0384b61958566e926dc50660321d12159025e767c18e043daf26b70104c39" - [[package]] name = "idna" version = "1.1.0" @@ -1180,17 +1076,6 @@ dependencies = [ "version_check", ] -[[package]] -name = "indexmap" -version = "1.9.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bd070e393353796e801d209ad339e89596eb4c8d430d18ede6a1cced8fafbd99" -dependencies = [ - "autocfg", - "hashbrown 0.12.3", - "serde", -] - [[package]] name = "indexmap" version = "2.13.0" @@ -1379,17 +1264,6 @@ dependencies = [ "windows-sys 0.61.2", ] -[[package]] -name = "mtgish-import" -version = "0.1.19" -dependencies = [ - "anyhow", - "engine", - "serde", - "serde_json", - "serde_with", -] - [[package]] name = "native-tls" version = "0.2.18" @@ -1547,7 +1421,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b4c5cc86750666a3ed20bdaf5ca2a0344f9c67674cae0515bec2da16fbaa47db" dependencies = [ "fixedbitset", - "indexmap 2.13.0", + "indexmap", ] [[package]] @@ -1852,26 +1726,6 @@ dependencies = [ "bitflags", ] -[[package]] -name = "ref-cast" -version = "1.0.25" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f354300ae66f76f1c85c5f84693f0ce81d747e2c3f21a45fef496d89c960bf7d" -dependencies = [ - "ref-cast-impl", -] - -[[package]] -name = "ref-cast-impl" -version = "1.0.25" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b7186006dcb21920990093f30e3dea63b7d6e977bf1256be20c3563a5db070da" -dependencies = [ - "proc-macro2", - "quote", - "syn", -] - [[package]] name = "regex-automata" version = "0.4.14" @@ -2038,30 +1892,6 @@ dependencies = [ "windows-sys 0.61.2", ] -[[package]] -name = "schemars" -version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4cd191f9397d57d581cddd31014772520aa448f65ef991055d7f61582c65165f" -dependencies = [ - "dyn-clone", - "ref-cast", - "serde", - "serde_json", -] - -[[package]] -name = "schemars" -version = "1.2.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a2b42f36aa1cd011945615b92222f6bf73c599a102a300334cd7f8dbeec726cc" -dependencies = [ - "dyn-clone", - "ref-cast", - "serde", - "serde_json", -] - [[package]] name = "scopeguard" version = "1.2.0" @@ -2227,37 +2057,6 @@ dependencies = [ "serde", ] -[[package]] -name = "serde_with" -version = "3.18.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dd5414fad8e6907dbdd5bc441a50ae8d6e26151a03b1de04d89a5576de61d01f" -dependencies = [ - "base64", - "chrono", - "hex", - "indexmap 1.9.3", - "indexmap 2.13.0", - "schemars 0.9.0", - "schemars 1.2.1", - "serde_core", - "serde_json", - "serde_with_macros", - "time", -] - -[[package]] -name = "serde_with_macros" -version = "3.18.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d3db8978e608f1fe7357e211969fd9abdcae80bac1ba7a3369bb7eb6b404eb65" -dependencies = [ - "darling", - "proc-macro2", - "quote", - "syn", -] - [[package]] name = "server-core" version = "0.1.19" @@ -2674,7 +2473,7 @@ version = "0.22.27" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "41fe8c660ae4257887cf66394862d21dbca4a6ddd26f04a3560410406a2f819a" dependencies = [ - "indexmap 2.13.0", + "indexmap", "serde", "serde_spanned", "toml_datetime", @@ -3046,7 +2845,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bb0e353e6a2fbdc176932bbaab493762eb1255a7900fe0fea1a2f96c296cc909" dependencies = [ "anyhow", - "indexmap 2.13.0", + "indexmap", "wasm-encoder", "wasmparser", ] @@ -3059,7 +2858,7 @@ checksum = "47b807c72e1bac69382b3a6fb3dbe8ea4c0ed87ff5629b8685ae6b9a611028fe" dependencies = [ "bitflags", "hashbrown 0.15.5", - "indexmap 2.13.0", + "indexmap", "semver", ] @@ -3083,41 +2882,6 @@ dependencies = [ "wasm-bindgen", ] -[[package]] -name = "windows-core" -version = "0.62.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b8e83a14d34d0623b51dce9581199302a221863196a1dde71a7663a4c2be9deb" -dependencies = [ - "windows-implement", - "windows-interface", - "windows-link", - "windows-result", - "windows-strings", -] - -[[package]] -name = "windows-implement" -version = "0.60.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "053e2e040ab57b9dc951b72c264860db7eb3b0200ba345b4e4c3b14f67855ddf" -dependencies = [ - "proc-macro2", - "quote", - "syn", -] - -[[package]] -name = "windows-interface" -version = "0.59.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3f316c4a2570ba26bbec722032c4099d8c8bc095efccdc15688708623367e358" -dependencies = [ - "proc-macro2", - "quote", - "syn", -] - [[package]] name = "windows-link" version = "0.2.1" @@ -3281,7 +3045,7 @@ checksum = "b7c566e0f4b284dd6561c786d9cb0142da491f46a9fbed79ea69cdad5db17f21" dependencies = [ "anyhow", "heck", - "indexmap 2.13.0", + "indexmap", "prettyplease", "syn", "wasm-metadata", @@ -3312,7 +3076,7 @@ checksum = "9d66ea20e9553b30172b5e831994e35fbde2d165325bec84fc43dbf6f4eb9cb2" dependencies = [ "anyhow", "bitflags", - "indexmap 2.13.0", + "indexmap", "log", "serde", "serde_derive", @@ -3331,7 +3095,7 @@ checksum = "ecc8ac4bc1dc3381b7f59c34f00b67e18f910c2c0f50015669dde7def656a736" dependencies = [ "anyhow", "id-arena", - "indexmap 2.13.0", + "indexmap", "log", "semver", "serde", diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 5c7fa2f2e8..344e06bb6a 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -200,14 +200,83 @@ pub(crate) fn mark_pending_continuation_parent(state: &mut GameState, kind: Effe /// than rolling their own `take + resolve_ability_chain`, so the parent /// event is never silently dropped. pub(crate) fn drain_pending_continuation(state: &mut GameState, events: &mut Vec) { - let Some(cont) = state.pending_continuation.take() else { - return; - }; - let PendingContinuation { chain, parent_kind } = cont; - let source_id = chain.source_id; - let _ = resolve_ability_chain(state, &chain, events, 0); - if let Some(kind) = parent_kind { - events.push(GameEvent::EffectResolved { kind, source_id }); + if let Some(cont) = state.pending_continuation.take() { + let PendingContinuation { chain, parent_kind } = cont; + let source_id = chain.source_id; + let _ = resolve_ability_chain(state, &chain, events, 0); + if let Some(kind) = parent_kind { + events.push(GameEvent::EffectResolved { kind, source_id }); + } + } + // CR 609.3 + CR 109.5: After the per-iteration chain drains, drive any + // remaining `repeat_for` iterations. Each resumed iteration may itself + // pause and re-stash via the loop in `resolve_ability_chain`, producing a + // chain of resumed iterations until the loop completes. + drain_pending_repeat_iteration(state, events); +} + +/// CR 609.3 + CR 109.5: Resume a paused `repeat_for` loop. Each iteration +/// may itself pause (re-stashing into `pending_repeat_iteration`); the outer +/// driver in `drain_pending_continuation` re-enters this on the next choice +/// resolution. If an iteration completes synchronously and a further +/// iteration also completes synchronously, this function drives them all +/// in-line so the loop only pauses again when an inner effect actually +/// transitions to a player-choice state. +fn drain_pending_repeat_iteration(state: &mut GameState, events: &mut Vec) { + while let Some(pending) = state.pending_repeat_iteration.take() { + let crate::types::game_state::PendingRepeatIteration { + ability, + tracked_members, + next_iteration, + total_iterations, + } = pending; + let initial_waiting_for = state.waiting_for.clone(); + let mut iteration = next_iteration; + let mut paused = false; + while iteration < total_iterations { + let mut iter_ability; + let iter_effective: &ResolvedAbility = + if let Some(member) = tracked_members.get(iteration) { + iter_ability = (*ability).clone(); + rebind_first_object_target(&mut iter_ability.targets, *member); + &iter_ability + } else { + &ability + }; + let _ = resolve_effect(state, iter_effective, events); + if state.waiting_for != initial_waiting_for { + let next = iteration + 1; + if next < total_iterations { + state.pending_repeat_iteration = + Some(crate::types::game_state::PendingRepeatIteration { + ability: ability.clone(), + tracked_members: tracked_members.clone(), + next_iteration: next, + total_iterations, + }); + } + paused = true; + break; + } + iteration += 1; + } + if paused { + // Loop paused mid-iteration; the next call to + // `drain_pending_continuation` will resume. + break; + } + // Loop completed without pausing. If a NESTED chain stashed a new + // pending_continuation during the resumed iterations, fall through + // and drain it; that drain will recurse back here for any iteration + // that re-stashed. + if state.pending_continuation.is_some() { + // Defer to the outer drain_pending_continuation by re-invoking it. + // We cannot call ourselves recursively here without risking + // unbounded stack on synchronous-only chains, so loop instead by + // letting the outer driver pick up the new chain. Break out and + // let the caller advance. + break; + } } } @@ -596,6 +665,62 @@ fn effect_uses_implicit_tracked_set_targets(effect: &Effect) -> bool { ) } +/// CR 603.7 + CR 109.5: Returns `true` when the effect resolves an acting +/// subject relative to the parent target — i.e., any effect-target slot +/// reachable via [`effect_target_filter`] contains +/// `TargetFilter::ParentTargetController` or `TargetFilter::ParentTarget`. +/// Used by the `repeat_for: TrackedSetSize` loop to decide whether +/// per-iteration parent rebinding is required. +/// +/// CR 109.5: "you/your" on an object refers to its controller; for an iterated +/// effect that derives the acting subject from the parent target (e.g., "its +/// controller" on Winds of Abandon's per-creature search), each iteration must +/// rebind the parent reference to the i-th tracked-set member so the per-iter +/// subject resolves correctly. +/// +/// Generic: scans whatever target filter the effect exposes via +/// `effect_target_filter`, so any future effect family that carries a +/// parent-target filter (search, draw, life-gain by parent's controller, etc.) +/// participates without code changes here. `Effect::target_filter()` already +/// surfaces `SearchLibrary::target_player`, so iterated-search variants are +/// covered through the same single path. +fn effect_refs_parent_target(effect: &Effect) -> bool { + effect_target_filter(effect).is_some_and(filter_refs_parent_target) +} + +/// Recurse into compound filters so a wrapped `ParentTargetController` is +/// detected wherever it appears (`Or { filters: [..., ParentTargetController, ...] }`). +fn filter_refs_parent_target(filter: &TargetFilter) -> bool { + match filter { + TargetFilter::ParentTargetController | TargetFilter::ParentTarget => true, + TargetFilter::Or { filters } | TargetFilter::And { filters } => { + filters.iter().any(filter_refs_parent_target) + } + TargetFilter::Not { filter } => filter_refs_parent_target(filter), + _ => false, + } +} + +/// CR 603.7 + CR 109.5: Replace the first `TargetRef::Object` in a target +/// slice with the supplied object id. Used by the `repeat_for: TrackedSetSize` +/// per-iteration rebind so the i-th iteration's parent reference (e.g., +/// `ParentTargetController` resolution in `search_library::resolve_library_owner`) +/// binds to the i-th tracked-set member, making "its controller" (CR 109.5) +/// resolve to the i-th object's controller per iteration. +fn rebind_first_object_target( + targets: &mut Vec, + new_id: crate::types::identifiers::ObjectId, +) { + if let Some(slot) = targets + .iter_mut() + .find(|t| matches!(t, TargetRef::Object(_))) + { + *slot = TargetRef::Object(new_id); + } else { + targets.push(TargetRef::Object(new_id)); + } +} + pub(crate) fn resolved_object_filter( ability: &ResolvedAbility, target_filter: &TargetFilter, @@ -1131,14 +1256,81 @@ pub fn resolve_ability_chain( 1 }; + // CR 603.7 + CR 608.2c + CR 109.5: Per-iteration parent-target + // rebinding for tracked-set iterations. When `repeat_for == + // TrackedSetSize` and the effect references the parent target via + // a context-ref filter (e.g., `ParentTargetController`, + // `ParentTarget`), each iteration must bind to a different member + // of the tracked set so the per-iteration acting subject is the + // i-th tracked object's controller (Winds of Abandon, where each + // exiled creature's controller searches their own library). + // + // Without this rebind, every iteration sees `effective.targets[0]` + // — the first exiled creature only — and only that creature's + // controller would search. + let iter_tracked_members: Vec = if matches!( + ability.repeat_for, + Some(crate::types::ability::QuantityExpr::Ref { + qty: crate::types::ability::QuantityRef::TrackedSetSize + }) + ) + && effect_refs_parent_target(&effective.effect) + { + state + .chain_tracked_set_id + .and_then(|id| state.tracked_object_sets.get(&id).cloned()) + .unwrap_or_default() + } else { + Vec::new() + }; + let initial_waiting_for = state.waiting_for.clone(); - for _ in 0..iterations { - let _ = resolve_effect(state, effective, events); - // Break if inner effect entered a player-choice state — avoid - // executing subsequent iterations against state awaiting input. + let mut iteration = 0usize; + while iteration < iterations { + // Snapshot per-iteration ability with parent-target rebinding when applicable. + let mut iter_ability; + let iter_effective: &ResolvedAbility = + if let Some(member) = iter_tracked_members.get(iteration) { + iter_ability = effective.clone(); + rebind_first_object_target(&mut iter_ability.targets, *member); + &iter_ability + } else { + effective + }; + let _ = resolve_effect(state, iter_effective, events); + // CR 609.3 + CR 109.5: When the inner effect enters an + // interactive WaitingFor (e.g. SearchChoice), stash the + // remaining iterations so `drain_pending_continuation` can + // resume the loop after the player choice (and its chained + // sub-ability) complete. Without this, only the first + // iteration would ever fire — the loop would break and the + // remaining iterations would be silently dropped. if state.waiting_for != initial_waiting_for { + let next_iteration = iteration + 1; + if next_iteration < iterations { + // The current iteration's sub_ability chain is wired + // through `pending_continuation` already; we only need + // to remember the loop's outer state. Strip + // `sub_ability` from the stashed effective ability so + // the resume path doesn't double-process it. + let mut resume_ability = effective.clone(); + resume_ability.sub_ability = None; + // Iterations resume the same effect family, not a chain + // of sub-abilities, so `repeat_for` is preserved on the + // stashed copy purely for diagnostic clarity — the + // resume path drives the loop count from + // `total_iterations` directly. + state.pending_repeat_iteration = + Some(crate::types::game_state::PendingRepeatIteration { + ability: Box::new(resume_ability), + tracked_members: iter_tracked_members.clone(), + next_iteration, + total_iterations: iterations, + }); + } break; } + iteration += 1; } } // end shares_quality_failed else } @@ -2931,6 +3123,328 @@ mod tests { ); } + /// CR 603.7 + CR 109.5 + CR 701.23a: Winds of Abandon-shape — per-iteration + /// parent-target rebinding for `repeat_for: TrackedSetSize` over a + /// `ParentTargetController` search. Two creatures controlled by *different* + /// opponents (P1 and P2) are exiled. Without the per-iteration rebind both + /// iterations would prompt the same player; with the rebind, the FIRST + /// iteration must prompt the controller of the FIRST tracked-set member + /// specifically (not just "some opponent"), proving the rebind is the only + /// mechanism that places the per-iteration creature as the parent. + /// + /// Critical: `ability.targets` starts EMPTY. Without the rebind path the + /// SearchLibrary resolver would fall through to `ability.controller` + /// (the caster, P0) rather than to either creature's controller, so the + /// assertion below is reachable only through the rebind. + #[test] + fn repeat_for_rebinds_parent_target_to_tracked_set_member_per_iteration() { + use crate::types::ability::SearchSelectionConstraint; + use crate::types::format::FormatConfig; + + // 3-player game so each tracked-set member can have a distinct + // controller — proves the rebind picks per-iteration, not "any opponent". + let mut state = GameState::new(FormatConfig::standard(), 3, 42); + + let creature_a = create_object( + &mut state, + CardId(50), + PlayerId(1), + "Bear".to_string(), + Zone::Exile, + ); + let creature_b = create_object( + &mut state, + CardId(51), + PlayerId(2), + "Wolf".to_string(), + Zone::Exile, + ); + state.objects.get_mut(&creature_a).unwrap().controller = PlayerId(1); + state.objects.get_mut(&creature_b).unwrap().controller = PlayerId(2); + + // Seed P1's and P2's libraries with basic lands so the search finds + // matching cards in each opponent's library. + for (lib_owner, card_id, name) in [ + (PlayerId(1), CardId(60), "Forest"), + (PlayerId(2), CardId(61), "Plains"), + ] { + let land = create_object( + &mut state, + card_id, + lib_owner, + name.to_string(), + Zone::Library, + ); + let obj = state.objects.get_mut(&land).unwrap(); + obj.card_types.core_types = vec![crate::types::card_type::CoreType::Land]; + obj.card_types + .supertypes + .push(crate::types::card_type::Supertype::Basic); + } + + // Publish a chain-scoped tracked set listing both creatures in order. + let set_id = TrackedSetId(state.next_tracked_set_id); + state.next_tracked_set_id += 1; + state + .tracked_object_sets + .insert(set_id, vec![creature_a, creature_b]); + state.chain_tracked_set_id = Some(set_id); + + // ability.targets is EMPTY: the only way for SearchLibrary's + // ParentTargetController to resolve to any opponent is via the + // per-iteration rebind populating targets[0] with the i-th member. + let mut ability = ResolvedAbility::new( + Effect::SearchLibrary { + filter: TargetFilter::Typed(TypedFilter::land().properties(vec![ + FilterProp::HasSupertype { + value: crate::types::card_type::Supertype::Basic, + }, + ])), + count: QuantityExpr::Fixed { value: 1 }, + reveal: false, + target_player: Some(TargetFilter::ParentTargetController), + up_to: false, + selection_constraint: SearchSelectionConstraint::None, + }, + vec![], + ObjectId(9000), + PlayerId(0), // caster is P0 + ); + ability.repeat_for = Some(QuantityExpr::Ref { + qty: QuantityRef::TrackedSetSize, + }); + + let mut events = Vec::new(); + // Depth=1 simulates being inside a larger chain (Winds of Abandon's + // outer chain publishes the tracked set in its first sub-ability). + // Calling at depth=0 would clear `chain_tracked_set_id` per CR 603.7's + // chain-local reset, defeating the test's setup. + resolve_ability_chain(&mut state, &ability, &mut events, 1).unwrap(); + + // First iteration must prompt P1 — controller of `creature_a`, the + // FIRST tracked-set member. If the rebind didn't run, this would + // resolve via `ability.controller` (P0) — which is not an + // opponent — and the SearchLibrary would never set + // WaitingFor::SearchChoice for P1 specifically. + match &state.waiting_for { + WaitingFor::SearchChoice { player, .. } => { + assert_eq!( + *player, + PlayerId(1), + "first iteration must prompt the controller of the FIRST tracked-set member (P1, not P2 or P0)" + ); + } + other => panic!("expected SearchChoice, got {:?}", other), + } + + // The remaining iteration must be stashed in `pending_repeat_iteration` + // so subsequent SearchChoice resolutions resume the loop. + let pending = state + .pending_repeat_iteration + .as_ref() + .expect("second iteration must be stashed for resumption"); + assert_eq!(pending.next_iteration, 1); + assert_eq!(pending.total_iterations, 2); + assert_eq!(pending.tracked_members, vec![creature_a, creature_b]); + } + + /// CR 609.3 + CR 109.5: End-to-end iteration resumption — overloaded Winds + /// of Abandon shape across two distinct opponent controllers. After the + /// FIRST iteration's SearchChoice is resolved (P1 picks a basic land), the + /// loop must resume and prompt the SECOND opponent (P2) for their own + /// search. Without the `pending_repeat_iteration` infrastructure, only + /// the first iteration would ever fire. + #[test] + fn repeat_for_resumes_iteration_after_search_choice_resolves() { + use crate::game::engine::apply; + use crate::types::ability::SearchSelectionConstraint; + use crate::types::actions::GameAction; + use crate::types::format::FormatConfig; + + let mut state = GameState::new(FormatConfig::standard(), 3, 42); + + let creature_a = create_object( + &mut state, + CardId(50), + PlayerId(1), + "Bear".to_string(), + Zone::Exile, + ); + let creature_b = create_object( + &mut state, + CardId(51), + PlayerId(2), + "Wolf".to_string(), + Zone::Exile, + ); + state.objects.get_mut(&creature_a).unwrap().controller = PlayerId(1); + state.objects.get_mut(&creature_b).unwrap().controller = PlayerId(2); + + // Seed each opponent's library with one basic land. + let p1_forest = create_object( + &mut state, + CardId(60), + PlayerId(1), + "Forest".to_string(), + Zone::Library, + ); + { + let obj = state.objects.get_mut(&p1_forest).unwrap(); + obj.card_types.core_types = vec![crate::types::card_type::CoreType::Land]; + obj.card_types + .supertypes + .push(crate::types::card_type::Supertype::Basic); + } + let p2_plains = create_object( + &mut state, + CardId(61), + PlayerId(2), + "Plains".to_string(), + Zone::Library, + ); + { + let obj = state.objects.get_mut(&p2_plains).unwrap(); + obj.card_types.core_types = vec![crate::types::card_type::CoreType::Land]; + obj.card_types + .supertypes + .push(crate::types::card_type::Supertype::Basic); + } + + let set_id = TrackedSetId(state.next_tracked_set_id); + state.next_tracked_set_id += 1; + state + .tracked_object_sets + .insert(set_id, vec![creature_a, creature_b]); + state.chain_tracked_set_id = Some(set_id); + + let mut ability = ResolvedAbility::new( + Effect::SearchLibrary { + filter: TargetFilter::Typed(TypedFilter::land().properties(vec![ + FilterProp::HasSupertype { + value: crate::types::card_type::Supertype::Basic, + }, + ])), + count: QuantityExpr::Fixed { value: 1 }, + reveal: false, + target_player: Some(TargetFilter::ParentTargetController), + up_to: false, + selection_constraint: SearchSelectionConstraint::None, + }, + vec![], + ObjectId(9000), + PlayerId(0), + ); + ability.repeat_for = Some(QuantityExpr::Ref { + qty: QuantityRef::TrackedSetSize, + }); + + let mut events = Vec::new(); + // Depth=1: simulate being inside Winds of Abandon's outer chain (the + // tracked set is published by the parent sub-ability before this + // iteration loop runs). See sibling test for rationale. + resolve_ability_chain(&mut state, &ability, &mut events, 1).unwrap(); + + // Iteration 0: P1 prompted. + match &state.waiting_for { + WaitingFor::SearchChoice { player, .. } => assert_eq!(*player, PlayerId(1)), + other => panic!("expected SearchChoice for P1, got {:?}", other), + } + + // P1 picks the Forest. After resolving, the loop must resume and + // prompt P2 for the second iteration. + apply( + &mut state, + PlayerId(1), + GameAction::SelectCards { + cards: vec![p1_forest], + }, + ) + .unwrap(); + + match &state.waiting_for { + WaitingFor::SearchChoice { player, .. } => assert_eq!( + *player, + PlayerId(2), + "second iteration must prompt the controller of the SECOND tracked-set member (P2). \ + Without iteration resumption, only the first iteration would ever fire." + ), + other => panic!( + "expected SearchChoice for P2 after P1 resolves, got {:?}. \ + This indicates the repeat_for loop did not resume.", + other + ), + } + + // P2 picks the Plains; the loop should now complete with no further + // pending iteration. + apply( + &mut state, + PlayerId(2), + GameAction::SelectCards { + cards: vec![p2_plains], + }, + ) + .unwrap(); + + assert!( + state.pending_repeat_iteration.is_none(), + "loop must clear pending_repeat_iteration after final iteration completes" + ); + } + + /// CR 603.7 + CR 608.2c: Regression — when `repeat_for` is set but the + /// effect does NOT use a parent-target reference (e.g. plain Draw), the + /// per-iteration rebind logic must NOT touch `ability.targets`. Guards + /// against the new rebind path leaking into unrelated `repeat_for` + /// callers. + #[test] + fn repeat_for_does_not_rebind_when_effect_lacks_parent_ref() { + let mut state = GameState::new_two_player(42); + for i in 0..5 { + create_object( + &mut state, + CardId(i + 10), + PlayerId(0), + format!("Card {}", i), + Zone::Library, + ); + } + + // Publish a tracked set with a pretend object so the rebind path could + // misfire if the gate were too loose. + let dummy = create_object( + &mut state, + CardId(99), + PlayerId(1), + "Dummy".to_string(), + Zone::Battlefield, + ); + let set_id = TrackedSetId(state.next_tracked_set_id); + state.next_tracked_set_id += 1; + state.tracked_object_sets.insert(set_id, vec![dummy]); + state.chain_tracked_set_id = Some(set_id); + + let mut ability = ResolvedAbility::new( + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + vec![], + ObjectId(100), + PlayerId(0), + ); + ability.repeat_for = Some(QuantityExpr::Ref { + qty: QuantityRef::TrackedSetSize, + }); + + let mut events = Vec::new(); + resolve_ability_chain(&mut state, &ability, &mut events, 0).unwrap(); + + // 1 tracked-set member → 1 iteration → 1 card drawn. Targets remain empty + // (no spurious Object rebind). + assert_eq!(state.players[0].hand.len(), 1); + } + #[test] fn resolve_ability_chain_player_scope_opponent_discard() { let mut state = GameState::new_two_player(42); diff --git a/crates/engine/src/game/effects/overload.rs b/crates/engine/src/game/effects/overload.rs index 2eecd1e065..cb3bd2d7e3 100644 --- a/crates/engine/src/game/effects/overload.rs +++ b/crates/engine/src/game/effects/overload.rs @@ -20,6 +20,12 @@ //! - `Bounce { target, destination }` → `BounceAll { target, destination }` //! (the canonical mass-bounce variant; mirrors `Destroy` → `DestroyAll` //! and `Pump` → `PumpAll` in shape — see CR 400.7 + CR 611.2c). +//! - `ChangeZone { destination, target, ... }` → `ChangeZoneAll { origin, destination, target }` +//! (Winds of Abandon: "Exile target creature you don't control" → exile +//! each. The single-target flags `enter_tapped`/`enter_transformed`/ +//! `under_your_control`/`enters_attacking`/`up_to`/`enter_with_counters` +//! are dropped — `ChangeZoneAll` does not carry them and the overload +//! corpus exiles to a hidden zone where they have no semantics.) //! //! Effects with no all-matching counterpart (e.g. `Counter` — Counterflux) //! are preserved unchanged; the overloaded cast simply has no useful effect @@ -93,6 +99,24 @@ fn transform_effect_in_place(effect: &mut Effect) { target, destination, }, + // CR 702.96b + CR 701.13a: Winds of Abandon overload — promote the + // single-target `ChangeZone` to its mass counterpart so "exile target + // creature you don't control" becomes "exile each creature you don't + // control". Single-target-only flags (enter_tapped, enter_transformed, + // under_your_control, enters_attacking, up_to, enter_with_counters) + // are dropped — `ChangeZoneAll` carries no equivalents and the + // overload corpus uses this only for hidden-zone exile where these + // modifiers have no semantics. + Effect::ChangeZone { + origin, + destination, + target, + .. + } => Effect::ChangeZoneAll { + origin, + destination, + target, + }, // Effects without an all-matching counterpart (e.g. `Counter` for // Counterflux) are preserved as-is. No overload corpus card has a // meaningful transformation for these today. @@ -204,6 +228,51 @@ mod tests { } } + /// CR 702.96b + CR 701.13a: Winds of Abandon overload — single-target + /// `ChangeZone(exile target opponent's creature)` must promote to + /// `ChangeZoneAll(exile each creature you don't control)`. The filter + /// (controller=Opponent) survives unchanged so the mass exile only hits + /// opponents' creatures, never the caster's own. + #[test] + fn change_zone_becomes_change_zone_all() { + use crate::types::ability::ControllerRef; + let mut def = leaf(Effect::ChangeZone { + origin: None, + destination: Zone::Exile, + target: TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + controller: Some(ControllerRef::Opponent), + properties: vec![], + }), + owner_library: false, + enter_transformed: false, + under_your_control: false, + enter_tapped: false, + enters_attacking: false, + up_to: false, + enter_with_counters: vec![], + }); + transform_ability_def(&mut def); + match *def.effect { + Effect::ChangeZoneAll { + origin, + destination, + ref target, + } => { + assert!(origin.is_none()); + assert_eq!(destination, Zone::Exile); + match target { + TargetFilter::Typed(tf) => { + assert_eq!(tf.controller, Some(ControllerRef::Opponent)); + assert!(tf.type_filters.contains(&TypeFilter::Creature)); + } + other => panic!("expected typed creature filter, got {other:?}"), + } + } + ref other => panic!("expected ChangeZoneAll, got {other:?}"), + } + } + #[test] fn counter_preserved_unchanged() { let mut def = leaf(Effect::Counter { diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index e2b838c65a..0d1bbecf15 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -9046,6 +9046,25 @@ pub(crate) fn normalize_verb_token(token: &str) -> String { _ if token.ends_with("ies") && token.len() > 3 => { format!("{}y", &token[..token.len() - 3]) } + // English orthographic rule (parser-internal, no game-rule annotation): + // verbs ending in /ʃ/, /tʃ/, /s/, /z/, /ks/ take "-es" instead of "-s" + // for third-person singular (search→searches, wash→washes, + // watch→watches, fix→fixes, buzz→buzzes). Strip "es" only when the + // resulting stem ends in `ch`/`sh`/`x`/`z` or is already an `-ss` + // stem (kiss→kisses). Without this branch, "searches" normalizes to + // "searche", which fails the PREDICATE_VERBS lookup and routes "its + // controller searches their library" to the Unimplemented fallback. + // allow-noncombinator: verb-morphology suffix check on pre-tokenized word + _ if token.ends_with("es") && token.len() > 2 && { + let stem = &token[..token.len() - 2]; + // allow-noncombinator: structural suffix check on pre-tokenized stem + ["ch", "sh", "ss"].iter().any(|s| stem.ends_with(s)) + || stem.ends_with('x') + || stem.ends_with('z') + } => + { + token[..token.len() - 2].to_string() + } // allow-noncombinator: verb-morphology suffix check on pre-tokenized word _ if token.ends_with('s') && !token.ends_with("ss") => token[..token.len() - 1].to_string(), _ => token.to_string(), @@ -11403,6 +11422,120 @@ mod tests { ); } + /// CR 608.2c + CR 117.3a + CR 701.23a + CR 609.3 + CR 603.7: Winds of + /// Abandon — iterated subject-anchored search. The structure mirrors + /// Assassin's Trophy but the search step carries `repeat_for: + /// TrackedSetSize` so each exiled creature's controller searches their own + /// library. The tracked-set anchor is published by the leading exile step; + /// the per-iteration parent-target rebind in `repeat_for` execution + /// (game/effects/mod.rs) ensures `ParentTargetController` resolves to a + /// different exiled creature per iteration. + #[test] + fn winds_of_abandon_iterated_subject_search_chain() { + use crate::types::ability::AbilityKind; + let def = parse_effect_chain( + "Exile target creature you don't control. For each creature exiled this way, its controller searches their library for a basic land card. Those players put those cards onto the battlefield tapped, then shuffle.", + AbilityKind::Spell, + ); + // Top-level: ChangeZone(exile target opponent's creature) + match &*def.effect { + Effect::ChangeZone { + destination, + target, + .. + } => { + assert_eq!(*destination, Zone::Exile); + assert!( + matches!(target, TargetFilter::Typed(_)), + "exile target should be a typed filter, got {:?}", + target + ); + } + other => panic!("expected top-level ChangeZone(Exile), got {:?}", other), + } + // Sub: SearchLibrary { target_player: ParentTargetController, basic land, + // repeat_for: TrackedSetSize } + let search = def + .sub_ability + .as_deref() + .expect("chain must continue into SearchLibrary"); + match &*search.effect { + Effect::SearchLibrary { + filter, + target_player, + .. + } => { + assert_eq!( + target_player.as_ref(), + Some(&TargetFilter::ParentTargetController), + "search must route to the parent target's controller" + ); + if let TargetFilter::Typed(tf) = filter { + assert!(tf.type_filters.contains(&TypeFilter::Land)); + assert!( + tf.properties.iter().any(|p| matches!( + p, + FilterProp::HasSupertype { + value: crate::types::card_type::Supertype::Basic + } + )), + "search filter must require Basic supertype" + ); + } else { + panic!("search filter must be Typed, got {:?}", filter); + } + } + other => panic!("expected SearchLibrary, got {:?}", other), + } + assert_eq!( + search.repeat_for, + Some(QuantityExpr::Ref { + qty: QuantityRef::TrackedSetSize + }), + "SearchLibrary must iterate over the tracked-set size" + ); + // Sub-sub: ChangeZone(Library→Battlefield, enter_tapped=true) + let put = search + .sub_ability + .as_deref() + .expect("search must chain a put-step"); + match &*put.effect { + Effect::ChangeZone { + origin, + destination, + enter_tapped, + .. + } => { + assert_eq!(*origin, Some(Zone::Library)); + assert_eq!(*destination, Zone::Battlefield); + assert!( + *enter_tapped, + "put-step must set enter_tapped (Oracle: 'onto the battlefield tapped')" + ); + } + other => panic!("expected ChangeZone Library→Battlefield, got {:?}", other), + } + // Sub-sub-sub: Shuffle with ParentTargetController so each searching + // player shuffles their own library (CR 701.23i). + let shuffle = put.sub_ability.as_deref().expect("must chain Shuffle"); + match &*shuffle.effect { + Effect::Shuffle { target } => { + assert_eq!( + *target, + TargetFilter::ParentTargetController, + "Shuffle must target the searching player, not the caster" + ); + } + other => panic!("expected Shuffle, got {:?}", other), + } + // No spurious tail — the third Oracle clause must have been absorbed. + assert!( + shuffle.sub_ability.is_none(), + "chain must end at Shuffle; got extra: {:?}", + shuffle.sub_ability + ); + } + #[test] fn parse_search_basic_land_to_hand() { let e = parse_effect( diff --git a/crates/engine/src/parser/oracle_effect/sequence.rs b/crates/engine/src/parser/oracle_effect/sequence.rs index af1e97db70..90ff4f3532 100644 --- a/crates/engine/src/parser/oracle_effect/sequence.rs +++ b/crates/engine/src/parser/oracle_effect/sequence.rs @@ -15,6 +15,36 @@ use crate::types::ability::{ }; use crate::types::zones::Zone; +/// CR 608.2c + CR 701.23i: Strip a leading player-subject from a search-result +/// continuation chunk so the absorption matcher sees the bare verb form. Used +/// by the SearchDestination follow-up absorber to handle iterated-search +/// variants (Winds of Abandon: "those players put those cards onto the +/// battlefield tapped") whose subject was demoted from a top-level subject +/// because the put-step has already been folded into the search continuation. +/// +/// Single nom `alt()` over the player-subject prefixes — extend by adding new +/// arms here, never by adding more enumerated `matches!` arms downstream. +/// +/// Intentionally does NOT delegate to `subject::parse_subject_application`: +/// that function is a full subject parser that returns a `SubjectApplication` +/// (filter + targeting + multi-target spec) for use at clause boundaries. +/// Here we only need to peel a known set of player-pronoun prefixes from a +/// continuation chunk before re-tokenizing — there is no filter to derive, +/// no target to attach, and no multi-target structure. The simpler local form +/// keeps the search-continuation absorber decoupled from the subject parser's +/// richer return type and avoids constructing/then-discarding a +/// `SubjectApplication` on the hot continuation path. +fn strip_search_result_subject(lower: &str) -> &str { + alt(( + tag::<_, _, VerboseError<&str>>("those players "), + tag("that player "), + tag("each player "), + )) + .parse(lower) + .map(|(rest, _)| rest) + .unwrap_or(lower) +} + /// Parse count from "choose one/two/three/N of them/those" text using nom combinator. /// Handles all chooser prefix forms: "choose ", "you choose ", "an opponent chooses ", /// "target opponent chooses ". @@ -1411,19 +1441,33 @@ pub(super) fn parse_followup_continuation_ast( // (e.g., Assassin's Trophy / Ranging Raptors / Harrow compound), the // explicit "put it onto the battlefield" chunk in the same sentence is // a paraphrase and must be absorbed to avoid a duplicate ChangeZone. + // + // CR 701.23i + CR 609.3: Iterated-search variants (Winds of Abandon class) + // surface a plural subject ("those players put those cards onto the + // battlefield tapped") because the search step has `repeat_for: + // TrackedSetSize`. The compound has already been folded by the + // SearchDestination intrinsic continuation; the standalone restatement + // here would duplicate the ChangeZone if not absorbed. Use a structural + // prefix-strip on the player-subject so all (subject × pronoun × tapped) + // permutations match without N! enumerated arms. Effect::ChangeZone { origin: Some(Zone::Library), destination: Zone::Battlefield, .. - } if matches!( - lower.trim().trim_end_matches('.'), - "put that card onto the battlefield" - | "put it onto the battlefield" - | "put them onto the battlefield" - | "put those cards onto the battlefield" - | "put that card onto the battlefield tapped" - | "put it onto the battlefield tapped" - ) => + } if { + let bare = strip_search_result_subject(lower.trim().trim_end_matches('.')); + matches!( + bare, + "put that card onto the battlefield" + | "put it onto the battlefield" + | "put them onto the battlefield" + | "put those cards onto the battlefield" + | "put that card onto the battlefield tapped" + | "put it onto the battlefield tapped" + | "put them onto the battlefield tapped" + | "put those cards onto the battlefield tapped" + ) + } => { Some(ContinuationAst::SearchResultClauseHandled) } diff --git a/crates/engine/src/types/game_state.rs b/crates/engine/src/types/game_state.rs index b14772a5c7..5ce2ccd648 100644 --- a/crates/engine/src/types/game_state.rs +++ b/crates/engine/src/types/game_state.rs @@ -426,6 +426,41 @@ impl PendingContinuation { } } +/// CR 609.3 + CR 109.5: Resume state for a `repeat_for` iteration loop paused +/// when the inner effect entered an interactive `WaitingFor` state. +/// +/// When `resolve_ability_chain` is executing the iteration loop for a +/// `repeat_for` quantity (e.g., Winds of Abandon overloaded, where each +/// exiled creature's controller searches their library), the inner effect can +/// transition to `WaitingFor::SearchChoice` (or any other player-choice +/// state). Without resumption, only the first iteration would ever run — the +/// loop breaks at the first paused iteration and the remaining iterations are +/// silently dropped. +/// +/// This struct stashes everything needed to re-enter the loop after the +/// current iteration's player choice (and any chained sub-ability) drains: +/// - `ability` — the effective per-iteration ability (parent of the loop's +/// `effect`); cloned with `sub_ability = None` because the sub-ability is +/// already wired through `pending_continuation` for the current iteration. +/// - `tracked_members` — the tracked-set members snapshotted at loop entry +/// (used by `effect_refs_parent_target` rebinding). Empty when no rebind +/// is required. +/// - `next_iteration` — index of the iteration that should run next when the +/// resume fires. +/// - `total_iterations` — original loop bound, used to detect completion. +/// +/// Drained by `drain_pending_continuation` after the per-iteration +/// `pending_continuation` chain fully drains. Each resumed iteration may +/// itself pause and re-stash this struct (recursive drive). +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct PendingRepeatIteration { + pub ability: Box, + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub tracked_members: Vec, + pub next_iteration: usize, + pub total_iterations: usize, +} + /// CR 603.7: A delayed triggered ability created during resolution of a spell or ability. /// Fires once at the specified condition, then is removed. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -2470,6 +2505,15 @@ pub struct GameState { #[serde(default, skip_serializing_if = "Option::is_none")] pub pending_continuation: Option, + /// CR 609.3 + CR 109.5: Pending `repeat_for` iteration loop paused mid-flight + /// because the inner effect entered an interactive `WaitingFor` state. + /// Drained by `drain_pending_continuation` AFTER `pending_continuation`, + /// so the per-iteration chain (e.g., the SearchLibrary's + /// "put-onto-battlefield" continuation) completes before the next + /// iteration begins. See [`PendingRepeatIteration`]. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub pending_repeat_iteration: Option, + /// Pending optional effect ability chain, awaiting player accept/decline. #[serde(default, skip_serializing_if = "Option::is_none")] pub pending_optional_effect: Option>, @@ -2804,6 +2848,7 @@ impl GameState { modal_modes_chosen_this_game: HashSet::new(), revealed_cards: HashSet::new(), pending_continuation: None, + pending_repeat_iteration: None, pending_optional_effect: None, last_named_choice: None, all_creature_types: Vec::new(), @@ -2972,6 +3017,7 @@ impl PartialEq for GameState { && self.modal_modes_chosen_this_turn == other.modal_modes_chosen_this_turn && self.modal_modes_chosen_this_game == other.modal_modes_chosen_this_game && self.pending_continuation == other.pending_continuation + && self.pending_repeat_iteration == other.pending_repeat_iteration && self.pending_cast == other.pending_cast && self.last_named_choice == other.last_named_choice && self.last_revealed_ids == other.last_revealed_ids From a529602ed45405e3241bab2532af470e161aee2c Mon Sep 17 00:00:00 2001 From: Matt Evans <1388610+matthewevans@users.noreply.github.com> Date: Sun, 26 Apr 2026 21:14:46 -0700 Subject: [PATCH 2/6] fix(overload): spell out ChangeZone field drops explicitly (no `..` rest) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The overload `Effect::ChangeZone → ChangeZoneAll` arm previously used `..` to absorb every field other than `origin`/`destination`/`target`. Today's dropped fields are semantically inert for hidden-zone exile, but `..` would silently absorb any newly-added `ChangeZone` field too, meaning a future field addition could go missing in the overloaded form without compiler help. Bind every field by name and annotate the rationale for each drop. Adding a new `ChangeZone` field will now fail to compile here, forcing a deliberate decision about overload semantics. PR #124 review feedback (finding #3). --- crates/engine/src/game/effects/overload.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/crates/engine/src/game/effects/overload.rs b/crates/engine/src/game/effects/overload.rs index cb3bd2d7e3..6a189aca1a 100644 --- a/crates/engine/src/game/effects/overload.rs +++ b/crates/engine/src/game/effects/overload.rs @@ -111,7 +111,18 @@ fn transform_effect_in_place(effect: &mut Effect) { origin, destination, target, - .. + // Single-target-only modifiers — `ChangeZoneAll` carries no + // equivalents and the overload corpus uses ChangeZone only for + // hidden-zone exile where these have no semantics. Bind each + // field by name (no `..`) so any new `ChangeZone` field added + // upstream forces a deliberate decision here. + owner_library: _, // dropped: ChangeZoneAll always uses target's library scope + enter_transformed: _, // dropped: hidden-zone exile, no battlefield-side effect + under_your_control: _, // dropped: hidden-zone exile, no controller swap + enter_tapped: _, // dropped: hidden-zone exile, tap state irrelevant + enters_attacking: _, // dropped: hidden-zone exile, combat irrelevant + up_to: _, // dropped: ChangeZoneAll has no count semantics + enter_with_counters: _, // dropped: hidden-zone exile, no counters } => Effect::ChangeZoneAll { origin, destination, From 64d2f7bf10610568ab19cca7e70f11e4739b76c8 Mon Sep 17 00:00:00 2001 From: Matt Evans <1388610+matthewevans@users.noreply.github.com> Date: Sun, 26 Apr 2026 21:15:06 -0700 Subject: [PATCH 3/6] fix(engine): preserve sub_ability on resumed repeat_for iterations + handle synchronous-continuation drain MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two interlocking bugs in the `repeat_for` resume path surfaced during PR #124 review of overloaded Winds of Abandon: 1. Iteration-resume path dropped sub_ability for iterations 2+. The stash site cleared `resume_ability.sub_ability = None` and the drain called `resolve_effect` directly, bypassing the chain-level wiring at line 1461 that stashes the SearchChoice continuation. For iteration 0, the put-onto-battlefield + shuffle continuation runs correctly; for iterations 1+, it never ran at all — opponents 2+ would be prompted to pick a card but their chosen card stayed in their library. Fix: keep `sub_ability` on the resumed copy and clear `repeat_for` instead (so the resume call doesn't re-enter the outer iteration loop). The drain now calls `resolve_ability_chain` (depth=1, to preserve chain-local tracked-set state) so each resumed iteration goes through the same line-1660 SearchChoice continuation wiring as iteration 0. Per-iteration parent-target rebinding still propagates correctly because `rebind_first_object_target` updates `iter_ability.targets`, which the line-1651 sub_ability propagation copies onto the continuation chain. 2. Synchronous-continuation case was unhandled. If an iteration set `pending_continuation` without changing `waiting_for`, the inner loop would increment and run the next iteration — clobbering the continuation — or, on the trailing iteration, fall through and break without re-stashing remaining iterations. Fix: detect a None→Some `pending_continuation` transition and re-stash with `next_iteration = iteration + 1` before breaking, so the outer drain runs the continuation and then re-enters this drain for the next iteration. Tests: - `repeat_for_resumed_iteration_runs_full_sub_ability_chain`: end-to-end across two distinct opponents, asserts both chosen lands land on the battlefield AND both Shuffle resolutions emit EffectResolved events. Would have caught finding #1 directly. - `drain_pending_repeat_iteration_restashes_on_synchronous_continuation`: exercises the synchronous-continuation case with a multi-iteration resume that completes without entering any choice state. PR #124 review feedback (findings #1, #2, #5 first/second tests). --- crates/engine/src/game/effects/mod.rs | 382 ++++++++++++++++++++++++-- 1 file changed, 357 insertions(+), 25 deletions(-) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 344e06bb6a..a3454833b1 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -231,6 +231,7 @@ fn drain_pending_repeat_iteration(state: &mut GameState, events: &mut Vec ChangeZone (Library -> Battlefield, enter_tapped=true) + // -> Shuffle (target=ParentTargetController) + let shuffle = ResolvedAbility::new( + Effect::Shuffle { + target: TargetFilter::ParentTargetController, + }, + vec![], + ObjectId(9000), + PlayerId(0), + ); + let put = ResolvedAbility::new( + Effect::ChangeZone { + origin: Some(Zone::Library), + destination: Zone::Battlefield, + target: TargetFilter::Any, + owner_library: false, + enter_transformed: false, + under_your_control: false, + enter_tapped: true, + enters_attacking: false, + up_to: false, + enter_with_counters: vec![], + }, + vec![], + ObjectId(9000), + PlayerId(0), + ) + .sub_ability(shuffle); + let mut search = ResolvedAbility::new( + Effect::SearchLibrary { + filter: TargetFilter::Typed(TypedFilter::land().properties(vec![ + FilterProp::HasSupertype { + value: crate::types::card_type::Supertype::Basic, + }, + ])), + count: QuantityExpr::Fixed { value: 1 }, + reveal: false, + target_player: Some(TargetFilter::ParentTargetController), + up_to: false, + selection_constraint: SearchSelectionConstraint::None, + }, + vec![], + ObjectId(9000), + PlayerId(0), + ) + .sub_ability(put); + search.repeat_for = Some(QuantityExpr::Ref { + qty: QuantityRef::TrackedSetSize, + }); + + let mut all_events: Vec = Vec::new(); + // depth=1 to preserve the chain-scoped tracked set we published above. + resolve_ability_chain(&mut state, &search, &mut all_events, 1).unwrap(); + + // Iteration 0: P1 prompted; P1 picks the Forest. + match &state.waiting_for { + WaitingFor::SearchChoice { player, .. } => assert_eq!(*player, PlayerId(1)), + other => panic!("expected SearchChoice for P1, got {:?}", other), + } + let r1 = apply( + &mut state, + PlayerId(1), + GameAction::SelectCards { + cards: vec![p1_forest], + }, + ) + .unwrap(); + all_events.extend(r1.events); + + // Iteration 1: P2 prompted; P2 picks the Plains. + match &state.waiting_for { + WaitingFor::SearchChoice { player, .. } => assert_eq!( + *player, + PlayerId(2), + "iteration 1 must prompt P2 — controller of the SECOND tracked-set member" + ), + other => panic!("expected SearchChoice for P2, got {:?}", other), + } + let r2 = apply( + &mut state, + PlayerId(2), + GameAction::SelectCards { + cards: vec![p2_plains], + }, + ) + .unwrap(); + all_events.extend(r2.events); + + // Both chosen lands MUST be on the battlefield. This is the regression + // that the resumed-iteration `sub_ability` preservation guards against + // — without it, p2_plains would still be in P2's library. + let forest_zone = state.objects.get(&p1_forest).unwrap().zone; + let plains_zone = state.objects.get(&p2_plains).unwrap().zone; + assert_eq!( + forest_zone, + Zone::Battlefield, + "P1's chosen Forest must be on the battlefield (iteration 0's sub_ability)" + ); + assert_eq!( + plains_zone, + Zone::Battlefield, + "P2's chosen Plains must be on the battlefield — failure means iteration 1's \ + sub_ability (put-onto-battlefield) was dropped on the resume path." + ); + + // Both controllers must own their respective lands on their side. + assert_eq!( + state.objects.get(&p1_forest).unwrap().controller, + PlayerId(1), + "Forest controller is P1" + ); + assert_eq!( + state.objects.get(&p2_plains).unwrap().controller, + PlayerId(2), + "Plains controller is P2" + ); + + // The Shuffle sub_ability must have run for each iteration. Each + // Shuffle resolution emits an EffectResolved { kind: Shuffle } event. + let shuffle_count = all_events + .iter() + .filter(|e| { + matches!( + e, + GameEvent::EffectResolved { + kind: EffectKind::Shuffle, + .. + } + ) + }) + .count(); + assert!( + shuffle_count >= 2, + "expected at least 2 Shuffle resolutions (one per iteration), got {}. \ + Failure means iteration 1's Shuffle sub_ability was dropped on the resume path.", + shuffle_count + ); + + assert!( + state.pending_repeat_iteration.is_none(), + "loop must clear pending_repeat_iteration after final iteration completes" + ); + } + + /// CR 609.3 + CR 109.5: Regression — `drain_pending_repeat_iteration` must + /// detect a synchronously-installed `pending_continuation` mid-iteration + /// and re-stash the remaining iterations. Without this, an iteration that + /// completes synchronously (no `waiting_for` transition) but stashes a + /// continuation would let subsequent iterations run with the prior + /// continuation still pending — clobbering it — or, in the trailing-break + /// case, silently drop subsequent iterations entirely. + /// + /// We construct a synthetic resume by hand: stage a + /// `PendingRepeatIteration` whose ability has a sub_ability chain that + /// completes synchronously but whose parent effect installs a continuation + /// after the FIRST resumed iteration. The drain must observe the + /// continuation transition and re-stash for iteration 2. + #[test] + fn drain_pending_repeat_iteration_restashes_on_synchronous_continuation() { + use crate::types::game_state::PendingRepeatIteration; + + let mut state = GameState::new_two_player(42); + // Seed P0's library so Draw has cards to draw. + for i in 0..5 { + create_object( + &mut state, + CardId(i + 10), + PlayerId(0), + format!("Card {}", i), + Zone::Library, + ); + } + + // Build a Draw ability with a chained sub_ability (also Draw). A pure + // Draw resolves synchronously — no waiting_for transition — but the + // sub_ability chain stashes through the line-1461 wiring on + // `pending_continuation` only when the parent transitions to a choice + // state. To force the synchronous-continuation path we install a + // pre-existing continuation manually before the drain runs the second + // iteration: the drain must see the unchanged continuation from + // iteration N's resolve and re-stash for N+1. + // + // Simpler isolation: install a `pending_continuation` between + // iterations by setting it as the initial state, then asserting the + // drain re-stashes when it sees the continuation present at install + // time NOT match an iteration's installation. Use the + // `installed_continuation` predicate by starting with no continuation + // and forcing the inner resolve to produce one. We achieve that by + // chaining a sub_ability under the iterated ability so each iteration + // wires it through line-1461. + let inner_draw = ResolvedAbility::new( + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + vec![], + ObjectId(100), + PlayerId(0), + ); + let mut iter_ability = ResolvedAbility::new( + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + vec![], + ObjectId(100), + PlayerId(0), + ) + .sub_ability(inner_draw); + // repeat_for is cleared on the resume copy by stash-time logic; mirror that. + iter_ability.repeat_for = None; + + // Stage a synthetic resume: 3 total iterations starting from iteration 1. + state.pending_repeat_iteration = Some(PendingRepeatIteration { + ability: Box::new(iter_ability), + tracked_members: vec![], + next_iteration: 1, + total_iterations: 3, + }); + + let mut events = Vec::new(); + super::drain_pending_repeat_iteration(&mut state, &mut events); + + // All three iterations completed synchronously (Draw + sub Draw). + // pending_repeat_iteration must be cleared, no iterations dropped. + // P0 should have drawn 2 cards per iteration × 2 iterations (1 + 2) + // = 4 cards (iteration 1 = parent draw + sub draw, iteration 2 = same). + assert!( + state.pending_repeat_iteration.is_none(), + "pending_repeat_iteration must clear once all iterations complete" + ); + assert_eq!( + state.players[0].hand.len(), + 4, + "two synchronous iterations × (parent Draw + sub Draw) = 4 cards" + ); + } + /// CR 603.7 + CR 608.2c: Regression — when `repeat_for` is set but the /// effect does NOT use a parent-target reference (e.g. plain Draw), the /// per-iteration rebind logic must NOT touch `ability.targets`. Guards From ee4d32617b676557c651ef552f97fc7b3afe4d9d Mon Sep 17 00:00:00 2001 From: Matt Evans <1388610+matthewevans@users.noreply.github.com> Date: Sun, 26 Apr 2026 21:15:15 -0700 Subject: [PATCH 4/6] fix(parser): narrow normalize_verb_token's -es rule to known predicate verbs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous orthographic rule stripped `-es` whenever the resulting stem ended in `ch`/`sh`/`ss`/`x`/`z`. This was correct for `searches → search` but over-applied to the `-eze`/`-eeze` family, producing invented stems like `freezes → freez`, `breezes → breez`, `sneezes → sneez` — none of which match any downstream lookup, but they violate the project's "parser must not swallow" rule by silently fabricating non-existent words for unknown inputs. Narrow the rule to only strip `-es` when the resulting stem is a registered `PREDICATE_VERBS` member. Unknown verbs now pass through unchanged, and the only verbs that take the strip are ones the parser explicitly knows about. Adds a regression test that asserts neither `freezes` nor `breezes` nor `sneezes` produces an invented stem, while the registered `searches → search` case still works. PR #124 review feedback (findings #4, #5 third test). --- crates/engine/src/parser/oracle_effect/mod.rs | 47 +++++++++++++++---- 1 file changed, 39 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 0d1bbecf15..65447fea48 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -9050,17 +9050,16 @@ pub(crate) fn normalize_verb_token(token: &str) -> String { // verbs ending in /ʃ/, /tʃ/, /s/, /z/, /ks/ take "-es" instead of "-s" // for third-person singular (search→searches, wash→washes, // watch→watches, fix→fixes, buzz→buzzes). Strip "es" only when the - // resulting stem ends in `ch`/`sh`/`x`/`z` or is already an `-ss` - // stem (kiss→kisses). Without this branch, "searches" normalizes to - // "searche", which fails the PREDICATE_VERBS lookup and routes "its - // controller searches their library" to the Unimplemented fallback. + // resulting stem is a known predicate verb — without this validator + // the rule over-strips real `-eze`/`-eeze` words like "freezes", + // "breezes", "sneezes" into nonsense stems ("freez", "breez", "sneez") + // that no downstream lookup recognizes. The PREDICATE_VERBS guard + // ensures unknown words pass through unchanged ("parser must not + // swallow"); only registered verbs are de-conjugated. // allow-noncombinator: verb-morphology suffix check on pre-tokenized word _ if token.ends_with("es") && token.len() > 2 && { let stem = &token[..token.len() - 2]; - // allow-noncombinator: structural suffix check on pre-tokenized stem - ["ch", "sh", "ss"].iter().any(|s| stem.ends_with(s)) - || stem.ends_with('x') - || stem.ends_with('z') + crate::parser::oracle_effect::subject::PREDICATE_VERBS.contains(&stem) } => { token[..token.len() - 2].to_string() @@ -9232,6 +9231,38 @@ mod tests { use crate::types::mana::ManaColor; use crate::types::zones::Zone; + /// Parser must not invent verb stems for unknown words. The "-es" stripping + /// rule in `normalize_verb_token` was previously triggered purely by + /// orthographic suffix (stem ends in `ch`/`sh`/`ss`/`x`/`z`), which + /// produced nonsense stems like `freez`/`breez`/`sneez` for the `-eze`/ + /// `-eeze` family. The narrowed rule only strips when the result is a + /// known `PREDICATE_VERBS` member, so unrecognized verbs pass through + /// unchanged ("parser must not swallow"). + #[test] + fn normalize_verb_token_does_not_invent_stems_for_unknown_verbs() { + // -eze / -eeze words must NOT be over-stripped. The actual stem + // ("freeze", "breeze", "sneeze") is not in PREDICATE_VERBS, so the + // -es branch must reject the strip and fall through to the -s branch + // (which strips a single trailing 's' but only if the word doesn't + // end in 'ss' — these end in 'es', not 'ss', so 's' strip applies and + // yields "freeze"/"breeze"/"sneeze"). The critical assertion is the + // negative: we never produce "freez"/"breez"/"sneez". + for token in ["freezes", "breezes", "sneezes"] { + let normalized = normalize_verb_token(token); + assert_ne!( + normalized, + &token[..token.len() - 2], + "normalize_verb_token({token:?}) must not produce the invented stem {:?}", + &token[..token.len() - 2] + ); + } + + // Sanity check: the registered "search" verb still de-conjugates so + // the Winds of Abandon-class chain ("its controller searches their + // library") continues to dispatch through the predicate path. + assert_eq!(normalize_verb_token("searches"), "search"); + } + /// CR 608.2c: "If , you may instead " must produce /// a conditional Dig alternative where the `else_ability` is the base Dig /// (with its patched filter) and the outer Dig carries the alternative's From 5092f09cdac4fbc35bc16e0fbccc36d301d849cb Mon Sep 17 00:00:00 2001 From: Matt Evans <1388610+matthewevans@users.noreply.github.com> Date: Sun, 26 Apr 2026 21:24:02 -0700 Subject: [PATCH 5/6] test(engine): genuinely exercise synchronous-continuation re-stash predicate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous version of `drain_pending_repeat_iteration_restashes_on_synchronous_continuation` constructed a Draw + sub-Draw setup that completed each iteration cleanly without ever installing a `pending_continuation` synchronously — meaning the `installed_continuation` predicate was never evaluated true. The test passed regardless of whether the predicate existed. Rewrite the test to use `ConditionInstead` with an `else_ability` and a non-Priority pre-set `waiting_for`, which exercises the line-1486 path that synchronously stashes the else branch into `pending_continuation` without changing `waiting_for`. Verified by temporarily disabling the `installed_continuation` predicate: the test now FAILS with the exact "pending_continuation overwritten before consumption" debug_assert that finding #2 describes. Asserts after one resumed iteration: - `pending_continuation` is Some (else_ability was stashed synchronously) - `pending_repeat_iteration` is re-stashed with `next_iteration = 2` - only iteration 1's parent Draw fired (1 card), proving the drain broke immediately on the synchronous-continuation transition PR #124 review feedback (finding #2 — proper test coverage). --- crates/engine/src/game/effects/mod.rs | 120 +++++++++++++++++--------- 1 file changed, 77 insertions(+), 43 deletions(-) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index a3454833b1..2792fd4c3e 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -3630,26 +3630,26 @@ mod tests { ); } - /// CR 609.3 + CR 109.5: Regression — `drain_pending_repeat_iteration` must - /// detect a synchronously-installed `pending_continuation` mid-iteration - /// and re-stash the remaining iterations. Without this, an iteration that - /// completes synchronously (no `waiting_for` transition) but stashes a - /// continuation would let subsequent iterations run with the prior - /// continuation still pending — clobbering it — or, in the trailing-break - /// case, silently drop subsequent iterations entirely. + /// CR 609.3 + CR 109.5: Direct unit test of the synchronous-continuation + /// re-stash predicate inside `drain_pending_repeat_iteration`. Constructs + /// a multi-iteration resume whose iterations install a `pending_continuation` + /// without changing `waiting_for`, then verifies the drain detects the + /// continuation transition and re-stashes the remaining iterations rather + /// than letting them be silently dropped. /// - /// We construct a synthetic resume by hand: stage a - /// `PendingRepeatIteration` whose ability has a sub_ability chain that - /// completes synchronously but whose parent effect installs a continuation - /// after the FIRST resumed iteration. The drain must observe the - /// continuation transition and re-stash for iteration 2. + /// Strategy: use `ConditionInstead` with `else_ability` set. When the + /// instead condition is NOT met, line 1486-1487 of `resolve_ability_chain` + /// stashes the `else_ability` chain into `pending_continuation` whenever + /// `waiting_for != Priority`. We pre-set `waiting_for` to a non-Priority + /// state so the stash fires synchronously without any waiting_for change, + /// directly exercising the new `installed_continuation` predicate. #[test] fn drain_pending_repeat_iteration_restashes_on_synchronous_continuation() { + use crate::types::ability::AbilityCondition; use crate::types::game_state::PendingRepeatIteration; let mut state = GameState::new_two_player(42); - // Seed P0's library so Draw has cards to draw. - for i in 0..5 { + for i in 0..10 { create_object( &mut state, CardId(i + 10), @@ -3659,24 +3659,27 @@ mod tests { ); } - // Build a Draw ability with a chained sub_ability (also Draw). A pure - // Draw resolves synchronously — no waiting_for transition — but the - // sub_ability chain stashes through the line-1461 wiring on - // `pending_continuation` only when the parent transitions to a choice - // state. To force the synchronous-continuation path we install a - // pre-existing continuation manually before the drain runs the second - // iteration: the drain must see the unchanged continuation from - // iteration N's resolve and re-stash for N+1. - // - // Simpler isolation: install a `pending_continuation` between - // iterations by setting it as the initial state, then asserting the - // drain re-stashes when it sees the continuation present at install - // time NOT match an iteration's installation. Use the - // `installed_continuation` predicate by starting with no continuation - // and forcing the inner resolve to produce one. We achieve that by - // chaining a sub_ability under the iterated ability so each iteration - // wires it through line-1461. - let inner_draw = ResolvedAbility::new( + // Pre-seed waiting_for to a non-Priority state so the + // `ConditionInstead` else-branch stash path fires synchronously + // (line 1486 requires `waiting_for != Priority`). The drain's + // `entered_choice` predicate compares against this initial value, so + // the same waiting_for at end-of-iteration registers as "no + // transition" — only `installed_continuation` can fire the re-stash. + state.waiting_for = WaitingFor::SearchChoice { + player: PlayerId(0), + cards: vec![], + count: 0, + reveal: false, + up_to: true, + constraint: crate::types::ability::SearchSelectionConstraint::None, + }; + + // Build a Draw ability (synchronous, no waiting_for change) with a + // sub_ability whose condition is `ConditionInstead` carrying an + // `else_ability`. When the inner condition evaluates to false, the + // else branch is stashed synchronously into pending_continuation + // via line 1486. + let else_branch = ResolvedAbility::new( Effect::Draw { count: QuantityExpr::Fixed { value: 1 }, target: TargetFilter::Controller, @@ -3685,6 +3688,22 @@ mod tests { ObjectId(100), PlayerId(0), ); + let mut sub = ResolvedAbility::new( + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + vec![], + ObjectId(100), + PlayerId(0), + ) + .condition(AbilityCondition::ConditionInstead { + // Pick a condition that evaluates to false in this state so the + // swap does NOT fire and the else branch stash path runs. + inner: Box::new(AbilityCondition::IsYourTurn { negated: true }), + }); + sub.else_ability = Some(Box::new(else_branch)); + let mut iter_ability = ResolvedAbility::new( Effect::Draw { count: QuantityExpr::Fixed { value: 1 }, @@ -3694,11 +3713,10 @@ mod tests { ObjectId(100), PlayerId(0), ) - .sub_ability(inner_draw); - // repeat_for is cleared on the resume copy by stash-time logic; mirror that. + .sub_ability(sub); iter_ability.repeat_for = None; - // Stage a synthetic resume: 3 total iterations starting from iteration 1. + // Stage a resume of 3 iterations starting at iteration 1. state.pending_repeat_iteration = Some(PendingRepeatIteration { ability: Box::new(iter_ability), tracked_members: vec![], @@ -3709,18 +3727,34 @@ mod tests { let mut events = Vec::new(); super::drain_pending_repeat_iteration(&mut state, &mut events); - // All three iterations completed synchronously (Draw + sub Draw). - // pending_repeat_iteration must be cleared, no iterations dropped. - // P0 should have drawn 2 cards per iteration × 2 iterations (1 + 2) - // = 4 cards (iteration 1 = parent draw + sub draw, iteration 2 = same). + // Iteration 1 ran: parent Draw fired (1 card), then the + // ConditionInstead sub stashed its else_ability into + // pending_continuation synchronously (no waiting_for change). The + // drain's `installed_continuation` predicate must observe this + // transition and re-stash iteration 2 for the next drain pass. assert!( - state.pending_repeat_iteration.is_none(), - "pending_repeat_iteration must clear once all iterations complete" + state.pending_continuation.is_some(), + "iteration 1 must have installed a synchronous pending_continuation \ + (else_ability of ConditionInstead)" + ); + let pending = state.pending_repeat_iteration.as_ref().expect( + "iteration 2 must be re-stashed — without the synchronous-continuation \ + predicate, this would be None and iteration 2 would be silently dropped", ); + assert_eq!( + pending.next_iteration, 2, + "re-stash must advance to iteration 2" + ); + assert_eq!(pending.total_iterations, 3); + + // Exactly one iteration's worth of effects fired before the break: + // iteration 1's parent Draw (1 card). The else_ability chain has not + // run yet — it is stashed in pending_continuation, awaiting the next + // drain_pending_continuation call. assert_eq!( state.players[0].hand.len(), - 4, - "two synchronous iterations × (parent Draw + sub Draw) = 4 cards" + 1, + "only iteration 1's parent Draw should have fired before the re-stash break" ); } From 8eca890a91c877f3d04de40b086451e4fb4097c9 Mon Sep 17 00:00:00 2001 From: Matt Evans <1388610+matthewevans@users.noreply.github.com> Date: Mon, 27 Apr 2026 12:22:50 -0700 Subject: [PATCH 6/6] feat(parser): accept dynamic QuantityRef RHS in mana-value comparator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parse_mana_value_suffix only matched the trigger-anaphoric "that " form on the right-hand side of "less than [or equal to]" / "greater than [or equal to]". Cards with non-anaphoric quantity-ref RHS phrases (Beseech the Queen "the number of lands you control", Bring to Light "the number of colors of mana spent to cast this spell", Vhal of Creation, Dominating Vampire, Drown in the Loch, Dreadhorde Arcanist, etc.) silently dropped the comparator — the search filter parser warned but the AST silently fell back to no MV constraint. Extended the combinator to delegate the RHS to the shared parse_quantity_ref building block when "that " doesn't follow. The phrase boundary is the next sentence terminator (',', '.'), preventing over-consumption into trailing search-and-shuffle clauses. Inverted cast_free_unqualified_rejects_complex_mv_filter to cast_free_unqualified_accepts_dynamic_mv_filter — the test's premise was specifically about the missing capability that this commit adds. +6 cards gained vs baseline, 0 regressions. --- crates/engine/src/game/effects/mod.rs | 3 -- crates/engine/src/parser/oracle_static.rs | 41 +++++++++++++++++++---- crates/engine/src/parser/oracle_target.rs | 41 +++++++++++++++++------ 3 files changed, 65 insertions(+), 20 deletions(-) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index cbd4770bcc..f07fd80229 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -3229,7 +3229,6 @@ mod tests { count: QuantityExpr::Fixed { value: 1 }, reveal: false, target_player: Some(TargetFilter::ParentTargetController), - up_to: false, selection_constraint: SearchSelectionConstraint::None, }, vec![], @@ -3353,7 +3352,6 @@ mod tests { count: QuantityExpr::Fixed { value: 1 }, reveal: false, target_player: Some(TargetFilter::ParentTargetController), - up_to: false, selection_constraint: SearchSelectionConstraint::None, }, vec![], @@ -3530,7 +3528,6 @@ mod tests { count: QuantityExpr::Fixed { value: 1 }, reveal: false, target_player: Some(TargetFilter::ParentTargetController), - up_to: false, selection_constraint: SearchSelectionConstraint::None, }, vec![], diff --git a/crates/engine/src/parser/oracle_static.rs b/crates/engine/src/parser/oracle_static.rs index 321b8ea71a..9a38452cee 100644 --- a/crates/engine/src/parser/oracle_static.rs +++ b/crates/engine/src/parser/oracle_static.rs @@ -9652,15 +9652,42 @@ mod tests { } } - // CR 601.2: Unqualified branch must reject filters that `parse_type_phrase` - // can't fully consume — Fires of Invention's dynamic-MV filter would - // otherwise misparse as `TargetFilter::Any` (full Omniscience). Better to - // decline than to silently overgrant casting permission. - #[test] - fn cast_free_unqualified_rejects_complex_mv_filter() { + // CR 601.2 + CR 119.3: Unqualified branch now accepts dynamic mana-value + // filters whose RHS is any `parse_quantity_ref` phrase (Fires of Invention + // class). Earlier the comparator only matched the trigger-anaphoric + // `that ` form, so this filter fell through to a partial parse and + // the test asserted the rejection (better-decline-than-overgrant). The + // comparator was extended to delegate the RHS to the shared + // `parse_quantity_ref` building block, so the filter now fully types as + // `CmcLE { value: Ref { ObjectCount { Land, You } } }` and the cast-free + // permission can carry it. The test is inverted: it now asserts the + // typed filter is preserved end-to-end. + #[test] + fn cast_free_unqualified_accepts_dynamic_mv_filter() { + use crate::types::ability::{FilterProp, QuantityExpr, QuantityRef, TargetFilter}; let text = "You may cast spells with mana value less than or equal to the number of lands you control without paying their mana costs."; let lower = text.to_lowercase(); - assert!(try_parse_cast_free_permission(text, &lower).is_none()); + let def = try_parse_cast_free_permission(text, &lower) + .expect("dynamic-MV filter should parse end-to-end"); + let filter = def.affected.expect("affected filter must be present"); + let TargetFilter::Typed(tf) = filter else { + panic!("expected Typed filter for Fires-of-Invention class"); + }; + let has_dynamic_cmc_le = tf.properties.iter().any(|p| { + matches!( + p, + FilterProp::CmcLE { + value: QuantityExpr::Ref { + qty: QuantityRef::ObjectCount { .. } + } + } + ) + }); + assert!( + has_dynamic_cmc_le, + "expected CmcLE with dynamic ObjectCount RHS, got {:?}", + tf.properties + ); } // Negative test: text without "without paying" must not match the diff --git a/crates/engine/src/parser/oracle_target.rs b/crates/engine/src/parser/oracle_target.rs index a0b89c92a1..56082e30fd 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -1901,17 +1901,38 @@ pub(crate) fn parse_mana_value_suffix(text: &str) -> Option<(FilterProp, usize)> } else { (false, a) }; - let (a, _) = tag::<_, _, Vbe>("that ").parse(a).ok()?; - // CR 120.3: "that damage" — the damage amount captured by the trigger - // (DamageDone events stamp `EventContextAmount`). - let (qty, after) = if let Ok((a2, _)) = tag::<_, _, Vbe>("damage").parse(a) { - (QuantityRef::EventContextAmount, a2) + // CR 120.3: Anaphoric "that " forms — bind to the trigger context. + // CR 119.3: Non-anaphoric quantity-ref forms — bind to a static or + // game-state quantity ("the number of lands you control", + // "the number of cards in your graveyard", "the amount of life you + // gained this turn", etc.). The two forms are mutually exclusive at + // this position; try anaphoric first, then fall through. + let (qty, after) = if let Ok((a2, _)) = tag::<_, _, Vbe>("that ").parse(a) { + // CR 120.3: "that damage" — the damage amount captured by the trigger + // (DamageDone events stamp `EventContextAmount`). + if let Ok((a3, _)) = tag::<_, _, Vbe>("damage").parse(a2) { + (QuantityRef::EventContextAmount, a3) + } else { + // Fall back to the type-word arm — "that " where is any + // single word terminating at punctuation/space (e.g., "creature", + // "spell"). Uses the source object's mana value. + let after = a2.find([',', '.', ' ']).map_or(a2, |i| &a2[i..]); + (QuantityRef::EventContextSourceManaValue, after) + } } else { - // Fall back to the type-word arm — "that " where is any - // single word terminating at punctuation/space (e.g., "creature", - // "spell"). Uses the source object's mana value. - let after = a.find([',', '.', ' ']).map_or(a, |i| &a[i..]); - (QuantityRef::EventContextSourceManaValue, after) + // CR 119.3: Generic quantity-ref RHS — extract the phrase up to the + // next sentence-terminating punctuation and delegate to the shared + // `parse_quantity_ref` building block. Unlocks Vhal's "the number + // of study counters removed this way", Beseech the Queen's "the + // number of lands you control", Bring to Light's "the number of + // colors of mana spent to cast this spell", etc. The terminator + // boundary (comma / period / end-of-input) prevents over-consuming + // into trailing search-and-shuffle clauses ("…, reveal it, put it + // into your hand" on Beseech the Queen). + let phrase_end = a.find([',', '.']).unwrap_or(a.len()); + let phrase = &a[..phrase_end]; + let qty = crate::parser::oracle_quantity::parse_quantity_ref(phrase)?; + (qty, &a[phrase_end..]) }; let make_value = |off: i32| { if off == 0 {