fix(parser): recognize plural "leave the battlefield" in the generic exile-until duration combinator#5871
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the duration parser in duration.rs to accept both singular ('leaves') and plural ('leave') verb forms for host-lifetime expiry, fixing a bug where cards with plural names (such as 'Cloak and Dagger, Entwined') would not correctly apply their 'until leaves the battlefield' duration. A comprehensive integration test was added to verify this behavior. The review feedback points out that the code comment cites an incorrect MTG Comprehensive Rule (CR) and violates the style guide's mandatory CR annotation format, suggesting a correction to use CR 611.2b with the proper description.
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.
| // "leaves the battlefield"/"leave the battlefield" alt) — CR 603.2c | ||
| // notes the plural "leave" form is used with plural/batched subjects. |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation and format.
Why it matters: The comment cites CR 603.2c (which governs triggered abilities triggering once per event) for a continuous effect duration parser, and does not follow the required CR <number>: <description> format. It should cite CR 611.2b which specifically governs continuous effects with 'until [something] leaves the battlefield' durations.
Suggested fix: Update the comment to use the correct CR 611.2b citation and format.
| // "leaves the battlefield"/"leave the battlefield" alt) — CR 603.2c | |
| // notes the plural "leave" form is used with plural/batched subjects. | |
| // "leaves the battlefield"/"leave the battlefield" alt). | |
| // CR 611.2b: Some continuous effects generated by the resolution of a spell or ability have durations phrased "until [something] leaves the battlefield." |
References
- Every rules-touching line of engine code must carry a comment of the form
CR <number>: <description>. The cited rule's body must describe what the code is doing. (link)
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: Changes requested
🔴 Blocker
-
[HIGH] The newly parsed duration is lost on the ordinary multi-candidate path.
crates/engine/src/game/engine_resolution_choices.rsreconstructsChangeZoneIterationCtxfromWaitingFor::EffectZoneChoicewithduration: None, as this PR's own module documentation notes. The runtime fixture deliberately removes every competing eligible card so it takes the single-candidate shortcut; it therefore cannot prove the advertised card works when an opponent has two nonland cards. Thread the duration through the choice/resume authority and add a two-eligible-card runtime test that selects one, then verifies the exile link returns it when Cloak and Dagger leaves. -
[HIGH] This leaves coverage dishonest for the full Oracle sentence. The parser still accepts Cloak and Dagger while dropping the
or the chosen creaturealternative, the secondaryup to one target creaturedeclaration it depends on, and the chosen-opponent hand binding. The test's direct zone mutation works around that last error rather than exercising the actual card. Preserve the clause as a strict unsupported marker until its target/anaphor binding is implemented, or complete the general engine/parser work in the same PR.
🟡 Non-blocking
- [MED] The new
CR 603.2ccitation is unrelated. That rule is about repeated triggering from one event; the verified duration rule isCR 611.2a. Correct the annotation while reworking the duration path.
✅ Clean
- The plural
leaveparser alternative is composed at the existing duration combinator and the parse-diff is limited to Cloak and Dagger.
Recommendation: Complete the general choice-resume and Oracle-clause handling, then resubmit with a multi-candidate end-to-end regression.
Cloak and Dagger, Entwined's ETB exiles a nonland card from the chosen opponent's hand "until Cloak and Dagger leave the battlefield" — plural verb agreement, since the card's own name is a plural subject. The "until X leaves the battlefield" duration combinator (parser/oracle_nom/duration.rs) only matched the singular "leaves" form, so `duration` silently stayed None and the exiled card never came back when the source left the battlefield (no ExileLinkKind::UntilSourceLeaves link was ever created). The sibling *trigger* detector for LTB already accepted both "leaves"/"leave" (oracle_trigger.rs, oracle_effect/mod.rs); the duration combinator just never got the same alternative. Fix: accept "leave the battlefield" alongside "leaves the battlefield" in parse_until_body, mirroring the existing trigger-side alternation. Added a regression test (issue_4235_cloak_and_dagger_entwined.rs) covering both the AST-level duration fix and a full runtime cast -> reveal -> exile -> destroy source -> return-to-hand pipeline. Two separate, unrelated gaps were found during investigation but are NOT fixed here (documented in the test's module doc and in investigated-issues.md): (1) the "or the chosen creature" alternative exile target has no parser support at all, and neither does the "up to one target creature they control" secondary target it depends on; (2) the exile filter isn't scoped to the specific chosen opponent's hand (matches a nonland card in either player's hand), and (3) WaitingFor::EffectZoneChoice's interactive round-trip drops `duration` entirely regardless of card. Both are real but architecturally larger than this scoped, card-specific bugfix warrants. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e round-trip; keep the unsupported alternative a strict failure Review follow-up (phase-rs#4235), both blockers + the citation note: 1. Duration carry-through (blocker 1). WaitingFor::EffectZoneChoice gains a serde-defaulted duration field, set from ability.duration at the change_zone pause site and consumed by the resume authority (engine_resolution_choices reconstructs ChangeZoneIterationCtx with the carried value instead of hardcoding None; the PayCost cost-exile reconstruction stays deliberately None — no 'until ...' idiom pays a cost). All ~30 construction sites threaded (visibility/server-core redaction rebuilds pass it through — a public effect parameter, not private hand info). New runtime regression drives a hand-built bounded exile through the production resolve_ability_chain -> change_zone::resolve -> EffectZoneChoice -> resume pipeline with TWO eligible candidates: the prompt carries the duration, the chosen card's UntilSourceLeaves{Hand} link survives the round-trip, and the card returns to its owner's hand when the source is destroyed. 2. Coverage honesty (blocker 2). The full printed sentence's 'or the chosen creature' alternative composes the hand pick with an anaphorically chosen object no filter can represent yet; the exile imperative now declines any 'or the chosen <type>' alternative (parse_exile_ast guard), so the whole clause lowers to an honest Effect::Unimplemented strict failure instead of a supported-looking exile that silently dropped the printed alternative. Pinned by a full-card parser test; the plural-'leave' duration fix itself is pinned on a supported single-referent subset of the same idiom. 3. Citation: the duration combinator's comment now cites CR 611.2a (the effect's stated duration), replacing the unrelated CR 603.2c. Verified in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib 17515 passed; engine integration 3751 passed.
638058c to
51dabfa
Compare
|
Addressed both blockers and the citation note. Blocker 1 — duration through the choice/resume authority: Blocker 2 — coverage honesty: the exile imperative now declines any "or the chosen " anaphoric alternative ( Citation: the duration combinator comment now cites CR 611.2a, replacing the unrelated CR 603.2c. Verified locally in an isolated target dir: fmt clean, workspace clippy |
📝 WalkthroughWalkthroughThe PR adds optional duration data to ChangesInteractive duration propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/engine/src/game/triggers.rsast-grep timed out on this file Comment |
|
Maintainer status: reviewed current head The prior parse-diff was produced for the July 15 head, so it cannot evidence this update. The earlier duration choice/resume and coverage-honesty blockers were checked against this head; no approval or enqueue will occur until the current evidence is available. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: Current-head review confirms duration survives the multi-candidate EffectZoneChoice resume path; the unsupported anaphoric alternative remains an explicit Effect::Unimplemented, and the refreshed parse diff is limited to Cloak and Dagger, Entwined.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_nom/duration.rs (1)
73-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared singular/plural "leave(s) the battlefield" alternation.
The added comment itself notes this exact alternation is already duplicated in
oracle_trigger.rs'sleaves_tailandoracle_effect/mod.rs. Now a third copy exists here. Consider factoring a singlefn leaves_the_battlefield_tail(...)combinator reused by all three call sites, so future drift (e.g. a fourth verb form) only needs a single edit.As per coding guidelines, "Reuse existing shared building blocks before adding utility or inline extraction logic" and "Build reusable card-class building blocks rather than one-card special cases."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_nom/duration.rs` around lines 73 - 91, Extract the duplicated singular/plural “leaves the battlefield” parser into a shared leaves_the_battlefield_tail combinator, then replace the inline alternations in the duration parser, oracle_trigger.rs leaves_tail, and oracle_effect/mod.rs with that helper. Preserve each caller’s existing surrounding subject parsing and behavior while centralizing future verb-form changes.Source: Coding guidelines
crates/engine/tests/integration/issue_4235_cloak_and_dagger_entwined.rs (1)
109-152: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winScope the
Unimplementedcheck to the exile clause, not "anywhere in the chain."
chain_has_unimplementedwalks the whole ETBeffect/sub_ability/else_abilitychain and passes ifEffect::Unimplementedshows up anywhere. That doesn't prove the new "or the chosen " decline guard inimperative.rsis what produced it — an unrelated parsing gap elsewhere in the full card text (e.g. the "choose target opponent and up to one target creature they control" declaration) could independently yield anUnimplementednode and make this test pass even if that specific guard were removed or broken.Consider asserting the
Unimplementedoccurs specifically on the node reached by walking to theExile-destinationChangeZoneclause (mirroring thecursorwalk in the test above it), or additionally assert the earlier "choose ... reveal their hand" legs parsed to non-Unimplementedshapes, so the test localizes the failure to the fix under test.As per path instructions, "a parser AST shape test does NOT prove runtime semantics or a coverage-support claim" and negative/presence assertions need "a paired positive reach-guard proving the input actually reached the code under test."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/issue_4235_cloak_and_dagger_entwined.rs` around lines 109 - 152, Scope the strict-failure assertion in full_card_with_chosen_creature_alternative_stays_strict_failure to the exile clause rather than any node in the ETB chain. Walk the relevant ability structure to the ChangeZone effect whose destination is Exile, then assert that this specific clause contains Effect::Unimplemented; also verify the preceding choose/reveal legs are parsed into supported shapes so the test proves execution reached the intended lowering guard.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/src/parser/oracle_nom/duration.rs`:
- Around line 73-91: Extract the duplicated singular/plural “leaves the
battlefield” parser into a shared leaves_the_battlefield_tail combinator, then
replace the inline alternations in the duration parser, oracle_trigger.rs
leaves_tail, and oracle_effect/mod.rs with that helper. Preserve each caller’s
existing surrounding subject parsing and behavior while centralizing future
verb-form changes.
In `@crates/engine/tests/integration/issue_4235_cloak_and_dagger_entwined.rs`:
- Around line 109-152: Scope the strict-failure assertion in
full_card_with_chosen_creature_alternative_stays_strict_failure to the exile
clause rather than any node in the ETB chain. Walk the relevant ability
structure to the ChangeZone effect whose destination is Exile, then assert that
this specific clause contains Effect::Unimplemented; also verify the preceding
choose/reveal legs are parsed into supported shapes so the test proves execution
reached the intended lowering guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dba2e368-f337-4108-bd60-7d35d201f77f
📒 Files selected for processing (24)
crates/engine/src/ai_support/candidates.rscrates/engine/src/game/costs.rscrates/engine/src/game/effects/attach.rscrates/engine/src/game/effects/blight.rscrates/engine/src/game/effects/bounce.rscrates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/put_on_top.rscrates/engine/src/game/effects/sacrifice.rscrates/engine/src/game/effects/tap_untap.rscrates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/public_state.rscrates/engine/src/game/triggers.rscrates/engine/src/game/visibility.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_nom/duration.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/cost_zone_pipeline.rscrates/engine/tests/integration/issue_4235_cloak_and_dagger_entwined.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/mimeoplasm_interactive_exile.rscrates/server-core/src/filter.rs
2 participants
Fixes #4235.
Cloak and Dagger, Entwined: "...You may exile a nonland card from their hand or the chosen creature until Cloak and Dagger leave the battlefield."
Root cause & fix
parse_until_body(the generic "until [source] leaves the battlefield" duration combinator) only matched the singular verb form "leaves the battlefield". Cloak and Dagger, Entwined's own name is a plural subject, so its printed oracle text uses plural agreement — "until Cloak and Dagger leave the battlefield" — which never matched. This left the exile sub-ability'sdurationfield silentlyNone, so noExileLinkKind::UntilSourceLeaveslink was ever created, and a card exiled by the ETB trigger never returned when Cloak and Dagger left the battlefield.The sibling trigger detector for leaves-the-battlefield already handled both "leaves"/"leave" forms — only the duration combinator was missing the alternative. Fixed by mirroring that existing alternation.
Testing
New regression test (AST-level + full runtime cast→reveal→exile→destroy-source→return-to-hand) — confirmed RED without the fix, GREEN with it.
Verified locally (rebased onto main): fmt clean, clippy clean, 16672 lib tests pass, 3129 integration tests pass.
Summary by CodeRabbit
Bug Fixes
Tests