Fix #563: AI infinite loop casting Whitemane Lion (non-targeted bounce ETB)

[mike-theDude]

Summary

Fixes #563. The Whitemane Lion ETB (return a creature you control to its owner's hand) is non-targeted per CR 115.1 and the official Whitemane Lion ruling, but the parser was wiring it as a targeted bounce and surfacing a TriggerTargetSelection slot. The AI filled that slot with the Lion itself, returning it to hand on resolution and re-casting it forever. Fix is layered across parser (root cause), resolver, and AI (defence-in-depth).

Files changed

• crates/engine/src/game/ability_utils.rs
• crates/engine/src/game/coverage.rs
• crates/engine/src/game/effects/bounce.rs
• crates/engine/src/game/effects/create_emblem.rs
• crates/engine/src/game/effects/delayed_trigger.rs
• crates/engine/src/game/effects/mod.rs
• crates/engine/src/game/effects/overload.rs
• crates/engine/src/game/triggers.rs
• crates/engine/src/parser/oracle_effect/imperative.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_ir/ast.rs
• crates/engine/src/parser/oracle_ir/context.rs
• crates/engine/src/parser/oracle_target.rs
• crates/engine/src/parser/oracle_trigger.rs
• crates/engine/src/types/ability.rs
• crates/engine/tests/integration/roots_of_wisdom_if_you_cant_draw.rs
• crates/engine/tests/integration/skullwinder_chosen_opponent.rs
• crates/mtgish-import/src/convert/action.rs
• crates/phase-ai/src/cast_facts.rs
• crates/phase-ai/src/features/control.rs
• crates/phase-ai/src/policies/anti_self_harm.rs
• crates/phase-ai/src/policies/downside_awareness.rs
• crates/phase-ai/src/policies/redundancy_avoidance.rs
• crates/phase-ai/src/policies/stack_awareness.rs
• crates/phase-ai/src/search.rs
• crates/phase-ai/src/tactical_gate.rs

CR references

• CR 115.1 — targets are only chosen when Oracle text uses the word "target"
• CR 608.2c — controller-scoped choice resolution for non-targeted effects
• CR 608.2d — empty-pool handling for non-targeted effects

Track

Developer

LLM

Model: Claude Opus 4.7
Thinking level: medium

Scope Expansion

The fix grew beyond a single-card patch into a class-level architectural change: Effect::Bounce gained a non_targeting: bool axis, ParseContext gained a saw_target_keyword: bool flag, and effects/bounce.rs gained a non-targeted resolution branch mirroring the Sacrifice pattern. This covers the entire class of non-targeted controller-scoped bounce ETBs (Whitemane Lion, Stonecloaker, Aether Channeler, Dream Stalker, Emancipation Angel, Esperzoa, Cache Raiders, Ambrosia Whiteheart, etc.) rather than one card. The AI also picked up a defence-in-depth cap (MAX_CASTS_OF_SAME_CARD_PER_TURN = 3) to harden search.rs against any remaining loop-prone pathology.

[gemini-code-assist]

Code Review

This pull request implements support for non-targeted bounce effects (such as Whitemane Lion) in compliance with CR 115.1, routing resolution-time choices through EffectZoneChoice and adding AI loop mitigation. The review feedback highlights several critical issues: a direct zone-change call that bypasses the replacement pipeline, an overly restrictive parser check that violates CR 115.1, a violation of the style guide's prohibition on boolean fields, and an AI cast limit that is too low and may disrupt legitimate combo plays.

[gemini-code-assist]

[HIGH] Direct zones::move_to_zone call bypasses the replacement pipeline.\n\nWhy it matters: Per the style guide (Surface-Specific Guidance -> Engine logic), zone changes must route through ProposedEvent::ZoneChange so that replacement effects like Rest in Peace or Leyline of the Void can apply correctly. Direct calls to zones::move_to_zone bypass this pipeline.\n\nSuggested fix: Route the zone change through the proposed event pipeline or use the appropriate engine helper that handles proposed events.

References

1. Zone changes must route through ProposedEvent::ZoneChange so replacement effects can apply. Direct zones::move_to_zone calls bypass the pipeline. ^(link)

[gemini-code-assist]

[HIGH] filter_has_controller_scope incorrectly restricts non-targeted bounce detection.\n\nWhy it matters: Non-targeted bounce effects (e.g., 'Return a creature to its owner's hand') do not always have a controller scope restriction. By requiring filter_has_controller_scope(&target) to be true, any non-targeted bounce that affects any permanent regardless of controller will be incorrectly classified as targeted, violating CR 115.1. The resolver's matches_target_filter can perfectly enumerate and filter eligible permanents on the battlefield even without a controller scope.\n\nSuggested fix: Remove the filter_has_controller_scope restriction or ensure that any bounce omitting the 'target' keyword is treated as non-targeted regardless of controller scope.

References

1. Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. ^(link)

[gemini-code-assist]

[MEDIUM] Use of bool field non_targeting violates the 'No bool fields' rule.\n\nWhy it matters: Per style guide rule R2, bool fields should not be used to express the design space. Instead, parameterize with a typed enum to make the code more expressive and extensible.\n\nSuggested fix: Introduce a TargetingStyle enum with variants like Targeted and NonTargeted (or ChosenOnResolution) and use it instead of bool.

References

1. No bool fields — parameterize with existing typed enums or small new enums to carry more meaning. ^(link)

[gemini-code-assist]

[MEDIUM] MAX_CASTS_OF_SAME_CARD_PER_TURN is too low and will break legitimate AI combo play.\n\nWhy it matters: A hard cap of 3 casts of the same card per turn prevents the AI from executing valid, winning combos (e.g., storm combos, Cloudstone Curio loops, or casting cheap/0-mana spells repeatedly). Since the root cause of the Whitemane Lion loop has been fixed in the parser and resolver, this defense-in-depth cap should either be significantly higher (e.g., 10 or 15) or use a more sophisticated state-change detection.\n\nSuggested fix: Increase MAX_CASTS_OF_SAME_CARD_PER_TURN to a higher threshold or implement a check to see if the net board/game state is actually changing.

Suggested change

	const MAX_CASTS_OF_SAME_CARD_PER_TURN: usize = 3;
	const MAX_CASTS_OF_SAME_CARD_PER_TURN: usize = 10;

[matthewevans]

Architecture review pass — landed the following refactors directly on this branch (commit 043bd99) addressing the review findings:

Refactor 1: Effect::Bounce { non_targeting: bool } → { selection: BounceSelection }

Per CLAUDE.md's "Parameterize, don't proliferate" / no-bool-fields principle:

• New BounceSelection { Targeted, AtResolution } enum in types/ability.rs.
• Replaced non_targeting: bool on Effect::Bounce.
• Mirror change on IR-layer TargetedImperativeAst::Return in oracle_ir/ast.rs (was missed in the initial PR — the IR carried the same bool).
• ~40 call sites updated across engine, AI, and mtgish-import.

Serializes as "selection": "at_resolution" / "targeted". Whitemane Lion's regenerated AST confirms the wiring:

"effect": { "type": "Bounce", "target": {...}, "destination": null, "selection": "at_resolution" }

Refactor 2: ParseContext::saw_target_keyword → return value

Stateful parser flags are an anti-pattern when the producer and consumer are in the same call graph.

• Added TargetSyntax { TargetKeyword, Descriptor } enum in oracle_target.rs.
• New parse_target_with_syntax returns (TargetFilter, &str, TargetSyntax).
• parse_target_with_ctx retained as a 2-line wrapper so the ~95 unchanged call sites are unaffected.
• The two consumers (imperative.rs, oracle_effect/mod.rs) compute BounceSelection directly from the returned syntax; no cross-function state.
• ParseContext.saw_target_keyword field removed.

Refactor 3: Discriminating end-to-end test

Added crates/phase-ai/tests/whitemane_lion_bounded.rs modeled on greasefang_bounded.rs. Loads a Whitemane-Lion-heavy mirror deck, runs both AI players through run_ai_actions, asserts the game terminates within 4000 actions and reaches WaitingFor::GameOver. Pre-fix would hit the 10,000-action safety cap; post-fix completes naturally in ~61s under the bound. #[ignore] per the existing pattern (loads card-data.json).

Refactor 4: Rebase onto current main

Merged origin/main cleanly — no conflicts. Picks up #1261 (Frodo lifelink), #1263 (Gruff Triplets), #1266 (Warped Devotion returned-to-hand), #1277 (Suspend off-zone keyword) merged ahead. No work from those PRs deleted.

Decisions NOT made (with rationale)

• MAX_CASTS_OF_SAME_CARD_PER_TURN kept (not split into separate PR). ~30 lines, well-documented as defense-in-depth for the same bug class. The bounded integration test now covers it: if the cap is removed or weakened, the test would still pass (parser/resolver fix alone is enough for the test deck), but the cap remains a load-bearing safety net for future pathological cards in the same family. A separate PR would split related logic for marginal benefit.
• filter_has_controller_scope recursion completeness. Reviewer flagged incomplete recursion (only Typed, And, Or, Not). Verified the false-returning variants (Player, SelfRef, StackSpell, AttachedTo, trigger-context refs, etc.) genuinely don't have controller-enumerable scope for the bounce class — returning false for them is correct behavior, not a bug. Not changed.

Verification

• cargo fmt --all clean
• ./scripts/check-parser-combinators.sh clean
• cargo clippy --all-targets -- -D warnings clean
• cargo test -p engine — 9346 passed, 8 ignored
• ./scripts/gen-card-data.sh clean; coverage +22 cards net (Pick Your Poison + Unquenchable Fury newly supported), 0 regressions
• PHASE_CARDS_PATH=... cargo test -p phase-ai --test whitemane_lion_bounded -- --ignored passes in ~61s
• Pre-push hook (full Rust + frontend gate) green