Fix Frodo lifelink animation parsing - #1261
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the animation parser in crates/engine/src/parser/oracle_effect/animation.rs to support parsing trailing keywords after a base power and toughness clause (e.g., "with base power and toughness 2/3 and lifelink"). It modifies split_animation_base_pt_clause to return parsed keywords and introduces helper functions to extract them, along with a corresponding unit test. The review feedback points out that the implementation relies on manual string parsing and splitting instead of idiomatic nom-based combinators, violating Rule R1. Additionally, the new rules-touching engine code lacks mandatory CR annotations as required by Rule R6.
| } | ||
|
|
||
| fn split_animation_base_pt_clause(text: &str) -> Option<(&str, i32, i32)> { | ||
| fn split_animation_base_pt_clause(text: &str) -> Option<(&str, i32, i32, Vec<Keyword>)> { |
There was a problem hiding this comment.
Avoid Manual String Parsing & Add CR Annotations
Avoid manual string splitting or verbatim string equality for parsing Oracle phrases, as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.
Additionally, ensure that this rules-touching engine code carries the appropriate CR annotations (e.g., CR 613.1d and CR 613.1f) per Rule R6 of the repository style guide.
| fn split_animation_base_pt_clause(text: &str) -> Option<(&str, i32, i32, Vec<Keyword>)> { | |
| fn animation_base_pt_parser(input: &str) -> IResult<&str, (i32, i32, Vec<Keyword>)> { |
References
- Rule R6: Every rules-touching line of engine code must carry a comment of the form
CR <number>: <description>. (link) - Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
a59a3a7 to
119f180
Compare
…um + drop ParseContext flag Addresses architecture review on phase-rs#1268: the original PR introduced two new bool fields (`Effect::Bounce.non_targeting`, `ParseContext.saw_target_keyword`) which violate the codebase's "no bool fields — parameterize with typed enums" principle (CLAUDE.md, feedback_no_bool_flags.md). This commit: 1. Adds `BounceSelection { Targeted, AtResolution }` in `types/ability.rs`. Replaces `non_targeting: bool` on `Effect::Bounce` with `selection`. Mirror change on the IR-layer `TargetedImperativeAst::Return` variant in `oracle_ir/ast.rs`. ~40 construction/destructure sites updated. 2. Drops `ParseContext::saw_target_keyword` and adds `parse_target_with_syntax` returning `TargetSyntax { TargetKeyword, Descriptor }` as a return-value discriminator. `parse_target_with_ctx` is kept as a 2-line wrapper so the ~95 unchanged call sites are unaffected. The two consumers (`oracle_effect/imperative.rs`, `oracle_effect/mod.rs`) compute `BounceSelection` directly from the returned syntax. 3. Adds `crates/phase-ai/tests/whitemane_lion_bounded.rs` — discriminating end-to-end integration test that loads a Whitemane-Lion-heavy deck mirror into the AI search engine and asserts the game terminates within 4000 actions (pre-fix would hit 10,000-action safety cap). `#[ignore]` per the existing `greasefang_bounded.rs` pattern (loads card-data.json). 4. Merges `origin/main` into the PR branch, picking up phase-rs#1261/phase-rs#1263/phase-rs#1266/phase-rs#1277 without conflict. Reverts an unrelated `known-tokens.toml` regeneration artifact (author's PR description explicitly excluded this file). 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; Whitemane Lion exports `"selection": "at_resolution"` correctly - `cargo test -p phase-ai --test whitemane_lion_bounded -- --ignored` passes in ~61s (game completes naturally, well under 4000-action bound)
…e ETB) (#1268) * Fix #563: AI infinite loop casting Whitemane Lion (non-targeted bounce ETB) The Whitemane Lion ETB ("return a creature you control to its owner's hand") is non-targeted per CR 115.1 and the 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 spans three layers: - Parser (root cause): add `saw_target_keyword: bool` to ParseContext and `non_targeting: bool` to Effect::Bounce so "return a creature you control" produces non_targeting=true, distinguished from "return target creature". - Resolver: extract_target_filter_from_effect carves out Bounce { non_targeting: true } so no target slot is opened. effects/bounce.rs adds a non-targeted branch that surfaces WaitingFor::EffectZoneChoice for the controller, mirroring the existing Sacrifice pattern (CR 608.2c/d). - AI (defence-in-depth): cast_facts.rs no longer requires targets for non-targeting bounces; search.rs caps same-card casts per turn at MAX_CASTS_OF_SAME_CARD_PER_TURN = 3 to prevent any remaining loop-prone pathology. Covers the class of non-targeted controller-scoped bounce ETBs: Whitemane Lion, Stonecloaker, Aether Channeler, Dream Stalker, Emancipation Angel, Esperzoa, Cache Raiders, Ambrosia Whiteheart, etc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * refactor(PR-1268): replace non_targeting bool with BounceSelection enum + drop ParseContext flag Addresses architecture review on #1268: the original PR introduced two new bool fields (`Effect::Bounce.non_targeting`, `ParseContext.saw_target_keyword`) which violate the codebase's "no bool fields — parameterize with typed enums" principle (CLAUDE.md, feedback_no_bool_flags.md). This commit: 1. Adds `BounceSelection { Targeted, AtResolution }` in `types/ability.rs`. Replaces `non_targeting: bool` on `Effect::Bounce` with `selection`. Mirror change on the IR-layer `TargetedImperativeAst::Return` variant in `oracle_ir/ast.rs`. ~40 construction/destructure sites updated. 2. Drops `ParseContext::saw_target_keyword` and adds `parse_target_with_syntax` returning `TargetSyntax { TargetKeyword, Descriptor }` as a return-value discriminator. `parse_target_with_ctx` is kept as a 2-line wrapper so the ~95 unchanged call sites are unaffected. The two consumers (`oracle_effect/imperative.rs`, `oracle_effect/mod.rs`) compute `BounceSelection` directly from the returned syntax. 3. Adds `crates/phase-ai/tests/whitemane_lion_bounded.rs` — discriminating end-to-end integration test that loads a Whitemane-Lion-heavy deck mirror into the AI search engine and asserts the game terminates within 4000 actions (pre-fix would hit 10,000-action safety cap). `#[ignore]` per the existing `greasefang_bounded.rs` pattern (loads card-data.json). 4. Merges `origin/main` into the PR branch, picking up #1261/#1263/#1266/#1277 without conflict. Reverts an unrelated `known-tokens.toml` regeneration artifact (author's PR description explicitly excluded this file). 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; Whitemane Lion exports `"selection": "at_resolution"` correctly - `cargo test -p phase-ai --test whitemane_lion_bounded -- --ignored` passes in ~61s (game completes naturally, well under 4000-action bound) --------- Co-authored-by: Michael Briningstool <mbriningstool@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
Summary
Fixes Frodo, Sauron's Bane so its first activated ability parses the trailing
lifelinkgrant in:The animation parser now preserves keyword text that appears after a fixed
with base power and toughness N/Nclause, so the exported continuous effect includesAddKeyword(Lifelink).Fixes #952.
Files changed
crates/engine/src/parser/oracle_effect/animation.rscrates/engine/src/parser/oracle_effect/mod.rsCR references
Track
Developer
Model: codex-5
Thinking: medium
Tier: Standard
Anchored on
crates/engine/src/parser/oracle_effect/animation.rs:661- existing animation keyword-tail extraction forwith <keyword>clauses.crates/engine/src/parser/oracle_effect/token.rs:940- existing shared keyword-list splitting andmap_token_keyworddelegation.Gate A
./scripts/check-parser-combinators.shOutput:
Verification
cargo fmt --allcargo fmt --all -- --check./scripts/check-parser-combinators.shcargo test -p engine frodo_source_condition_preserves_lifelink_grantcargo test -p engine animation_base_pt_preserves_trailing_bare_keywordcargo clippy --all-targets -- -D warningscargo test -p engine./scripts/gen-card-data.shNotes
./scripts/gen-card-data.shconfirmed the regenerated Frodo export includesAddKeyword(Lifelink).