fix(parser): lower 'gains no life instead' to Prevent replacement (#743) - #3166
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements parsing and execution support for lifegain-negation replacement effects (such as Sulfuric Vortex's "gains no life instead") by mapping them to a structured QuantityModification::Prevent instead of an Unimplemented no-op. It also refactors player scope parsing for lifegain replacements into a shared helper and adds comprehensive unit and integration tests. The feedback suggests wrapping the lifegain-negation parser combinator in all_consuming to prevent partial matches on compound effects.
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.
| let mut combinator = preceded( | ||
| subject, | ||
| value((), alt((tag("gains no life"), tag("gain no life")))), | ||
| ); | ||
| combinator.parse(lower_body.trim()).is_ok() |
There was a problem hiding this comment.
Using all_consuming ensures that the entire effect body is matched. Without it, a compound effect starting with a lifegain negation (e.g., "gains no life and draws a card instead") could be partially matched, resulting in the execute effect being silently dropped.
| let mut combinator = preceded( | |
| subject, | |
| value((), alt((tag("gains no life"), tag("gain no life")))), | |
| ); | |
| combinator.parse(lower_body.trim()).is_ok() | |
| let mut combinator = all_consuming(preceded( | |
| subject, | |
| value((), alt((tag("gains no life"), tag("gain no life")))), | |
| )); | |
| combinator.parse(lower_body.trim()).is_ok() |
References
- 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.
Summary
Fixes #743 — Sulfuric Vortex's second ability ("If a player would gain life, that player gains no life instead") was silently dropped. (The damage half was already correct, structurally identical to Roiling Vortex.)
Root cause
The "would gain life … instead" replacement clause was routed through
parse_effect_chain, which lowered "gain no life" toEffect::Unimplemented. The engine treatsUnimplementedas passthrough → no substitution → the life gain proceeded.Fix (parser-only)
In the existing "would gain life … instead" branch of
oracle_replacement.rs, detect the lifegain-negation body with nom combinators (value(Prevent, alt((tag("gains no life"), tag("gain no life"))))over the lowercased body) and emit.quantity_modification(QuantityModification::Prevent)with noexecute, mirroringparse_global_player_counter_prohibition. The engine'sgain_life_applieralready readsquantity_modificationfirst andPrevent => ApplyResult::Preventedfully suppresses the gain (CR 614.6) — no engine change needed.Scope (corrected from the plan)
The genuine "would gain life … gains no life instead" replacement class is exactly 2 cards: Sulfuric Vortex and Flames of the Blood Hand. (The Erebos / Everlasting Torment / Leyline of Punishment "can't gain life" cards are a separate, already-handled
StaticAbilityMode::CantGainLifestatic class.) Flames carries athis turnduration (CR 611.2a) and is explicitly guarded out of the permanentPrevent(!scan_contains(lower, "would gain life this turn")) so it keeps its prior deferred behavior — its durational variant is a follow-up.Tests
sulfuric_vortex_prevents_lifegain(runtime): casts a real "gain 3 life" spell with Sulfuric Vortex out, asserts life unchanged; paired control proves the spell gains life without the Vortex.quantity_modification: Prevent, notUnimplemented.Plan reviewed clean + implementation reviewed clean (independent reviewer confirmed bug-is-fixed, nom-compliance, and Flames scoping). CR numbers grep-verified (119.10, 614.6, 614.1a, 611.2a).