feat(parser): Vraska, Betrayal's Sting -2 become-Treasure and -9 poison-to-difference#5403
Conversation
…on-to-difference
Both loyalty abilities parsed to Unimplemented.
[-9] 'If target player has fewer than nine poison counters, they get a number of
poison counters equal to the difference': split_leading_conditional severed the
threshold from the body, and the body's 'a number of ... equal to the difference'
had no literal count for try_parse_player_counter. A new whole-sentence oracle_nom
rider (player_counter_difference) dispatched in parse_effect_chain_ir BEFORE the
conditional split captures the threshold N and emits GivePlayerCounter with a
ClampMin{0, Offset{9, Multiply{-1, Ref(TargetControllerCounter{Poison})}}} count =
max(0, 9 - current), bound to the same chosen target player. condition:None — the
clamp encodes the 'if fewer than' gate (>=9 -> 0 -> no-op). parse_player_counter_kind
is consolidated into oracle_nom/primitives.rs as the single kind authority.
[-2] 'Target creature becomes a Treasure artifact with {T},Sac:Add any color and
loses all other card types and abilities': ordered ContinuousModification set
(SetCardTypes[Artifact], RemoveAllSubtypes{Creature}+{Artifact} BEFORE AddSubtype
so Treasure survives per CR 613.7a written-order, RemoveAllAbilities before the
granted ability per CR 613.8a).
No new engine variants. CR 107.1b/122.1/122.1f/205.1a/613.1d/613.1f/613.7a/613.8a.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements parsing and logic for Vraska, Betrayal's Sting's loyalty abilities, specifically adding a threshold player-counter top-up rider for the [-9] ability and a full-replacement animation parser for the [-2] ability, alongside corresponding integration tests. Feedback highlights a critical omission where the new player_counter_difference::try_parse is not wired into the main parser dispatch, a medium issue regarding incomplete subtype removal for non-creature/non-artifact card types under CR 205.1a, and a parser limitation failing to handle the singular 'counter' form.
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.
| pub mod enchant; | ||
| pub mod error; | ||
| pub mod filter; | ||
| pub mod player_counter_difference; |
There was a problem hiding this comment.
[CRITICAL] Missing wiring for player_counter_difference::try_parse.\n\nWhy it matters: The new player_counter_difference module is declared, but its try_parse function is never actually called or dispatched in parse_effect_chain_ir (or any other parser file) in this PR. Without this wiring, the [-9] ability will still parse to Unimplemented, and the integration tests will fail to compile or run correctly.\n\nSuggested fix: Ensure the modified file containing parse_effect_chain_ir (likely crates/engine/src/parser/oracle_effect/mod.rs or chain.rs) is committed and includes the dispatch to player_counter_difference::try_parse before the conditional split.
| result.push(ContinuousModification::RemoveAllSubtypes { | ||
| set: SubtypeSet::Creature, | ||
| }); | ||
| result.push(ContinuousModification::RemoveAllSubtypes { | ||
| set: SubtypeSet::Artifact, | ||
| }); |
There was a problem hiding this comment.
[MEDIUM] Incomplete subtype removal for non-creature/non-artifact card types.\n\nWhy it matters: If the target creature has other card types (such as an Enchantment Creature or an animated Planeswalker/Land) and becomes a "Treasure artifact", it loses those other card types but will incorrectly retain their associated subtypes (e.g., Enchantment or Planeswalker subtypes) because RemoveAllSubtypes is only applied to Creature and Artifact.\n\nSuggested fix: Add RemoveAllSubtypes for other potential subtype sets (like Enchantment, Planeswalker, and Land) to ensure full compliance with CR 205.1a when losing those card types.
References
- Strict fidelity to the MTG Comprehensive Rules (CR) requires that every game rule, validation, and computed value matches the CR exactly (CR 205.1a). (link)
| let (rest, _) = tag::<_, _, OracleError<'_>>(" counters, they get a number of ") | ||
| .parse(rest) | ||
| .ok()?; |
There was a problem hiding this comment.
[MEDIUM] Singular counter form is not handled.\n\nWhy it matters: If the threshold is exactly one (e.g., "fewer than one poison counter"), the text will use the singular "counter" instead of "counters", causing the hardcoded " counters, they get a number of " tag to fail to parse.\n\nSuggested fix: Use alt to support both singular and plural forms. Also, propagate the parser error directly instead of swallowing it with .ok()?.
| let (rest, _) = tag::<_, _, OracleError<'_>>(" counters, they get a number of ") | |
| .parse(rest) | |
| .ok()?; | |
| let (rest, _) = alt(( | |
| tag::<_, _, OracleError<'_>>(" counters, they get a number of "), | |
| tag(" counter, they get a number of "), | |
| )) | |
| .parse(rest)?; |
References
- Universal Review Lenses L2 (Sibling coverage) requires checking if plural/singular/possessive/negated variants are covered when extending parser arms or strings. (link)
- Avoid swallowing all errors (e.g., using
.ok()) when only specific errors are expected to be handled or ignored. Propagate unexpected errors (such as invariant violations or invalid actions) to prevent masking critical bugs.
Parse changes introduced by this PR · 1 card(s), 4 signature(s) (baseline: main
|
1 participant
Both loyalty abilities parsed to Unimplemented.
[-9] 'If target player has fewer than nine poison counters, they get a number of
poison counters equal to the difference': split_leading_conditional severed the
threshold from the body, and the body's 'a number of ... equal to the difference'
had no literal count for try_parse_player_counter. A new whole-sentence oracle_nom
rider (player_counter_difference) dispatched in parse_effect_chain_ir BEFORE the
conditional split captures the threshold N and emits GivePlayerCounter with a
ClampMin{0, Offset{9, Multiply{-1, Ref(TargetControllerCounter{Poison})}}} count =
max(0, 9 - current), bound to the same chosen target player. condition:None — the
clamp encodes the 'if fewer than' gate (>=9 -> 0 -> no-op). parse_player_counter_kind
is consolidated into oracle_nom/primitives.rs as the single kind authority.
[-2] 'Target creature becomes a Treasure artifact with {T},Sac:Add any color and
loses all other card types and abilities': ordered ContinuousModification set
(SetCardTypes[Artifact], RemoveAllSubtypes{Creature}+{Artifact} BEFORE AddSubtype
so Treasure survives per CR 613.7a written-order, RemoveAllAbilities before the
granted ability per CR 613.8a).
No new engine variants. CR 107.1b/122.1/122.1f/205.1a/613.1d/613.1f/613.7a/613.8a.