fix(parser): Choice/modal AST nodes have no 'at random' selection axis, so 'choose ... at random' is

[ntindle]

Summary

Fixes a parser misparse affecting 3 card(s) in the Doctor Who Commander precons.

Root cause: Choice/modal AST nodes have no 'at random' selection axis, so 'choose ... at random' is parsed as a deliberate controller choice (CR 700.2 / 706 violated).

Cards corrected

• Cult of Skaro
• Strax, Sontaran Nurse
• River Song's Diary

Fix

Implemented WHO misparse cluster #5 ("choose ... at random" selection axis) plus the Strax damage-trigger object-recipient gap (Fix B). All four cluster cards now parse correctly in card-data.json: Cult of Skaro modal.selection={"type":"Random"}, Strax drop1 valid_target=Typed(Creature), Strax drop2 Choose.selection={"type":"Random"} with the WhenYouDo Fight sub intact, River Song's Diary ChooseFromZone.random=true.

FIX A (random selection axis): Added three typed selection fields reusing the two EXISTING {Chosen,Random} enums — no new enum type or variant (verified via grep + inventory): Effect::Choose.selection (TargetSelectionMode, type-tagged serde), Effect::ChooseFromZone.selection (CardSelectionMode via card_selection_bool_compat, bare-bool serde matching Discard/RevealHand), ModalChoice.selection (TargetSelectionMode). Parser detection uses only nom combinators / the sanctioned scan_contains qualifier-flag idiom (the imperative.rs:1233 precedent) across parse_choose_ast (NamedChoice), try_parse_choose_from_zone, parse_choose_anaphoric (FromTrackedSet), try_parse_choose_player_to_verb (the ACTUAL Strax path — the plan missed this; "choose a player" routes here, not parse_choose_ast), and parse_modal_header_ast. AST: added selection to ChooseImperativeAst::{NamedChoice,FromZone,FromTrackedSet} and ModalHeaderAst.

ENGINE RESOLUTION (Choose + ChooseFromZone): Implemented choose::resolve_random_in_chain and choose_from_zone::resolve_random_in_chain, called from resolve_ability_chain at the chain resolution point where a mutable ability exists (mirroring TargetSelectionMode::Random's random_select_targets_for_ability) — no interactive WaitingFor is raised. Extracted bind_named_choice (threading source_id: Option) as the shared persist/layer/last_named_choice authority between the interactive ChooseOption handler and the random resolver (the interactive handler now delegates to it — no behavior change; refactor preserves the 2924-2955 block byte-faithfully). The Choose-Player random path binds the chosen player onto the ability's chosen_players so the chain propagates it via apply_parent_chain_context.

CRITICAL CORRECTION the plan under-specified: the dependent reflexive Fight (ChosenPlayer-scoped target) needs chosen_players BEFORE its target slots are built. I added a small targeted fix to try_begin_reflexive_target_selection to propagate the parent's chosen_players onto the reflexive ability before build_target_slots runs. This is the only way the inline random Choose-Player path correctly resolves Strax's Fight.

FIX B (object-recipient damage trigger): Rewrote the oracle_trigger.rs:6010 doc comment to state the new "object axis IS matched" contract and the None->Typed tightening. Added a final alt() arm (preceded(alt((tag("a "),tag("an "))), parse_type_phrase_nom)) ordered AFTER all player-recipient arms so "to a creature/planeswalker/permanent" yields Typed(...). IMPORTANT MATCHER FIX (plan said no matcher change needed — that was WRONG): player_matches_filter's wildcard _ => true let a Typed(Creature) valid_target match a PLAYER recipient (Strax's combat damage to a player would still fire the +1/+1). I added is_object_only_damage_filter + a guard in match_damage_done's Player(pid) branch so an object-only filter rejects every player recipient — symmetric to the existing object-branch guard, and careful NOT to reject Or{Player,Planeswalker}. Verified directly in source that this gap was real and reachable.

CR annotations (all verified against docs/MagicCompRules.txt): CR 608.2d (line 2791, the announce-by-player default the "at random" override negates — primary anchor), CR 700.2b (3203, controller of modal triggered ability), CR 701.9b (3325, cited only as "(analogous)" precedent — never as governing rule), CR 120.1/120.3 (1085/1095, damage recipients object-vs-player), CR 102.2, CR 603.2, CR 109.4 (594), CR 607.2d/613.1 (preserved in bind_named_choice). Confirmed spec's "CR 706" is wrong (706 = Rolling a Die, line 5540) and is used in no annotation.

VERIFICATION (worktree NOT under Tilt, ran cargo directly): cargo fmt --all clean (--check exit 0); cargo clippy -p engine --all-targets exit 0 with ZERO warnings; cargo test -p engine all green (12130 lib + 1065 integration, 0 failed) — includes the 16 new building-block tests (parser at-random detection for all 3 constructs + non-random defaults, Fix B parser + matcher regression proving creature-matches/planeswalker-rejected/player-rejected, resolve_random_in_chain for both resolvers binding without prompting). Parser combinator diff gate PASS (only false positive: an Iterator::find in test code, not str dispatch). check-parser-combinators.sh failures are all pre-existing lines vs an old baseline, none in my added parser-dispatch lines. server-core + phase-ai build clean. ~138-146 compiler-enforced construction-site updates applied across engine/database/parser/tests + dormant mtgish-import + phase-ai + server-core (all default to Chosen). Updated the Questing Beast snapshot to the now-CORRECT Typed(Planeswalker) valid_target (was null) — the intended Fix B tightening. Did NOT commit. NOTE: full cargo build --workspace later hit a genuine environment "No space left on device" (disk 100% full, 411Gi used) AFTER all in-scope engine verification had already passed from cache; engine lib + tests + server-core re-verified green after freeing a little space.

Files changed

• crates/engine/src/types/ability.rs
• crates/engine/src/parser/oracle_ir/ast.rs
• crates/engine/src/parser/oracle_modal.rs
• crates/engine/src/parser/oracle_effect/imperative.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_effect/sequence.rs
• crates/engine/src/parser/oracle_effect/subject.rs
• crates/engine/src/parser/oracle.rs
• crates/engine/src/parser/oracle_replacement.rs
• crates/engine/src/parser/oracle_trigger.rs
• crates/engine/src/game/effects/choose.rs
• crates/engine/src/game/effects/choose_from_zone.rs
• crates/engine/src/game/effects/mod.rs
• crates/engine/src/game/engine_resolution_choices.rs
• crates/engine/src/game/trigger_matchers.rs
• crates/engine/src/game/engine.rs
• crates/engine/src/game/replacement.rs
• crates/engine/src/game/triggers.rs
• crates/engine/src/game/coverage.rs
• crates/engine/src/database/synthesis.rs
• crates/engine/tests/integration/issue_1374_atraxa_grand_unifier.rs
• crates/engine/tests/integration/frostcliff_siege_anchor_word_modes.rs
• crates/engine/tests/integration/issue_2360_the_rack.rs
• crates/engine/tests/integration/snapshots/integration__oracle_parser__snapshot_questing_beast.snap
• crates/phase-ai/src/policies/mill_targeting.rs
• crates/server-core/src/filter.rs
• crates/mtgish-import/src/convert/action.rs
• crates/mtgish-import/src/convert/mod.rs
• crates/mtgish-import/src/convert/replacement.rs

CR references

• CR 608.2d
• CR 700.2
• CR 700.2a
• CR 700.2b
• CR 700.2e
• CR 701.9b
• CR 120.1
• CR 120.3
• CR 102.2
• CR 603.2
• CR 109.4
• CR 607.2d
• CR 613.1

Verification

• cd /Users/ntindle/code/random/magic/phase-main-base && cargo fmt --all — pass - clean, no files changed, nothing to commit
• cd /Users/ntindle/code/random/magic/phase-main-base && ./scripts/check-parser-combinators.sh <upstream/main merge-base 8f4a4d1c6> — pass - exit 0, clean
• cd /Users/ntindle/code/random/magic/phase-main-base && cargo clippy -p engine --all-targets -- -D warnings — pass - no errors/warnings; the fix's own changed files are clippy-clean, no pre-existing debt surfaced
• cd /Users/ntindle/code/random/magic/phase-main-base && cargo test -p engine — pass - all green, 0 failures
• cd /Users/ntindle/code/random/magic/phase-main-base && cargo run --profile tool --features cli --bin oracle-gen -- data --filter "cult of skaro|strax, sontaran nurse|river song's diary" — pass - all three cards parse with the 'at random' selection axis present, no Unimplemented/Unknown gaps
  Cards confirmed re-parsed correctly: Cult of Skaro, River Song's Diary, Strax, Sontaran Nurse

🤖 Generated with Claude Code

[gemini-code-assist]

Warning

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

[matthewevans]

Verdict: Changes required — merge conflict blocks the queue (architecture otherwise sound)

Blocking: branch is CONFLICTING / DIRTY

mergeStateStatus: DIRTY, mergeable: CONFLICTING against origin/main at head fd205da. The merge queue cannot speculate a rebase through a textual conflict, so this cannot enqueue regardless of code quality. Please merge origin/main into the branch and resolve conflicts in the surrounding architectural style, then re-request review.

Architecture review (informational — looks good, pending conflict resolution)

The selection-axis design is idiomatic and lands at the right seam:

• No new enum / no new bool. Reuses the existing {Chosen, Random} enums — Effect::Choose.selection: TargetSelectionMode, Effect::ChooseFromZone.selection: CardSelectionMode, ModalChoice.selection: TargetSelectionMode. This is the parameterize-don't-proliferate path: a typed selection axis on the existing nodes rather than a sibling effect variant.
• Parser detection uses the sanctioned qualifier idiom. nom_primitives::scan_contains(lower, "at random") (the imperative.rs qualifier-flag precedent), not raw contains/split_once dispatch. The one .find(|t| matches!(t.mode, TriggerMode::DamageDone)) hit is an iterator predicate over a typed TriggerMode enum, not string dispatch — fine.
• Random resolution routes through the existing axis. Random selections are resolved up front via random_select_targets_for_ability rather than the interactive resolver, with is_random() guards (and debug_assert!s that a Random modal never reaches the interactive begin_pending_trigger_target_selection). Resolution is non-interactive and game-driven (state.rng), matching CR 700.2b / 608.2d (the game, not the controller, makes the choice).
• CR annotations verified against docs/MagicCompRules.txt: 608.2d (effect-offered choices, random selection), 700.2b (modal triggered-ability mode selection). Both check out.

To do before this can advance

1. Resolve the origin/main conflict (required — queue blocker).
2. After rebase, confirm the four cluster cards still parse as claimed (Cult of Skaro modal.selection=Random, Strax drop2 Choose.selection=Random with the WhenYouDo Fight sub intact, River Song's Diary ChooseFromZone.random=true) and that the random-resolution tests still discriminate (assert no interactive prompt is raised).

No blocking architecture findings — the only hard blocker is the merge conflict.

[matthewevans]

Maintainer follow-up — conflict resolved, a real regression caught & fixed, enqueuing

Merged origin/main and resolved the conflicts. CI-green was not sufficient here — the merge compiled and 12k tests passed, but the cluster-card re-verification you asked for surfaced a genuine regression, now fixed.

Conflicts resolved

• choose_from_zone.rs: kept both resolve_random_in_chain (this PR) and the EachPlayer per-player machinery (prompt_next_each_player + helpers, from main).
• trigger_matchers.rs: main independently added damage_recipient_filter_can_match_player (handles Or/And recursion) for the same purpose as this PR's is_object_only_damage_filter. Kept main's more complete authority and dropped the duplicate (it covers Strax's Typed([Creature]) → reject-player case).
• imperative.rs: threaded the new selection field through main's for each player ChooseFromZone promotion.

Regression caught + fixed. Main also landed Strax-class object-recipient scoping, as the guarded parse_object_recipient_filter (it declines "creature or player"/"creature or opponent" so the player leg survives). This PR's Fix B had added a second, unguarded object arm inside the player-axis parse_damage_to_qualifier_with_rest; post-merge it matched "a creature" in "a creature or player" and dropped the player leg — regressing the Crovax/Flesh Reaver class (creature_or_player_recipient_not_mis_scoped failed). Removed the redundant arm so the guarded parser is the single authority; Strax still scopes to Typed(Creature), and "creature or player" correctly yields valid_target=None.

Cluster-card re-verification (post-rebase, via oracle-gen):

• Cult of Skaro → modal.selection = Random ✓
• Strax → Choose.selection = Random, with the WhenYouDo Fight sub (ChosenPlayer-scoped target) intact ✓, and its DamageDone valid_target = Typed(Creature) ✓
• River Song's Diary → ChooseFromZone.random = true, CastFromZone sub intact ✓

Verification: fmt + parser gate clean · clippy -p engine --all-targets -D warnings clean · cargo test -p engine green (0 failed across 61 binaries).

[matthewevans]

Conflicts resolved; caught and fixed a real 'creature or player' regression from two PRs independently adding object-recipient scoping (kept main's guarded authority). Cluster cards re-verified; full engine suite green.

[matthewevans]

Re-reviewed at head 6008c65 after rebasing onto post-#3571 main. The earlier CI red was the #3571 ratchet false-positive — but with that fixed, a genuine, distinct regression is now surfaced that must be addressed before this can land. Converting to changes-requested.

[HIGH] Target-fallback regression on Tariel, Reckoner of Souls (CI-gating). The card-data coverage gate fails at this head:

REGRESSED (coverage honesty) — 1 cards: Tariel, Reckoner of Souls  [ParseWarning:target-fallback]
DIAGNOSTIC REGRESSION: target-fallback increased from 41 to 45 (+1 real, exceeds new-card allowance of +0)
FAIL: one or more diagnostic categories regressed.

This is not Marvel churn — Tariel is a long-supported baseline card (so #3571's $bsup guard does not exclude it), and the sibling PR #3509 (same data, same post-#3571 base) passes clean, so the delta is this PR's code, not the dataset.

Root cause is squarely in this PR's blast radius: Tariel's ability is "{2}{B}{B}{B}, {T}: Choose a creature card in a graveyard. Flip three coins…" and this PR introduces game/effects/choose_from_zone.rs (new) + game/effects/choose.rs (+196) + the oracle_effect/imperative.rs choose changes. The new choose-from-zone parsing path causes Tariel's "choose a creature card in a graveyard" to fall back to a generic target (the target-fallback warning) instead of the zone-scoped choose it parsed as on main. Net effect: Tariel's reanimation would resolve against the wrong subject.

Suggested fix: trace Tariel through the new choose_from_zone / imperative.rs choose path and ensure "choose a creature card in a graveyard" still resolves to the graveyard-scoped choose (not a target-fallback). Add a discriminating coverage/runtime test for Tariel so the gate stays green. Once target-fallback reads +0 again, re-request review and I'll re-enqueue — the rest of the PR was already approved.

Auto-merge disabled until this is resolved (it would never merge while CI is red, but disabling avoids a surprise merge if the gate is force-green'd).

[matthewevans]

Follow-up — target-fallback regression fixed & rebased

Addressed the Card data (coverage) CI failure (target-fallback +1 real) and brought the branch current with main.

Root cause. The at-random selection axis detected "at random" and set selection = Random, but left the qualifier in choice_text. For a pre-zone qualifier — "Choose a creature card at random from target opponent's graveyard" (Tariel, Reckoner of Souls) — "at random" landed in the search-filter prefix ("a creature card at random"), which parse_search_filter can't classify, emitting a spurious search-filter-suffix unmatched TargetFallback. Tariel parsed cleanly on baseline (as a plain TargetOnly), so this was a real regression; Deadbridge Chant and Higure regressed the same way.

Fix. Strip the captured " at random" qualifier from choice_text before the zone/filter parse (try_parse_choose_from_zone). The selection axis still records Random; all three cards now parse with zero warnings and selection: Random + the correct zone_owner (Tariel TargetedPlayer, Deadbridge/Higure Controller). Added a discriminating regression test that asserts no TargetFallback is emitted (fails pre-fix).

Verification (branch now current with main): clippy -p engine --all-targets -D warnings clean · cargo test -p engine green (0 failed across 61 binaries, incl. 21 choose/at-random tests + the new pin) · coverage gate EXIT 0 — target-fallback 43 and swallowed-clause 1077 both match baseline (the earlier swallowed-clause delta was baseline lag from being behind main, now resolved), 0 engine regressions, net +3 cards supported.

[matthewevans]

Target-fallback regression fixed: the at-random ChooseFromZone path now strips the captured 'at random' qualifier before the filter parse, so Tariel/Deadbridge/Higure parse clean. Branch current with main; coverage gate EXIT 0 (target-fallback + swallowed-clause both at baseline), full engine suite green, discriminating regression test added.