fix(parser): bind a dig-from-among "that creature" shared-type reference to the trigger subject (Conjurer's Mantle #5900) - #6528
Conversation
… the trigger subject (Conjurer's Mantle phase-rs#5900) Conjurer's Mantle's attack trigger ("Whenever equipped creature attacks, look at the top six cards of your library. You may reveal a card that shares a creature type with that creature from among them and put it into your hand.") never let you reveal a matching card, because the shared-creature-type filter matched nothing. Root cause, two layers of the same lost-context bug: 1. `parse_dig_from_among` parsed the "from among them" reveal filter with the ctx-free `parse_target`, so the enclosing trigger subject never reached the filter's `parse_shared_quality_reference`. 2. Even with the ctx in scope, `parse_shared_quality_reference` routed only the bare pronoun "it" through the ctx-aware `resolve_pronoun_target`; the singular demonstrative "that creature" fell through to the ctx-free `parse_target`, binding the shared-quality reference to `ParentTarget`. In this attacks trigger there is no chosen target / recipient / effect-context object, so `parent_target_shared_quality_values` returns `None` and the "shares a creature type" test matched no card — the reported symptom. Fix: thread the `ParseContext` through `parse_dig_from_among`, and resolve the singular object demonstratives ("that creature" / "that permanent" / "that card") via the same `resolve_pronoun_target` path "it" already used. With a non-source trigger subject in scope the reference now binds to `TriggeringSource` (the attacking equipped creature); without one it stays `ParentTarget`, so non-trigger dig-from-among filters are unchanged. Regression: a runtime test drives the equipped Goblin's attack through the real combat + Dig reveal pipeline and asserts the shared-type Goblin card is selectable (empty pre-fix), plus a parser guard pinning the `TriggeringSource` binding. Both RED pre-fix, GREEN post-fix (verified via stash). Closes phase-rs#5900 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe parser now threads ChangesConjurer’s Mantle parser fix
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_target.rs (1)
6163-6178: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing "that token" demonstrative variant.
The loop covers
"that creature","that permanent","that card"but omits"that token". The sibling anaphora combinator incrates/engine/src/parser/oracle_effect/counter.rs(counter_anaphor_created_token_binding) already treats"that token"/"the token"/"the permanent"as part of the same demonstrative family for an analogous binding, suggesting"that token"is a real Oracle form for shared-quality references (e.g. "a card that shares a card type with that token") that this arm would currently miss and fall through to the genericparse_targetpath.Suggested fix
- for demonstrative in ["that creature", "that permanent", "that card"] { + for demonstrative in ["that creature", "that permanent", "that card", "that token"] {🤖 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_target.rs` around lines 6163 - 6178, Extend the demonstrative list in the pronoun-resolution loop around resolve_pronoun_target to include “that token”. Preserve the existing ctx-aware binding and fallback behavior for the other singular demonstratives.crates/engine/src/parser/oracle_effect/sequence.rs (1)
5275-5291: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate filter-resolution block across both
from amongbranches.Both the "milled this way" branch and the "from among them/those" branch carry an identical empty/
"card"/"cards"/"of them"guard plus the same multi-line CR-citedparse_target_with_ctxcall and comment. This is the same class of duplication that caused issue#5900(one branch had drifted to ctx-freeparse_targetwhile the other usedparse_target_with_ctx). Extracting a small shared helper would make the two branches structurally impossible to desync again.Suggested extraction
+fn resolve_dig_from_among_filter_text(filter_text: &str, ctx: &mut ParseContext) -> TargetFilter { + if filter_text.is_empty() + || filter_text == "card" + || filter_text == "cards" + || filter_text == "of them" + { + return TargetFilter::Any; + } + // CR 608.2c + CR 608.2k: parse the "from among them" reveal/put filter + // with the enclosing ctx so an anaphoric reference inside it binds to + // the trigger subject (`TriggeringSource`) when one exists (`#5900`). + let (parsed_filter, _) = parse_target_with_ctx(filter_text, ctx); + parsed_filter +}Then both call sites become
let filter = resolve_dig_from_among_filter_text(filter_text, ctx);.Also applies to: 5382-5398
🤖 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_effect/sequence.rs` around lines 5275 - 5291, Extract the duplicated filter-resolution logic from both “milled this way” and “from among them/those” branches into a shared helper such as resolve_dig_from_among_filter_text, preserving the empty/“card”/“cards”/“of them” handling and ctx-aware parse_target_with_ctx behavior. Replace both local blocks with calls to the helper, passing filter_text and ctx.
🤖 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_effect/sequence.rs`:
- Around line 5275-5291: Extract the duplicated filter-resolution logic from
both “milled this way” and “from among them/those” branches into a shared helper
such as resolve_dig_from_among_filter_text, preserving the
empty/“card”/“cards”/“of them” handling and ctx-aware parse_target_with_ctx
behavior. Replace both local blocks with calls to the helper, passing
filter_text and ctx.
In `@crates/engine/src/parser/oracle_target.rs`:
- Around line 6163-6178: Extend the demonstrative list in the pronoun-resolution
loop around resolve_pronoun_target to include “that token”. Preserve the
existing ctx-aware binding and fallback behavior for the other singular
demonstratives.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a55ef39-405d-4b51-b7fe-eadbfd5f2c46
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_target.rscrates/engine/tests/integration/issue_5900_conjurers_mantle.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved: current-head parser context propagation uses the established target parser, and the registered combat-to-Dig regression is discriminating.
…nce to the trigger subject (Conjurer's Mantle phase-rs#5900) (phase-rs#6528) * fix(parser): bind a dig-from-among "that creature" shared-type ref to the trigger subject (Conjurer's Mantle phase-rs#5900) Conjurer's Mantle's attack trigger ("Whenever equipped creature attacks, look at the top six cards of your library. You may reveal a card that shares a creature type with that creature from among them and put it into your hand.") never let you reveal a matching card, because the shared-creature-type filter matched nothing. Root cause, two layers of the same lost-context bug: 1. `parse_dig_from_among` parsed the "from among them" reveal filter with the ctx-free `parse_target`, so the enclosing trigger subject never reached the filter's `parse_shared_quality_reference`. 2. Even with the ctx in scope, `parse_shared_quality_reference` routed only the bare pronoun "it" through the ctx-aware `resolve_pronoun_target`; the singular demonstrative "that creature" fell through to the ctx-free `parse_target`, binding the shared-quality reference to `ParentTarget`. In this attacks trigger there is no chosen target / recipient / effect-context object, so `parent_target_shared_quality_values` returns `None` and the "shares a creature type" test matched no card — the reported symptom. Fix: thread the `ParseContext` through `parse_dig_from_among`, and resolve the singular object demonstratives ("that creature" / "that permanent" / "that card") via the same `resolve_pronoun_target` path "it" already used. With a non-source trigger subject in scope the reference now binds to `TriggeringSource` (the attacking equipped creature); without one it stays `ParentTarget`, so non-trigger dig-from-among filters are unchanged. Regression: a runtime test drives the equipped Goblin's attack through the real combat + Dig reveal pipeline and asserts the shared-type Goblin card is selectable (empty pre-fix), plus a parser guard pinning the `TriggeringSource` binding. Both RED pre-fix, GREEN post-fix (verified via stash). Closes phase-rs#5900 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(PR-6528): resolve parser review findings --------- Co-authored-by: rsnetworkinginc <rsnetworkinginc@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Summary
Fixes Conjurer's Mantle (#5900): its attack trigger — "Whenever equipped creature attacks, look at the top six cards of your library. You may reveal a card that shares a creature type with that creature from among them and put it into your hand." — never let you reveal a matching card, because the shared-creature-type filter matched nothing.
Root cause
Two layers of the same lost-context bug:
parse_dig_from_amongparsed the "from among them" reveal filter with the ctx-freeparse_target, so the enclosing trigger subject never reached the filter'sparse_shared_quality_reference.parse_shared_quality_referencerouted only the bare pronoun "it" through the ctx-awareresolve_pronoun_target; the singular demonstrative "that creature" fell through to the ctx-freeparse_target, binding the shared-qualityreferencetoParentTarget.In this attacks trigger there is no chosen target / recipient / effect-context object, so
parent_target_shared_quality_values(game/filter.rs) returnsNoneand the "shares a creature type" test matched no card — exactly the reported symptom (no card in the top six is ever selectable).Sibling cards that reference the triggering creature with the pronoun "it" (Mana Echoes, the attacker anthem) already bind to
TriggeringSource; only the demonstrative form in the dig-from-among reveal filter was missing.Fix
ParseContextthroughparse_dig_from_amongso the reveal filter parses with the trigger subject in scope.resolve_pronoun_targetpath the bare pronoun "it" already used.With a non-source trigger subject in scope the reference now binds to
TriggeringSource(the attacking equipped creature); without one it staysParentTarget, so non-trigger dig-from-among filters are unchanged.resolve_pronoun_target's existing subject discriminator (typed /AttachedTosubject →TriggeringSource;None/SelfRef/Any→ParentTarget) contains the blast radius to genuine trigger-subject contexts.Tests (RED pre-fix / GREEN post-fix, verified via stash)
conjurers_mantle_reveal_matches_shared_creature_type— drives the equipped Goblin's attack through the real combat + Dig reveal pipeline and asserts the shared-type Goblin card in the top six is selectable (empty pre-fix); non-creature fillers stay non-selectable; the revealed Goblin lands in hand.conjurers_mantle_shared_type_reference_binds_to_trigger_subject— parser guard pinning the reference toTriggeringSource(ParentTargetpre-fix).CI parity (isolated
CARGO_TARGET_DIR)cargo fmt --all -- --check— cleancargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings— cleancargo test -p engine --lib— 17565 passed, 0 failedcargo test -p engine --test integration— 3837 passed, 0 failedCloses #5900
Model: claude-opus-4
Tier: Standard
Thinking: high
Gate A: PASS —
cargo fmt --all -- --check,cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings,cargo test -p engine --lib(17565 passed / 0 failed), andcargo test -p engine --test integration(3837 passed / 0 failed), all clean in an isolatedCARGO_TARGET_DIR, pre- and post-rebase onto latestorigin/main.Anchored on:
crates/engine/src/parser/oracle_effect/sequence.rs—parse_dig_from_amongthreadsParseContextand parses the "from among them" filter viaparse_target_with_ctx.crates/engine/src/parser/oracle_target.rs—parse_shared_quality_referenceroutes the singular object demonstratives through the existing ctx-awareresolve_pronoun_target.crates/engine/src/game/filter.rs—parent_target_shared_quality_values/object_shares_quality_with_reference_filter(unchanged) resolveTriggeringSourcefromstate.current_trigger_events(theAttackersDeclaredevent), which is why the trigger-subject binding is the correct seam.Summary by CodeRabbit
Bug Fixes
Tests