fix(parser): Runadi, Behemoth Caller ETB counters and haste threshold - #6735
Conversation
Runadi's first ability failed to parse its composite "where X is its
mana value minus 4" quantity clause, misrouting the whole ability to
the generic self-ETB fallback and scoping the counter grant to Runadi
herself instead of the cast creature. Delegate to the shared
where-X-is suffix parser (already used by the sibling self-ETB path)
so composite/offset quantities resolve here too, and thread the
entering object through the Recipient scope resolver so "its mana
value" reads the creature actually entering, not the static source.
Separately, the haste static's "with three or more +1/+1 counters"
filter left "or more" stuck onto the counter-type text (a garbage
Generic("or more +1/+1") counter type), so no creature ever matched
and haste never applied. Strip the redundant or-more/or-greater
qualifier after the count, mirroring the existing mana-value handling.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe parser now represents cast-based “enters with counters” abilities as one-shot triggers, supports composite “where X is” quantities and counter-threshold qualifiers, and resolves recipient quantities against entering objects during ETB replacements. Runadi integration tests cover counters, haste, source removal, and spell-specific replacement binding. ChangesETB quantity and parser behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SpellCast
participant OracleParser
participant Trigger
participant AddTargetReplacement
participant EnteringCreature
SpellCast->>OracleParser: parse enters-with-counters text
OracleParser->>Trigger: create SpellCast trigger
Trigger->>AddTargetReplacement: install replacement bound to spell
AddTargetReplacement->>EnteringCreature: apply counters on entry
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Parse changes introduced by this PR · 8 card(s), 11 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed at current head a475003e30bf127aba9482c4b06553ff43122fc4.
[BLOCKING] Runadi’s Whenever you cast … ability is modeled as a static, object-hosted replacement, so it stops working if Runadi leaves before the cast creature resolves. oracle_replacement.rs:705-709 routes the whole sentence through parse_whenever_you_cast_enters_with, and :4728-4736 / :4828-4842 lower it to ReplacementDefinition. The replacement scan in game/replacement.rs:6249-6281 admits this source only from battlefield/command, whereas triggered abilities follow the independent trigger/stack path in functioning_abilities.rs:439-449.
The local official CR source confirms the distinction: CR 603.1 defines Whenever as a triggered ability and CR 603.3 puts it on the stack. CR 614.1c/614.12 covers the later enter-with replacement, not erasing the preceding trigger. Thus, removing Runadi after the cast but before the spell resolves loses the effect entirely; the implementation also never uses the stack for this ability. The new integration test resolves while Runadi remains on the battlefield, so it cannot expose that case.
Please model the Whenever you cast … portion as a TriggerDefinition, whose resolution creates a floating, entrant-bound replacement for the qualifying spell’s battlefield entry (or retain honest unsupported coverage until that exists). Add a regression that removes Runadi after the trigger is created and proves the qualified spell still enters with the correct counters; preserve the no-qualifying-spell behavior as well. Do not address this by widening the object-hosted replacement zones.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the trigger is now correctly stack-based, but its one-shot replacement is still not bound to the spell that triggered it.
🔴 Blocker
[HIGH] The floating replacement can be consumed by a different qualifying creature before the triggering spell resolves. Evidence: crates/engine/src/parser/oracle_replacement.rs:4911-4916 installs AddTargetReplacement with TargetFilter::None; crates/engine/src/game/effects/add_target_replacement.rs:231-242 consequently pushes that payload into the global pending-replacement registry. The parser comment at oracle_replacement.rs:4890-4895 assumes no event can interleave after the trigger resolves, but that is false: after the spell-cast trigger resolves, CR 117.3b gives the active player priority. They can cast another qualifying flash creature, whose battlefield entry is then the first matching zone-change event and consumes the one-shot replacement.
The replacement must be bound to the triggering SpellCast object through the event-context TriggeringSource / host semantics, rather than a global TargetFilter::None scan. Please add a runtime regression that casts two qualifying entrants with the second entering during that priority window, and proves the counters apply only to the original triggering spell's entrant.
The parse-diff sticky (<!-- coverage-parse-diff -->, comment 5108471570) was generated at 2026-07-28T18:58:35Z and predates this head (6f7275e5287ac73aab64875fae7875fd796f9d08, committed 2026-07-28T19:38:21Z). Regenerate/reconcile current-head parse-diff evidence after CI; this is evidence freshness, not a separate correctness finding.
Recommendation: request changes — preserve the trigger design, bind the replacement to its triggering spell, and cover the interleaving case.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@crates/engine/src/parser/oracle_replacement.rs`:
- Around line 4875-4927: Scope the floating replacement created by
parse_whenever_you_cast_enters_with_trigger to the specific spell instance that
caused the SpellCast trigger, rather than matching any qualifying battlefield
entry. Preserve the existing pending_damage_replacements installation and
consume_on_apply behavior, but ensure find_applicable_replacements can reject
entries from other spells before consuming the replacement.
In `@crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs`:
- Around line 177-182: Replace the direct `GameObject.zone` mutation for
`runadi` in the scenario setup with the existing `scenario`/`runner` zone-change
API that emits the replacement-aware `ProposedEvent::ZoneChange` pipeline.
Assert that Runadi is in the graveyard before calling `commit.resolve()`, while
preserving the existing stack and trigger setup.
- Around line 128-139: The MV4 negative test lacks a positive reach guard and
can pass if Runadi’s trigger was never registered. Update
runadi_grants_no_counters_below_mv5_threshold and its setup path to assert the
relevant ability parses successfully without Effect::Unimplemented and the
creature uses the normal runtime pipeline before retaining the existing (0,
false) filter assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c3ce4b2e-2233-41b9-8771-5edd3138611c
📒 Files selected for processing (3)
crates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs
The one-shot floating replacement Runadi's SpellCast trigger installs was scoped only by type/mana-value filter, so a different qualifying creature entering the battlefield during the priority window between the trigger resolving and the originally-cast spell resolving could consume it first, leaving the intended entrant uncountered. Bind the install to the specific spell that caused the trigger: the parser embeds a TRIGGERING_SPELL_PLACEHOLDER sentinel inside the replacement's valid_card (AND-combined with the existing filter), and Effect::AddTargetReplacement's resolve function concretizes it to the real triggering spell's id from the current trigger event at install time (or to a match-nothing id if none is extractable). Using a sentinel object id instead of a new ReplacementDefinition field avoids rippling a struct change through every exhaustive construction site, including the dormant mtgish-import crate. Also address review nits: the "Runadi leaves before the spell resolves" regression now drives the departure through the production zone-change pipeline instead of poking the object's zone field directly, and the negative mana-value test asserts the trigger actually registered before checking the zero-counter outcome. Added a new regression that casts two qualifying creatures back to back, proving each entrant gets only its own counters.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/game/effects/add_target_replacement.rs (1)
76-119: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the placeholder walk exhaustive.
TargetFilteris a closed enum, but both helpers fall back to_for unknown variants. That lets a future compositeTargetFiltervariant silently skip placeholder binding and leave the trigger source unbound. Match every current leaf explicitly so the compiler forces this code to be updated whenTargetFiltergrows.🤖 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/game/effects/add_target_replacement.rs` around lines 76 - 119, Make target_filter_contains_placeholder and concretize_triggering_spell_placeholder exhaustively match every current TargetFilter variant instead of using wildcard arms. Explicitly handle each leaf variant with the existing non-placeholder behavior, while preserving recursive handling for And, Or, and Not, so adding a new variant causes a compiler error until these helpers are updated.Sources: Coding guidelines, 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/game/effects/add_target_replacement.rs`:
- Around line 76-119: Make target_filter_contains_placeholder and
concretize_triggering_spell_placeholder exhaustively match every current
TargetFilter variant instead of using wildcard arms. Explicitly handle each leaf
variant with the existing non-placeholder behavior, while preserving recursive
handling for And, Or, and Not, so adding a new variant causes a compiler error
until these helpers are updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f2a2813d-7652-434d-aeac-05349af0ddd1
📒 Files selected for processing (4)
crates/engine/src/game/effects/add_target_replacement.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/types/identifiers.rscrates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/parser/oracle_replacement.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved at current head 48ebcdb: stack-trigger binding, spell-specific floating replacement, current parse-diff, and discriminating runtime interleaving coverage verified.
2 participants
Summary
parse_quantity_refcall couldn't express arithmetic, so the wholeparse_whenever_you_cast_enters_withcombinator bailed via?and the ability fell through to the generic self-ETB fallback (parse_enters_with_counters), which wrongly scoped the effect to Runadi herself (valid_card: SelfRef) instead of the cast creature, and absorbed the mana-value condition text as a garbage literal counter type.parse_enters_with_where_x_suffix(the existing shared authority for this grammar, already used by the sibling self-ETB path), which supports composite/offset expressions.ObjectScope::Recipient's resolution (object_for_scope/object_id_for_scopeingame/quantity.rs) never consultedQuantityContext.entering(the object entering the battlefield in an ETB-scoped replacement), so "its mana value" would have resolved to the static replacement source (Runadi) rather than the entering creature. Added anenteringfallback between the existingrecipient/targetschecks and the finalsourcefallback — inert for every non-ETB caller (ctx.enteringis only ever set by ETB-counter extraction), so existing CR 613.4c layer-recipient consumers (Blessing of the Nephilim, Civic Saber) are unaffected.Counters { counters: OfType(Generic("or more +1/+1")), ... }— the counter-count parser consumed "three" and a single trailing space but never stripped the redundant "or more"/"or greater" qualifier, so it leaked into the counter-type slice. No creature could ever match the filter, so haste never applied even once counters existed. Fixed by stripping the qualifier after the count, mirroring the existing CMC "N or greater" handling.Files changed
crates/engine/src/parser/oracle_replacement.rs— delegateparse_whenever_you_cast_enters_with's "where X is" clause to the shared composite-quantity parser; addedparses_runadi_behemoth_caller_replacementandwhenever_you_cast_enters_with_garbage_quantity_fails_closedtests.crates/engine/src/game/quantity.rs— addedctx.enteringfallback toObjectScope::Recipientresolution inobject_for_scopeandobject_id_for_scope.crates/engine/src/types/ability.rs— updatedObjectScope::Recipientdoc comment to describe the completed resolution chain.crates/engine/src/parser/oracle_target.rs— strip redundant "or more "/"or greater " after a counter count inparse_counter_spec_after_lead; addedparse_counter_suffix_three_or_more_plus1plus1andparse_counter_suffix_two_or_greater_stunregression tests.crates/engine/tests/integration/runadi_behemoth_caller_etb_counters.rs(new) — 3 runtime tests: MV8 creature enters with 4 counters and gains haste; MV5 creature enters with exactly 1 counter, no haste; MV4 creature enters with 0 counters (filter correctly excludes it), no haste.crates/engine/tests/integration/main.rs— registered the new test module.Track: Developer
Model: claude-sonnet-5
Tier: Frontier
Thinking: high
Implementation method (required): Method: not-applicable — root-caused and fixed directly after an initial
/engine-implementerplanning+review-plan round (which surfaced two real gaps that were incorporated); the full plan→review-plan→implement→review-impl pipeline could not be completed end-to-end within this session's time budget after an earlier interruption, so this is a direct, carefully-verified hand fix rather than a completed pipeline run.CR references: CR 614.1c (enters-with replacement effects), CR 202.3 (mana value), CR 107.1 + CR 604.3 (arithmetic quantity expressions), CR 613.4c (recipient-scoped continuous effects / layer 7c), CR 122.1 (counter-count filter comparators)
Verification
cargo fmt --all -- --check— cleancargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings— clean (fixed oneunnecessary_lazy_evaluationslint along the way)cargo test -p engine --lib):parses_runadi_behemoth_caller_replacement,whenever_you_cast_enters_with_garbage_quantity_fails_closed,parses_wildgrowth_archaic_replacement(sibling, no regression),parses_fixed_count_variant(sibling, no regression),plain_whenever_you_cast_is_not_replacement(no regression),parse_counter_suffix_three_or_more_plus1plus1,parse_counter_suffix_two_or_greater_stun, all existingparse_counter_suffix_*tests,resolve_object_mana_value_*(no regression) — all pass.cargo test -p engine --test integration runadi): all 3 new tests pass (MV8→4 counters+haste, MV5→1 counter/no haste, MV4→0 counters/no haste).cargo nextest run --profile ci --workspace --exclude phase-tauri --exclude mtgish-import --features engine/proptest— started but did not finish confirming within this session due to a mid-run interruption; not run to completion locally. Every directly-relevant test (all touched functions' existing + new tests) was run individually and passed; the sharedObjectScope::Recipientresolver change is scoped narrowly enough (only reachable whenctx.enteringisSome, set at exactly two call sites, both ETB-counter extraction) that a workspace-wide regression is considered unlikely, but this is not proven by a full suite run.Gate A: not generated this round (no PR/CI run yet).
Anchored on:
crates/engine/src/parser/oracle_replacement.rs:4741(parse_whenever_you_cast_enters_with) — root cause of the counter-scoping bugcrates/engine/src/game/quantity.rs:4687(object_for_scope'sRecipientarm) — root cause of the runtime scope-resolution gapcrates/engine/src/parser/oracle_target.rs:5527(parse_counter_spec_after_lead's count parser) — root cause of the haste-filter garbage counter typeFinal review-impl: not run this round (see Method note above).
Claimed parse impact: Fixes Runadi, Behemoth Caller's first ability (ETB counters) and second ability (haste threshold) end-to-end; the composite quantity-clause fix and the Recipient-scope fix generalize to the whole "Wildgrowth Archaic cousin" card family (Boreal Outrider, Torgal, and any future card using this shape with an arithmetic mana-value/power/toughness quantity); the "or more"/"or greater" counter-count fix generalizes to any card using "with N or more/or greater counter(s)" in a filter.
Scope Expansion: None.
Validation Failures: Full-workspace
cargo nextestwas not completed this session (see Verification above) — flagging honestly rather than claiming false completeness.CI Failures: None observed in what was run.
Closes #6492
Summary by CodeRabbit