fix(parser): thread optional flag for "you may choose N" triggered modals (#5640)

[andriypolanski]

Closes #5640

Summary

Shadrix Silverquill's begin-combat trigger (you may choose two) was parsed as a mandatory modal because the TriggeredModal block path split the header off the condition but never stamped optionality on the trigger or execute ability. The count spec correctly stays (2, 2) — declining means skipping the entire ability, not choosing zero modes.

This fix adds an optional_trigger flag to ModalHeaderAst for the "you may choose N" class (distinct from "you may choose up to N", which only lowers min_choices). TriggeredModal lowering now sets both TriggerDefinition.optional and execute.optional. A companion fix propagates modifiers.optional onto PreLowered trigger bodies (inline modals that already detected leading you may).

Files changed

File	Change
crates/engine/src/parser/oracle_ir/ast.rs	ModalHeaderAst.optional_trigger field
crates/engine/src/parser/oracle_modal.rs	Detect flag in parse_modal_header_ast; stamp optional on TriggeredModal lowering; parser tests
crates/engine/src/parser/oracle_trigger.rs	Propagate modifiers.optional on PreLowered bodies
crates/engine/src/parser/oracle_tests.rs	Assert Shadrix trigger + execute optional; DifferentTargetPlayers unchanged

Test plan

• parse_modal_header_you_may_choose_fixed_count_sets_optional_trigger
• parse_modal_header_you_may_choose_up_to_does_not_set_optional_trigger
• triggered_modal_header_supports_you_may_choose_and_constraints (optional + constraints)
• Runtime: begin-combat trigger offers decline; accepting still requires exactly two modes
• DifferentTargetPlayers constraint enforced at mode selection

Issue

Closes #5640

[gemini-code-assist]

Code Review

This pull request implements support for CR 608.2c by identifying triggered modal headers of the form 'you may choose N' as optional triggers, propagating this optionality during lowering, and adding corresponding tests. The review feedback highlights a violation of Rule R2 (No bool fields) across several files, recommending the replacement of the optional_trigger boolean field with a typed enum ModalOptionality to better express the design space.

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.

[matthewevans]

Changes requested — the optional-trigger design is plausible, but the current head does not compile and has no runtime proof.

🔴 Blocker

• crates/engine/src/parser/oracle_modal.rs:722-724 leaves the second nom::tag error type unconstrained; every Rust CI consumer reports E0283 there. Use the same explicit OracleError type as the preceding combinator (or an existing typed helper) so the parser is compilable.
• crates/engine/src/parser/oracle_trigger.rs:1387 pattern-matches TriggerBody::PreLowered(mut ability) through &ir.body, moving Box<AbilityDefinition> out of a shared reference (E0507). Borrow/clone the boxed body before stamping optionality, preserving lower_trigger_ir's non-consuming contract.
• The PR body leaves the Shadrix runtime decline/accept behavior unchecked. Parser-shape assertions cannot prove that a real begin-combat trigger offers a decline and still enforces exactly two modes on acceptance; add a registered scenario regression for both branches.

🟡 Non-blocking

• None.

✅ Clean

• The proposed distinction between you may choose N and you may choose up to N correctly preserves the fixed-count modal shape in the parser tests.

Recommendation: repair the two compile errors and add the discriminating triggered-modal runtime regression, then request re-review.

[github-actions]

Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main c7275f4e8bed)

1 card(s) · trigger/Phase · field optional: ∅ → yes

Examples: Shadrix Silverquill

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

Changes requested

🔴 Blocker

• Current head 44115cdeb4a1bd49a7a501e51699c5946f674463 fails CI. cargo fmt --check reports formatting drift in oracle_modal.rs and issue_5640_shadrix_optional_triggered_modal.rs; the integration test also fails to compile because lines 100 and 204 pass private runner.state instead of runner.state() to drain_order_triggers_with_identity.

🟡 Non-blocking

• The prior request for a discriminating runtime proof remains relevant after the build is fixed: exercise the Shadrix decline and accept paths, including the exactly-two-mode constraint.

✅ Clean

• The previous inference/type and borrowed-body compiler errors are absent from this current CI result.

Recommendation: fix the current CI blockers, add the runtime proof, and request re-review.

[matthewevans]

Changes requested — the new runtime regression proves optionality is applied after modal selection has already been required.

🔴 Blocker

• crates/engine/src/game/triggers.rs:4964-5057 builds the trigger's modal choice and pauses for AbilityModeChoice before the triggered execute ability can reach its optional resolution path. The current regression proves the consequence: crates/engine/tests/integration/issue_5640_shadrix_optional_triggered_modal.rs:111-123 tries to decline, immediately receives AbilityModeChoice, and CI fails with Must choose at least 2 modes, got 0. Shadrix's verified Oracle text is: “At the beginning of combat on your turn, you may choose two. Each mode must target a different player.” The controller must be offered the ability-level decline before the fixed two-mode chooser; stamping optional at oracle_modal.rs:1043-1079 is too late. Route that choice through the pending-trigger/stack construction path first, then enter modal_choice_for_player only after acceptance.

• crates/engine/src/parser/oracle_ir/ast.rs:1803 introduces optional_trigger: bool for a distinct modal semantic axis. Keep the triggered-ability optionality typed at the same boundary that owns the routing decision (with explicit mandatory vs may-decline variants); the suggested ModalOptionality name does not exist in the current tree, so introducing a well-scoped typed representation is preferable to adding another bool and only comparing it differently.

🟡 Non-blocking

• CI also reports rustfmt drift in crates/engine/src/parser/oracle_modal.rs:2158-2164; resolve it with the behavioral fix.

✅ Clean

• issue_5640_shadrix_optional_triggered_modal.rs is registered and, unlike the earlier AST-only coverage, reaches the real begin-combat trigger and precisely exposes the ordering failure.
• The current parse artifact is scoped to the intended Shadrix optional field change.

Recommendation: redesign the optional-trigger handoff so decline precedes AbilityModeChoice, replace the new boolean with the typed routing state, then retain this regression plus an accepted exactly-two-modes path.

[matthewevans]

Verdict — the current head is correct at the pending-trigger construction seam and is ready for the queue.

🔴 Blocker

• None.

🟡 Non-blocking

• None.

✅ Clean

• ModalOptionality::{Mandatory, MayDecline} replaces the boolean routing state; all constructors and lowering sites use the typed boundary.
• Decline now precedes AbilityModeChoice, while acceptance preserves the fixed two-mode requirement and the different-target-player constraint.
• The registered Shadrix scenario drives the real begin-combat path for decline, invalid one-mode acceptance, and successful two-mode resolution; it would fail if the pre-modal decision route were removed.
• Current-head CI is green, the parse diff is limited to Shadrix's trigger optionality, and all seven resolved Gemini threads were verified against this head.

Recommendation: approve and enqueue.