fix(parser): Fight/attacks-trigger target drops the 'creature defending player controls' scope, so t

[ntindle]

Summary

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

Root cause: Fight/attacks-trigger target drops the 'creature defending player controls' scope, so the Fight can target any creature instead of one the defending player controls.

Cards corrected

• Ace, Fearless Rebel

Fix

Fixed cluster #17: the "up to one" target optionality dropped on the Fight leg of Ace, Fearless Rebel and the whole "fights up to N target" class. The controller-scope half (DefendingPlayer) was already landed on this branch via 992c970; the only remaining defect was the lost target cardinality. Threaded the MultiTargetSpec that strip_optional_target_prefix already computes through TargetedImperativeAst::Fight (added Option field) and stamped it onto ParsedEffectClause.multi_target in lower_imperative_family_ast (where a clause is in scope), which lower.rs propagates to AbilityDefinition.multi_target with min=0 (CR 115.6 zero-targets-allowed). Deliberately did NOT add a field to Effect::Fight — optionality is an ability-level target-count axis whose single authority is game/ability_utils.rs:1587. Both construction sites (imperative.rs main path, mod.rs compound-splitter) now capture the spec. Added building-block tests across the optionality axis: combinator-level strip test (defending-player suffix variant), effect-lowering tests (up-to-one -> up_to(1), mandatory -> None via parse_effect_chain), and end-to-end trigger tests walking the then-sequence chain to the nested Fight sub-ability asserting BOTH multi_target==up_to(1) AND ControllerRef::DefendingPlayer, plus an Or-target orthogonality variant. Verification all green: cargo fmt clean, check-parser-combinators pass, targeted parser diff gate found zero string dispatch in added lines, clippy -p engine -D warnings exit 0, cargo test -p engine --lib 12027 passed 0 failed, cargo coverage and cargo semantic-audit exit 0 (Ace no longer flagged). Regenerated card-data.json confirms Ace's Fight leg now carries multi_target{min:0,max:Fixed(1)} with controller DefendingPlayer retained; Apex Altisaur (sibling) gained the same optionality; The Tarrasque (mandatory fight) correctly has no multi_target (no regression). 3 pre-existing db-load-path test failures (stale committed export still containing renamed FilterProp::AttackingAlone, unrelated to my diff) were resolved as a side effect of the planned card-data regeneration. No edits to controller-scope code (oracle_nom/filter.rs, oracle_target.rs, distribute_controller_to_or), no Effect::Fight field, no new up-to extractor, no MULTI_TARGET_VERBS change. Did not commit. known-tokens.toml shows modified as a gen-card-data.sh side-effect output (already dirty at session start, not hand-edited).

Files changed

• crates/engine/src/parser/oracle_ir/ast.rs
• crates/engine/src/parser/oracle_effect/imperative.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_trigger.rs
• client/public/card-data.json
• crates/engine/data/known-tokens.toml

CR references

• CR 115.6 (verified line 857: a spell or ability that requires targets may allow zero targets to be chosen) — authorizing rule for the new optional-target AST field, the lower_imperative_family_ast interception arm, and the optionality tests
• CR 701.14a (verified line 3378: Fight) — on the mandatory-fight no-optionality test
• CR 508.5 / 508.5a (verified lines 2317/2319: defending-player determination) — referenced in end-to-end test docs for the already-landed DefendingPlayer scope; no code change

Verification

• cd /Users/ntindle/code/random/magic/phase-main-base && cargo fmt --all — pass
• cd /Users/ntindle/code/random/magic/phase-main-base && ./scripts/check-parser-combinators.sh "$(git -C /Users/ntindle/code/random/magic/phase-main-base merge-base upstream/main HEAD)" — pass
• cd /Users/ntindle/code/random/magic/phase-main-base && cargo clippy -p engine --all-targets -- -D warnings — pass
• cd /Users/ntindle/code/random/magic/phase-main-base && cargo test -p engine — pass
• cd /Users/ntindle/code/random/magic/phase-main-base && cargo run --profile tool --features cli --bin oracle-gen -- data --filter "ace, fearless rebel" — pass
  Cards confirmed re-parsed correctly: ace, fearless rebel

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request implements support for 'up to one target' cardinality in 'fight' effects (CR 115.6) by preserving the optional-target specification (multi_target) through the AST and applying it at the clause layer (lower_imperative_family_ast). Unit tests have been added to verify both the optional and mandatory fight target behaviors. The review feedback suggests a performance improvement in lower_imperative_family_ast to avoid an unnecessary clone of multi_target by passing None to lower_targeted_action_ast, which deliberately ignores this field.

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.

[gemini-code-assist]

Performance Improvement: Avoid Unnecessary Clone

[MEDIUM] Avoid unnecessary clone of multi_target.

Why it matters: MultiTargetSpec and its nested QuantityExpr can be heap-allocated, so cloning it unnecessarily during AST lowering adds overhead. Since lower_targeted_action_ast ignores the multi_target field for Fight, we can pass None instead of cloning.

Suggested fix: Pass None to lower_targeted_action_ast and move multi_target directly into clause.multi_target.

Suggested change

	let mut clause =
	parsed_clause(lower_targeted_action_ast(TargetedImperativeAst::Fight {
	target,
	multi_target: multi_target.clone(),
	}));
	clause.multi_target = multi_target;
	let mut clause =
	parsed_clause(lower_targeted_action_ast(TargetedImperativeAst::Fight {
	target,
	multi_target: None,
	}));
	clause.multi_target = multi_target;

[matthewevans]

Approved after reviewing the updated head and the maintainer cleanup commit.

I re-diffed the actual post-update head against current origin/main. The remaining PR delta is limited to the fight-target parser/AST lowering path plus regression coverage. The change keeps the "up to one" cardinality at the clause/ability level (multi_target) instead of adding a field to Effect::Fight, and preserves the defending-player target scope through the attack-trigger sub-ability chain. I also addressed Gemini's open review note by avoiding the unnecessary multi_target clone when lowering the bare Effect::Fight.

Local verification passed: parser combinator gate, cargo fmt --all -- --check, diff check, focused fight parser tests, and the full repo pre-push hook (Rust clippy, parser tests, phase-ai tests, card-data validate/coverage, frontend lint/type-check; lint warnings only in pre-existing frontend fast-refresh files). GitHub CI is pending on the updated branch, so auto-merge can let the required checks gate landing.