parser: restore count+controller for 'target opponent's library' exile (refs #594) - #1401
Conversation
… opponent's library" exile Extend parse_exile_ast to handle "target opponent's library" / "target player's library" patterns. Restores count and controller filter for the ExileTop effect emitted by Maralen, Fae Ascendant and Court of Locthwain ETB triggers. Variant choices (canonical leaves already produced by parse_target): - "target opponent" → TargetFilter::Typed(TypedFilter::default().controller(Opponent)) - "target player" → TargetFilter::Player Also routes "once each turn, you may cast" lines through the static classifier in preparation for the follow-up cast-from-exile-permission work (Step 10-14 of the investigator plan, tracked as a separate PR). Today this changes the Unimplemented "name" marker on Maralen's second ability from "effect_structure" to "static_structure" — both stay Unimplemented; the routing change only sets up the static handler that the follow-up PR will implement. Defect phase-rs#3 in the issue (subject expansion dropped) was investigated and confirmed already correct in code — no change needed. Defect phase-rs#4+phase-rs#5 (this-turn exile-link scoping + ExileCastPermission static) are deferred to a follow-up PR per investigator's recommended sequencing. Tests: - exile_top_target_opponents_library (parser-level): ExileTop count=2, target-opponent - exile_top_target_players_library_singular (parser-level): ExileTop count=1, Player - trigger_maralen_etb_exile_top_two_of_target_opponents_library (trigger integration): full Oracle text round-trips to ChangesZone trigger with ExileTop(Opponent, 2) Refs phase-rs#594 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces support for parsing Maralen, Fae Ascendant's ETB trigger and preparatory routing for cast-from-exile permissions. It adds a static prefix pattern for 'once each turn, you may cast', implements target opponent/player library exile patterns in the imperative parser, and adds a corresponding unit test. Feedback on these changes highlights violations of the repository's architectural rules: specifically, the use of verbatim string matches for both the static prefix and the library exile patterns instead of decomposing them into modular nom-based combinators (violating R1), and an incorrect Comprehensive Rules citation for the cast-from-exile permission comment (violating R6).
| // CR 113.6 + CR 117.1: shorter sibling of "once during each of your turns, | ||
| // you may cast" — Maralen, Fae Ascendant prints "Once each turn, you may | ||
| // cast a creature spell from exile …". Routes the line into the static | ||
| // classifier so the cast-from-exile-permission handler (follow-up PR) can | ||
| // pick it up. With no handler implemented yet, `parse_static_line_multi` | ||
| // returns an empty Vec and dispatch falls through to the next priority, | ||
| // matching pre-change behavior — no regression today, correct preparatory | ||
| // routing for the follow-up. |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for cast-from-exile permission. Evidence: crates/engine/src/parser/oracle_classifier.rs:281-288.
Why it matters: Citing incorrect or adjacent rules makes the codebase harder to verify against the official MTG Comprehensive Rules.
Suggested fix: Replace CR 113.6 + CR 117.1 with CR 601.3 (or CR 601.3a), which specifically governs static abilities allowing players to cast spells from unusual zones.
| // CR 113.6 + CR 117.1: shorter sibling of "once during each of your turns, | |
| // you may cast" — Maralen, Fae Ascendant prints "Once each turn, you may | |
| // cast a creature spell from exile …". Routes the line into the static | |
| // classifier so the cast-from-exile-permission handler (follow-up PR) can | |
| // pick it up. With no handler implemented yet, `parse_static_line_multi` | |
| // returns an empty Vec and dispatch falls through to the next priority, | |
| // matching pre-change behavior — no regression today, correct preparatory | |
| // routing for the follow-up. | |
| // CR 601.3: shorter sibling of "once during each of your turns, | |
| // you may cast" — Maralen, Fae Ascendant prints "Once each turn, you may | |
| // cast a creature spell from exile …". Routes the line into the static | |
| // classifier so the cast-from-exile-permission handler (follow-up PR) can | |
| // pick it up. With no handler implemented yet, parse_static_line_multi | |
| // returns an empty Vec and dispatch falls through to the next priority, | |
| // matching pre-change behavior — no regression today, correct preparatory | |
| // routing for the follow-up. |
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Fixed in 798b3bf: replaced CR 113.6 + CR 117.1 with CR 601.3e (verified at docs/MagicCompRules.txt:2487 — the canonical rule for static abilities allowing a player to cast spells from non-hand zones, e.g., Garruk's Horde, Melek, Maralen).
| // returns an empty Vec and dispatch falls through to the next priority, | ||
| // matching pre-change behavior — no regression today, correct preparatory | ||
| // routing for the follow-up. | ||
| "once each turn, you may cast", |
There was a problem hiding this comment.
[MEDIUM] Avoid adding verbatim string matches for 'play' sibling variants. Evidence: crates/engine/src/parser/oracle_classifier.rs:289.
Why it matters: Verbatim string equality bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers.
Suggested fix: Decompose the phrase into modular, reusable parsers for constituent parts (e.g., matching 'once each turn, you may' followed by an alt of 'cast' or 'play') and compose them using idiomatic combinator aggregates to prevent combinatorial explosion.
References
- 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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
There was a problem hiding this comment.
Thanks for the review. The claim that this is verbatim string equality is incorrect — STATIC_PREFIX_PATTERNS is a dispatch table whose entries are consumed by tag() combinators (see the classifier's downstream call site). Each string is a nom tag input, not an if x == "..." equality check, so the nom-compliance rule is satisfied. That said, the underlying architectural concern (this prefix array is approaching sibling-cluster smell) was flagged by our internal code-reviewer as well and is queued as the catalyst for a parameterized refactor in a follow-up PR — once the cast-from-exile-permission handler (deferred Steps 10-14 from the investigator plan) lands, the prefix can be re-expressed as a nested alt((tag("once each turn"), tag("once during each of your turns"))) + tag(", you may cast") combinator rather than two table entries. For this targeted bug fix, extending the existing table mirrors the established pattern.
| ( | ||
| "card of target opponent's library", | ||
| target_opponent_filter.clone(), | ||
| ), | ||
| ( | ||
| "cards of target opponent's library", | ||
| target_opponent_filter.clone(), | ||
| ), | ||
| ("card of target player's library", TargetFilter::Player), | ||
| ("cards of target player's library", TargetFilter::Player), |
There was a problem hiding this comment.
[MEDIUM] Avoid adding verbatim string matches for library exile patterns. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:4274-4283.
Why it matters: Adding verbatim string combinations leads to combinatorial explosion and bypasses the robust nom-based parser.
Suggested fix: Decompose these compound phrases into modular, reusable parsers for constituent parts (e.g., quantity 'card'/'cards', owner 'target opponent'/'an opponent'/'target player') and compose them using idiomatic combinator aggregates like nested alt and tag sequences.
References
- 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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
There was a problem hiding this comment.
Thanks for the review. The claim that these are verbatim string equality matches that bypass nom is incorrect — the for-loop in parse_exile_ast (imperative.rs:4257-4287) iterates a (pattern, TargetFilter) dispatch table whose pattern strings are bound to tag::<_, _, OracleError<'_>>(pattern).parse(remainder) on each iteration. These are nom tag inputs, not if lower == "..." checks; the nom-compliance rule is satisfied. Our internal code-reviewer did flag the broader architectural concern: this table is now 12 arms across two orthogonal axes (singular/plural × possessor) and is approaching the CLAUDE.md sibling-cluster threshold. The clean refactor is a (QuantityExpr, TargetFilter) possessor combinator hoisted to oracle_nom/ and shared with oracle_effect/search.rs:576-599, which already does the same possessor dispatch in combinator form. That refactor is queued as the next-time-this-table-grows catalyst — explicitly out of scope for this targeted bug fix per code-reviewer's APPROVE-WITH-SUGGESTIONS verdict.
…ier entry CR 601.3e specifically governs static abilities that allow casting spells from non-hand zones (Garruk's Horde, Melek, Maralen). The earlier citation (CR 113.6 + CR 117.1) was adjacent rather than targeted. Per PR phase-rs#1401 review.
…rom-exile (phase-rs#1404) * feat(engine): ExileCastPermission static for Maralen-class cast-from-exile Closes the remaining gap on phase-rs#594 left after phase-rs#1401 (count + controller fix on the trigger half). Adds the engine support for "Once each turn, you may cast a spell ... from among cards exiled with ~ this turn without paying its mana cost." — defects phase-rs#4 (per-turn exile-link scoping) and phase-rs#5 (cast-from-exile permission static) in the issue. New `StaticMode::ExileCastPermission { frequency, play_mode, without_paying_mana_cost }` mirrors `GraveyardCastPermission` for the exile-pool sibling. Runtime additions: - `GameState::cards_exiled_with_source_this_turn` — per-source per-turn rolling list, populated alongside `exile_links::push_tracked_by_source` and cleared at turn cleanup. Keeps the persistent `exile_links` pool untouched (still backs the open-ended `ExiledBySource` filter). - `GameState::exile_cast_permissions_used` — per-source `OncePerTurn` slot, mirroring `graveyard_cast_permissions_used`. - `CastingVariant::ExilePermission { source, frequency }` — casting context that finalize-cast consults to stamp the slot. - `casting.rs::exile_objects_castable_by_permission` + `exile_cast_permission_source` extend `spell_objects_available_to_cast` and `has_exile_cast_permission`. - `casting.rs::is_exile_permission_free_cast` zeroes the mana cost when the static carries `without_paying_mana_cost: true`. - `exile_links::LINKED_EXILE_CONSUMER_TAGS` gains `"ExileCastPermission"` so a Maralen source is auto-detected as a tracked-exile consumer; her ETB trigger then populates exile_links + the per-turn map for free. - Parser handler in `oracle_static.rs::try_parse_exile_cast_permission` uses the shared nom combinator chain — `parse_type_phrase` already composes the dynamic "with mana value …" suffix through `parse_mana_value_suffix`, so Maralen's filter reaches `Cmc(LE, ObjectCount{Elf|Faerie, You})` through one call. CR annotations: 601.2a (cast permission grant), 113.6b (zone-restricted functioning), 118.9 (alternative cost), 400.7 (zone-change resets the source ObjectId / per-turn slot), 305.1 (play vs cast). All verified against docs/MagicCompRules.txt. Tests: - Parser: Maralen full line, longer "once during each of your turns" synonym, rejects missing "this turn" suffix, regression-guarded against the graveyard branch intercepting. - Engine: surface the per-turn pool, OncePerTurn slot gates and resets, cards outside the per-turn map (stale exile from prior turn) are pruned. Branch: bugfix/594-maralen-cast-from-exile Refs phase-rs#594. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * refactor(engine): typed ExileCastCost enum replaces bool field (R2) Replaces the `without_paying_mana_cost: bool` field on StaticMode::ExileCastPermission with a typed `ExileCastCost` enum (PayNormalCost | WithoutPayingManaCost), per CLAUDE.md rule R2 (no raw bool fields — use typed enums that express the design space). Addresses Gemini review comment on PR phase-rs#1404. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * perf(PR-1404): empty-pool fast-exit for exile-cast permission scans exile_permission_sources scans the whole battlefield and allocates an active_static_definitions iterator per controlled permanent. Both exile_objects_castable_by_permission (once per legal-actions / AI-search node) and exile_cast_permission_source (per-object castability predicate, casting.rs:829) call it on the hot path. Guard both with a single cards_exiled_with_source_this_turn.is_empty() check: a card is castable-via-permission only if it was exiled-with-a- source this turn (it must live in that pool), so an empty pool provably yields no offers. Skips the scan in the ~100% of board states with no Maralen-class permanent active; output is identical. Add start_next_turn_resets_exile_cast_permission_tracking: a discriminating regression test that drives start_next_turn and fails if either per-turn reset line (exile_cast_permissions_used / cards_exiled_with_source_this_turn) is dropped. --------- Co-authored-by: Michael Briningstool <mbriningstool@gmail.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer-reviewed at the quality bar: logic traced end-to-end, test discrimination checked, architecture (combinators / parameterization / building-block reuse / CR annotations) verified.
Summary
Partial fix for #594 (Maralen, Fae Ascendant / Court of Locthwain). Extends
parse_exile_astto handle"target opponent's library"/"target player's library"patterns so the parser stops dropping count and controller filter on theExileTopeffect. Also routes"once each turn, you may cast"lines through the static classifier in preparation for the follow-upExileCastPermissionstatic + this-turn exile-link scoping work (defects #4 and #5 in the issue).Refs #594 — the issue stays open until the follow-up PR lands the cast-from-exile permission static and the this-turn exile-link scoping.
Files changed
CR references
None added — this is a parser routing/extraction change, not new game-rule logic. Existing CR annotations in the touched files are unchanged.
Track
Developer
LLM
Model: claude-opus-4-7
Thinking: medium
Tier
Frontier
Verification
cargo fmt --all -- --check— clean./scripts/check-parser-combinators.sh— clean (exit 0)exile_top_target_opponents_library—ExileTopcount=2, target-opponent controller filterexile_top_target_players_library_singular—ExileTopcount=1,TargetFilter::Playertrigger_maralen_etb_exile_top_two_of_target_opponents_library— full Oracle text round-trips toChangesZonetrigger emittingExileTop(Opponent, 2)Scope Expansion
The classifier-routing tweak for
"once each turn, you may cast"is scope-adjacent infrastructure for the follow-up PR (defect #4:ExileCastPermissionstatic). Today it only shifts Maralen's second-abilityUnimplementedmarker from"effect_structure"to"static_structure"— both stayUnimplemented. The static handler that consumes this routing will land in the follow-up PR alongside the this-turn exile-link scoping (defect #5).Defect #3 in the issue (subject expansion dropped on the ETB trigger) was investigated and confirmed already correct in code — no change needed here.
Validation Failures
None.
CI Failures
None.