fix(engine): scope Vorinclex-class counter replacements by actor and let loyalty costs pause on CR 616.1 ordering choices#6261
Conversation
…let loyalty costs pause on CR 616.1 ordering choices
Player 0 activating a loyalty ability with an opponent's Vorinclex,
Monstrous Raider on the battlefield panicked at planeswalker.rs:399
("a loyalty cost cannot pause on a self zone move"): the doubling
clause parsed with no valid_player scope so it applied to every
player's counter additions, joining the halving clause as two
order-material quantity modifiers -> NeedsChoice -> Paused ->
unreachable!.
- Add CounterReplacementSubject { Recipient (default), Actor } on
ReplacementDefinition: Vorinclex-class clauses ("you / an opponent /
a player would put ... counters") scope by the ACTOR putting the
counters (per the official ruling), while the Doubling Season family
keeps scoping by the affected permanent via valid_card.
- Parser: parse_counter_replacement now emits all three actor scopes
(You / Opponent / AnyPlayer) instead of only the opponent form.
- Runtime: AddCounter replacement applicability compares the
subject-selected player (CounterPlacement::actor() vs recipient
controller) in both player- and object-counter branches (CR 614.1a).
- Loyalty activations can now legitimately pause on a CR 616.1
replacement-ordering choice (e.g. own Doubling Season + opponent
Vorinclex): finalize_loyalty_activation parks a typed
PendingCostMoveResume::LoyaltyActivation and resume_loyalty_activation
completes the tail exactly once after the choice (CR 606.3/606.4).
Tests: parser scope emission, actor!=recipient runtime discrimination,
SelfRef-doubler no-regression, loyalty halving/doubling via apply(),
and full pause->ChooseReplacement->resume integration for both
orderings.
There was a problem hiding this comment.
Code Review
This pull request implements actor-scoped counter replacements (such as Vorinclex, Monstrous Raider) and refactors planeswalker loyalty activations to correctly pause and resume during replacement-ordering choices (CR 614.1a, CR 606.4, CR 616.1). Review feedback highlights a violation of the parser style guide (R1) due to the use of scan_contains instead of nom combinators in oracle_replacement.rs, and suggests using idiomatic Rust let-else statements instead of nested match blocks in replacement.rs.
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.
| } else if nom_primitives::scan_contains(lower, "a player would put") { | ||
| def.valid_player = Some(ReplacementPlayerScope::AnyPlayer); | ||
| def.counter_replacement_subject = CounterReplacementSubject::Actor; | ||
| } else if nom_primitives::scan_contains(lower, "you would put") { | ||
| def.valid_player = Some(ReplacementPlayerScope::You); | ||
| def.counter_replacement_subject = CounterReplacementSubject::Actor; | ||
| } |
There was a problem hiding this comment.
Avoid using string scanning helpers like scan_contains 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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.
References
- R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. (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.
| let scope_player = match repl_def.counter_replacement_subject { | ||
| CounterReplacementSubject::Actor => placement.actor(), | ||
| CounterReplacementSubject::Recipient => { | ||
| match state.objects.get(&object_id).map(|o| o.controller) { | ||
| Some(c) => c, | ||
| None => return false, | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
[MEDIUM] Use idiomatic
let-elsestatement instead of nestedmatch. Evidence:crates/engine/src/game/replacement.rs:6030-6035.
Why it matters: Usinglet-elseavoids deep nesting and makes the fallback control flow explicit and idiomatic.
Suggested fix: Replace the nestedmatchwith alet-elsestatement.
| let scope_player = match repl_def.counter_replacement_subject { | |
| CounterReplacementSubject::Actor => placement.actor(), | |
| CounterReplacementSubject::Recipient => { | |
| match state.objects.get(&object_id).map(|o| o.controller) { | |
| Some(c) => c, | |
| None => return false, | |
| } | |
| } | |
| }; | |
| let scope_player = match repl_def.counter_replacement_subject { | |
| CounterReplacementSubject::Actor => placement.actor(), | |
| CounterReplacementSubject::Recipient => { | |
| let Some(c) = state.objects.get(&object_id).map(|o| o.controller) else { | |
| return false; | |
| }; | |
| c | |
| } | |
| }; |
References
- Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. (link)
Parse changes introduced by this PR · 4 card(s), 1 signature(s) (baseline: main
|
…lacementDefinition literals Merging origin/main added new mtgish-import construction sites that predate the counter_replacement_subject field this branch adds to ReplacementDefinition. Engine sites go through ReplacementDefinition::new() (which seeds the field); the mtgish-import converter builds exhaustive literals, so each of the 9 sites must name the field explicitly. Recipient is the default (non-Vorinclex) scope.
Player 0 activating a loyalty ability with an opponent's Vorinclex,
Monstrous Raider on the battlefield panicked at planeswalker.rs:399
("a loyalty cost cannot pause on a self zone move"): the doubling
clause parsed with no valid_player scope so it applied to every
player's counter additions, joining the halving clause as two
order-material quantity modifiers -> NeedsChoice -> Paused ->
unreachable!.
ReplacementDefinition: Vorinclex-class clauses ("you / an opponent /
a player would put ... counters") scope by the ACTOR putting the
counters (per the official ruling), while the Doubling Season family
keeps scoping by the affected permanent via valid_card.
(You / Opponent / AnyPlayer) instead of only the opponent form.
subject-selected player (CounterPlacement::actor() vs recipient
controller) in both player- and object-counter branches (CR 614.1a).
replacement-ordering choice (e.g. own Doubling Season + opponent
Vorinclex): finalize_loyalty_activation parks a typed
PendingCostMoveResume::LoyaltyActivation and resume_loyalty_activation
completes the tail exactly once after the choice (CR 606.3/606.4).
Tests: parser scope emission, actor!=recipient runtime discrimination,
SelfRef-doubler no-regression, loyalty halving/doubling via apply(),
and full pause->ChooseReplacement->resume integration for both
orderings.