Skip to content

fix(parser): recover multi-target spec for tap/untap up-to phrases#3752

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
andriypolanski:fix/recover-multi-target-spec
Jun 18, 2026
Merged

fix(parser): recover multi-target spec for tap/untap up-to phrases#3752
matthewevans merged 3 commits into
phase-rs:mainfrom
andriypolanski:fix/recover-multi-target-spec

Conversation

@andriypolanski

Copy link
Copy Markdown
Contributor

Summary

Elder Deep-Fiend's "When you cast this spell, tap up to four target permanents" was parsed as a single-target tap effect because the imperative parser stripped the "up to four target" quantifier but never stamped MultiTargetSpec onto the ability. At cast time the engine surfaced a malformed TriggerTargetSelection (one mandatory slot instead of up to four optional slots), which could crash the React client with a blank blue screen.

This PR adds two post-parse recovery helpers in oracle_effect/lower.rs, mirroring the existing extract_exact_target_multi_target / extract_bounded_target_multi_target pattern:

  • extract_optional_target_multi_target — recovers "tap up to four target permanents" and the full MULTI_TARGET_VERBS class (exile, tap, untap, goad, return, destroy, choose).
  • extract_verb_up_to_multi_target — recovers non-target-word shapes like "untap up to five lands" (Peregrine Drake class) via the existing strip_any_number_quantifier building block.

Closes #3274

Root cause

The Fight imperative path already preserved multi_target through TargetedImperativeAst::Fight and a lower_imperative_family_ast intercept. Tap/Untap imperatives called strip_optional_target_prefix but discarded the returned spec. The ability lowerer only recovered exact-count and bounded-count phrases, not "up to N target …".

Changes

File Change
crates/engine/src/parser/oracle_effect/lower.rs Add extract_optional_target_multi_target, extract_verb_up_to_multi_target; wire into ability-definition lowering; unit tests
crates/engine/tests/integration/issue_3274_elder_deep_fiend.rs Parser + runtime regression tests
crates/engine/tests/integration/main.rs Register new integration module

Test plan

  • extract_optional_target_multi_target_recovers_tap_up_to_four (unit)
  • extract_verb_up_to_multi_target_recovers_untap_lands (unit)
  • elder_deep_fiend_cast_trigger_parses_up_to_four_multi_target — AST carries MultiTargetSpec::up_to(4)
  • elder_deep_fiend_cast_trigger_surfaces_four_optional_target_slots — casting with ≥4 legal permanents yields four optional TriggerTargetSelection slots
  • Manual: cast Elder Deep-Fiend (normal or Emerge) in browser; confirm targeting overlay accepts 0–4 permanents without blank-screen crash

Closes

Fixes #3274

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@andriypolanski andriypolanski changed the title fix(parser): recover multi-target spec for tap/untap up-to phrases (#… fix(parser): recover multi-target spec for tap/untap up-to phrases Jun 18, 2026
@matthewevans matthewevans added the bug Bug fix label Jun 18, 2026
@andriypolanski andriypolanski force-pushed the fix/recover-multi-target-spec branch from eb1c82c to ccc45fe Compare June 18, 2026 22:41
@matthewevans matthewevans self-assigned this Jun 18, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: approve

Parser/runtime regression reviewed clean, maintainer/contributor clippy fix is limited to the test pattern, CI is green, and the current head is labeled bug.

@matthewevans matthewevans added this pull request to the merge queue Jun 18, 2026
@matthewevans matthewevans removed their assignment Jun 18, 2026
Merged via the queue into phase-rs:main with commit f635293 Jun 18, 2026
17 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Elder Deep-fiend seems to crash the platform — I wasn't sure if it was [[Elder Deep-Fiend]], or some combination of car…

2 participants