fix(engine): fire ETB observers when an as-enters-choice permanent enters (#830) - #3167
Conversation
…ion so ETB observers fire (#830)
There was a problem hiding this comment.
Code Review
This pull request addresses issue #830 where enters-the-battlefield (ETB) observers failed to trigger for permanents with 'As it enters, choose...' replacement effects. The solution consolidates deferred battlefield-entry event replay logic into a single helper, replay_deferred_entry_events, and extends mid-entry choice capture to support NamedChoice events. This ensures ETB triggers are deferred and replayed exactly once against the fully realized post-choice object. Comprehensive integration tests have been added to verify the fix and guard against regressions. No review comments were provided, so there is no feedback to address.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Summary
Fixes #830 — when a permanent enters carrying an "As it enters, choose …" replacement (e.g. Valgavoth's Lair → "choose a color"), the entry pauses on
WaitingFor::NamedChoiceinstead ofWaitingFor::Priority. The canonical priority-time trigger collection was skipped, so every ETB observer (constellation like Doomwake Giant, Soul Warden, etc.) was silently dropped for that entry.Root cause
run_post_action_pipelinecollects ETB triggers only on aWaitingFor::Priorityresult; an as-enters-choice entry returnsNamedChoice, so the entering permanent'sZoneChangednever reachedprocess_triggers.Fix
Generalize the existing
state.deferred_entry_eventsmechanism (previously gated to copy-target / counter choices) to capture as-entersNamedChoice { source_id: Some(_) }entries, and replay them on choice resolution through a sharedreplay_deferred_entry_eventshelper that surfaces interactiveOrderTriggers/DistributeAmongpauses. CR 614.12a (the choice is made before the permanent enters).Double-fire fix (engine_priority.rs)
The
CastSpellpath (creature/enchantment via stack resolution) does not skiprun_post_action_pipelinethe wayPlayLanddoes, so an as-enters-choice creature's ETB observer was collected twice — once from the still-present entryZoneChangedinevents, once from the deferred replay. The trigger scan now excludes events already indeferred_entry_eventswhile leaving them ineventsfor frontend animation (a "two readers, one writer" contract). This also eliminates a pre-existing latent double-fire on the copy-target-choice path.Class coverage
Engine-pipeline-level fix covering ~280 as-enters-choice permanents × the entire ETB-observer class — both
PlayLand(lands) andCastSpell(creatures/enchantments) entry paths.Tests (
constellation_enters_with_choice.rs, 3/3)as_enters_choice_land_fires_constellation— land path repro (toughness 2→1; discriminates zero-fire AND double-fire).plain_enchantment_still_fires_constellation— regression guard for the untouched Priority path.as_enters_choice_creature_fires_soul_warden— creature-via-stack double-fire discriminator (life +1, not +2).Plan reviewed clean. Implementation reviewed clean twice (the second review, after the double-fire fix, adversarially verified copy-path safety, hot-path perf N≈0,
GameEventPartialEqsoundness, and double-fire completeness). Fulltest-enginesuite green (12791 passed). CR numbers grep-verified.