Fix Vigor prevention scope and counter follow-up - #1426
Fix Vigor prevention scope and counter follow-up#1426monsterdavidliu-ux wants to merge 2 commits into
Conversation
Parse "another creature you control" as valid_card on damage prevention replacements, and route the +1/+1 counter rider to the prevented recipient.
There was a problem hiding this comment.
Code Review
This pull request implements damage prevention replacement parsing and scoping logic to correctly handle cards like Vigor, including adding comprehensive unit and integration tests. A high-severity issue was identified in the parser where a manual string prefix check (.starts_with(',')) was used instead of idiomatic nom combinators, violating Rule R1 of the style guide.
| if rest.starts_with(',') { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
[HIGH] Avoid verbatim string or character checks for parsing Oracle phrases as it bypasses the robust nom-based parser.
Why it matters: Bypassing the nom-based parser with manual string/character checks creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.
Suggested fix: Use nom combinators to parse the delimiter.
let (rest, _) = nom::character::complete::char(',')(rest)?;References
- Rule R1: Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. Any new .contains(), .starts_with(), .ends_with(), .find(), or .split_once() used for parsing dispatch in non-test parser code is a finding. (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 to prevent combinatorial explosion and improve maintainability.
Replace starts_with(',') with char(',').parse() so the trailing-comma
case stays correct for ", prevent that damage" while satisfying parser R1.
…ation lens) Graft phase-rs#1426's runtime integration tests (vigor_regression.rs) onto this PR so the prevention scope + counter-recipient behavior is driven through the real apply() pipeline, not just parser-AST shape. The runtime paths (valid_card gate in replacement.rs, PostReplacementDamageTarget in targeting.rs:636) were only exercised by sibling cards before. Tighten vigor_event_recipient_filter_and_counter_target_rewrite to pin count == EventContextAmount (the field try_parse_for_each_effect sets for the suffix-form for-each) instead of the loose repeat_for-OR-count disjunction that did not discriminate which field carries the amount.
|
Thanks for this, @monsterdavidliu-ux — your diagnosis of Vigor was correct and your integration tests are excellent. I'm leaving this PR open so you can see the reasoning, but I want to be transparent: I'm merging the Vigor fix via #1425 instead, for an architectural reason. Both PRs make Vigor work, but they fix it at different seams:
Because both PRs edit the same region of Your work isn't lost: your two pipeline integration tests ( |
|
Thanks for the detailed explanation and for preserving the integration tests — I really appreciate it. The architectural reasoning makes sense, and I’m glad the tests could still contribute to the final fix. Happy to contribute more in the future 🙏 |
Summary
valid_card.Fixes Vigor — damage prevention applies to all creatures (missing "you control") and +1/+1 counter rider doesn't fire #1417
Test plan
cargo test -p engine vigor_prevention_scopes_to_other_creatures_you_controlcargo test -p engine --test integration vigor