From f69fc75fcdb4628a2df7ce4f0135a65d6d80b58f Mon Sep 17 00:00:00 2001 From: Christopher Galpin <52485+CodeOptimist@users.noreply.github.com> Date: Sun, 2 Aug 2026 00:47:18 -0600 Subject: [PATCH] fix(engine): stop search tutors double-firing landfall/ETB observers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Search tutors that put a card into its destination zone (e.g. Nature's Lore / Cultivate / fetchlands putting a land onto the battlefield) fired every observer triggered ability twice for a single zone change. A single land entry from a search tutor fired each landfall source twice (and each ETB observer twice). ## Root cause `park_search_observer_triggers` (engine_resolution_choices.rs) parks a search's post-put/shuffle events for the next priority checkpoint. But the `ZoneChanged` events for cards put into their destination zone by the search's `ChangeZone` delivery were **already** collected (exactly once) by the zone-change pipeline's segment/settlement collections (`append_and_collect_logical_zone_trigger_segment` / `complete_logical_zone_trigger_collection`). Those same `ZoneChanged` occurrences still sit in `events[events_before_drain..]`, so parking the whole slice re-collected each entrant's observers a second time — a single land entry fired landfall twice. ## Fix In `park_search_observer_triggers`, drop any `ZoneChanged` occurrence whose event is already represented in `state.deferred_triggers`, mirroring the existing `deferred_logical_zone_events` guard in `engine_priority.rs`. The dedup is value-equality over the full `ZoneChanged` event, so: - a genuinely new/different zone change still fires its observers exactly once; - multiple distinct cards entering from one search each fire once (never collapsed); - non-zone events (EffectResolved, Shuffle, PlayerPerformedAction, ...) remain eligible and their observers are still parked; - the reference-based `retained_zone_events` collection matches the `deferred_logical_zone_events` idiom exactly (no extra allocation). ## Testing - New integration repro `search_landfall_double_fire_repro.rs`: - single landfall source + single land from a search tutor fires exactly once; - two landfall sources + single land fire exactly once each (two total). - Verified the tests are discriminating: reverting the dedup makes them fail with landfall firing twice (2 and 4 instead of 1 and 2). - `cargo clippy -p phase-engine --all-targets -- -D warnings`: clean. - Full `cargo nextest -p phase-engine`: 22,606 integration + 18,229 unit tests pass, including the 72-test search/landfall/ETB surface and the sibling issue_5336 Kodama Nature's Lore path. --- .../src/game/engine_resolution_choices.rs | 24 +++ crates/engine/tests/integration/main.rs | 1 + .../search_landfall_double_fire_repro.rs | 155 ++++++++++++++++++ 3 files changed, 180 insertions(+) create mode 100644 crates/engine/tests/integration/search_landfall_double_fire_repro.rs diff --git a/crates/engine/src/game/engine_resolution_choices.rs b/crates/engine/src/game/engine_resolution_choices.rs index 9dc5efc027..f624d4a647 100644 --- a/crates/engine/src/game/engine_resolution_choices.rs +++ b/crates/engine/src/game/engine_resolution_choices.rs @@ -616,9 +616,33 @@ fn park_search_observer_triggers( events: &[GameEvent], events_before_drain: usize, ) -> ResolutionChoiceOutcome { + // CR 603.2 + CR 603.3b + CR 701.23: park the search's post-put/shuffle + // observer events for the next priority checkpoint. The ZoneChanged events + // for cards put onto the battlefield by the search's ChangeZone delivery + // were ALREADY collected (once) by the zone-change pipeline's segment / + // settlement collections (`append_and_collect_logical_zone_trigger_segment` + // / `complete_logical_zone_trigger_collection`). Those same `ZoneChanged` + // occurrences sit in `events[events_before_drain..]`, so collecting this + // whole slice again would DOUBLE-fire every entrant's observer triggers + // (landfall, ETB observers) — a single land entry fires landfall twice. + // + // Mirror the generic post-priority scan's `deferred_logical_zone_events` + // guard (engine_priority.rs): drop any `ZoneChanged` occurrence whose event + // is already represented in `state.deferred_triggers`. Other event kinds + // (EffectResolved, Shuffle, PlayerPerformedAction, ...) remain eligible so + // their own observers are still parked. + let retained_zone_events: Vec<_> = state + .deferred_triggers + .iter() + .flat_map(|context| context.trigger_events.iter()) + .filter(|event| matches!(event, GameEvent::ZoneChanged { .. })) + .collect(); let trigger_events: Vec = events[events_before_drain..] .iter() .filter(|ev| !matches!(ev, GameEvent::PhaseChanged { .. })) + .filter(|ev| { + !matches!(ev, GameEvent::ZoneChanged { .. }) || !retained_zone_events.contains(ev) + }) .cloned() .collect(); if !trigger_events.is_empty() { diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 70234a00c7..9ef49d5c64 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -1158,6 +1158,7 @@ mod sarkhan_dragon_ascendant_behold; mod scarblade_malice_delayed_dies_762; mod scry_choice_not_clobbered_by_triggers; mod scry_substituted_draw_per_card_replacement; +mod search_landfall_double_fire_repro; mod secret_of_bloodbending_control_window; mod she_hulk_wallbreaker_becomes_blocked_4599; mod shelob_repro_token; diff --git a/crates/engine/tests/integration/search_landfall_double_fire_repro.rs b/crates/engine/tests/integration/search_landfall_double_fire_repro.rs new file mode 100644 index 0000000000..98ff212f40 --- /dev/null +++ b/crates/engine/tests/integration/search_landfall_double_fire_repro.rs @@ -0,0 +1,155 @@ +//! Repro for the reported double-fire: casting a search tutor that puts a land +//! onto the battlefield (Prishe's Wanderings / Nature's Lore pattern) fires each +//! landfall observer trigger twice for the single land entry. +//! +//! A landfall permanent ("Whenever a land enters the battlefield under your +//! control, ") must produce EXACTLY ONE trigger per land entry. + +use engine::game::scenario::{GameScenario, P0}; +use engine::types::mana::ManaColor; +use engine::types::phase::Phase; +use engine::types::zones::Zone; + +const LANDFALL_ORACLE: &str = + "Whenever a land enters the battlefield under your control, draw a card."; + +const NATURES_LORE_ORACLE: &str = + "Search your library for a Forest card, put that card onto the battlefield, then shuffle."; + +fn seed_forest_on_library_top( + runner: &mut engine::game::scenario::GameRunner, +) -> engine::types::identifiers::ObjectId { + use engine::types::card_type::CoreType; + let card_id = engine::types::identifiers::CardId(runner.state().next_object_id); + let id = engine::game::zones::create_object( + runner.state_mut(), + card_id, + P0, + "Forest".to_string(), + Zone::Library, + ); + let obj = runner.state_mut().objects.get_mut(&id).unwrap(); + obj.card_types.core_types.push(CoreType::Land); + obj.base_card_types = obj.card_types.clone(); + obj.card_types.subtypes.push("Forest".to_string()); + runner.state_mut().players[P0.0 as usize] + .library + .insert(0, id); + id +} + +#[test] +fn landfall_fires_once_per_land_etb_from_search_tutor() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.add_basic_land(P0, ManaColor::Green); + + // A landfall permanent on the battlefield. + scenario + .add_creature_from_oracle(P0, "Landfall Scout", 1, 1, LANDFALL_ORACLE) + .id(); + + let natures_lore = scenario + .add_spell_to_hand_from_oracle(P0, "Nature's Lore", false, NATURES_LORE_ORACLE) + .id(); + + let mut runner = scenario.build(); + let forest = seed_forest_on_library_top(&mut runner); + runner.cast(natures_lore).search_first_legal().resolve(); + + // The land entered the battlefield exactly once. + assert_eq!( + runner.state().objects[&forest].zone, + Zone::Battlefield, + "Nature's Lore must put the searched Forest onto the battlefield" + ); + + // Drain the search tutor to a priority window so deferred triggers settle. + runner.advance_until_stack_empty(); + + // Exactly ONE landfall trigger must have fired for the single land entry. + let landfall_count = runner + .state() + .deferred_triggers + .iter() + .filter(|ctx| { + ctx.pending + .description + .as_deref() + .unwrap_or("") + .contains("Whenever a land enters") + }) + .count(); + assert_eq!( + landfall_count, + 1, + "landfall must fire exactly once per land ETB; deferred=[{:?}]", + runner + .state() + .deferred_triggers + .iter() + .map(|c| c.pending.description.clone().unwrap_or_default()) + .collect::>() + ); +} + +/// The reported shape: TWO separate landfall permanents on the battlefield and +/// one land entering via a search tutor. Each landfall permanent must fire +/// exactly ONE trigger for the single land — two total, never four (the +/// double-fire reported for Sazh's Chocobo + Bird token). +#[test] +fn two_landfall_sources_fire_once_each_for_single_search_land() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.add_basic_land(P0, ManaColor::Green); + + scenario + .add_creature_from_oracle(P0, "Landfall Scout", 1, 1, LANDFALL_ORACLE) + .id(); + scenario + .add_creature_from_oracle(P0, "Landfall Warden", 2, 2, LANDFALL_ORACLE) + .id(); + + let natures_lore = scenario + .add_spell_to_hand_from_oracle(P0, "Nature's Lore", false, NATURES_LORE_ORACLE) + .id(); + + let mut runner = scenario.build(); + let forest = seed_forest_on_library_top(&mut runner); + runner.cast(natures_lore).search_first_legal().resolve(); + runner.advance_until_stack_empty(); + + // The land entered the battlefield exactly once. + assert_eq!( + runner.state().objects[&forest].zone, + Zone::Battlefield, + "Nature's Lore must put the searched Forest onto the battlefield" + ); + + let landfall_count = runner + .state() + .deferred_triggers + .iter() + .filter(|ctx| { + ctx.pending + .description + .as_deref() + .unwrap_or("") + .contains("Whenever a land enters") + }) + .count(); + assert_eq!( + landfall_count, + 2, + "two landfall sources must fire exactly once each for the single land ETB; deferred=[{:?}]", + runner + .state() + .deferred_triggers + .iter() + .map(|c| ( + c.pending.source_id, + c.pending.description.clone().unwrap_or_default() + )) + .collect::>() + ); +}