Fix Yuriko, the Tiger's Shadow - #808
Conversation
Bare-anaphoric possessives ("that card's mana value", "that creature's
power") in chained triggered-ability instructions parsed to
`ObjectScope::CostPaidObject`, whose runtime fallback ordered the
trigger-event source ahead of the reveal/move-captured
`effect_context_object`. For Yuriko's "reveal the top card... each
opponent loses life equal to that card's mana value", that meant the
opponent lost life equal to the attacking Ninja's mana value instead of
the revealed card's.
Route the bare-anaphoric class to `ObjectScope::Anaphoric` (CR 608.2c
earlier-instruction referent), whose inverse slot order reads the
`effect_context_object` first. The participle-possessive class
("the sacrificed creature's power") still routes to `CostPaidObject`
(CR 608.2k cost / trigger-condition referent).
Refactor `is_event_context_referent: bool` to
`classify_possessive_referent: Option<ObjectScope>` so the possessive-
split arm picks the right scope per the prefix's grammatical role. Extend
`delayed_trigger::snapshot_quantity_ref` to bake `Anaphoric` against the
parent's first object target alongside `CostPaidObject` and `Target`, so
Mana Drain's delayed mana refund keeps working after the parse-shape
change.
Class-not-card: the fix moves 87 cards beyond Yuriko (Mana Drain, Dark
Confidant phase-rs#511, Reanimate, Calibrated Blast, Explosive Revelation,
Twisted Justice, Vendetta, Devour Flesh, etc.) onto the correct
`Anaphoric` runtime arm. Allowlist guard expanded 156 -> 244.
CR references verified against docs/MagicCompRules.txt: CR 119.2, 119.3,
202.3, 510.1b, 603.2, 603.7c, 608.2c, 608.2h, 608.2k, 701.20, 701.20b.
There was a problem hiding this comment.
Code Review
This pull request addresses a resolution bug for bare anaphoric possessives, such as those found on Yuriko, the Tiger's Shadow, by introducing a specialized classifier that correctly routes these phrases to the Anaphoric object scope. The changes include updates to the delayed trigger snapshot logic, a significant expansion of the anaphoric scope allowlist guard, and new integration tests. Feedback highlights a logic error in the participle matching implementation that could lead to false positives for player-based possessives and notes missing participles and card types; a refactor using more robust nom combinators is suggested to align with the project's architectural standards.
| let participle_with_boundary = nom::sequence::pair( | ||
| alt(( | ||
| tag::<_, _, OracleError<'_>>("destroyed"), | ||
| tag("countered"), | ||
| tag("returned"), | ||
| tag("targeted"), | ||
| tag("revealed"), | ||
| tag("drawn"), | ||
| tag("copied"), | ||
| )), | ||
| tag(" "), | ||
| ); | ||
| if value((), participle_with_boundary).parse(rest).is_ok() { | ||
| return Some(ObjectScope::CostPaidObject); | ||
| } | ||
|
|
||
| // CR 608.2c: bare anaphoric — "that <type>" / "the <type>" with no | ||
| // participle adjective in between. The type word must be the entire | ||
| // remainder (no trailing modifiers), which `all_consuming` enforces. | ||
| let bare_type = alt(( | ||
| tag::<_, _, OracleError<'_>>("creature"), | ||
| tag("spell"), | ||
| tag("card"), | ||
| tag("permanent"), | ||
| tag("artifact"), | ||
| tag("enchantment"), | ||
| tag("planeswalker"), | ||
| tag("land"), | ||
| )); | ||
| if nom::combinator::all_consuming(value((), bare_type)) | ||
| .parse(rest) | ||
| .is_ok() | ||
| { | ||
| return Some(ObjectScope::Anaphoric); | ||
| } |
There was a problem hiding this comment.
[HIGH] Logic error in participle matching allows false positives on player-possessives and misses documented cost-referents. Evidence: crates/engine/src/parser/oracle_quantity.rs:891. Why it matters: The current implementation of participle_with_boundary only verifies that the prefix starts with a participle and a space, but it does not validate that the remainder of the string is a valid object type. This causes phrases like "the targeted player" to incorrectly classify as ObjectScope::CostPaidObject (which only applies to objects per CR 608.2k), potentially leading to runtime resolution errors. Additionally, the participle list is missing "sacrificed", "exiled", and "discarded" despite being documented as handled (line 873), and the bare_type list is missing the "battle" permanent type (CR 110.4). Suggested fix: Refactor the classifier to use all_consuming and verify the type word follows the participle using modular nom-based combinators to avoid fragile string matching.
let bare_type = alt((
tag::<_, _, OracleError<'_>>("creature"),
tag("spell"),
tag("card"),
tag("permanent"),
tag("artifact"),
tag("enchantment"),
tag("planeswalker"),
tag("land"),
tag("battle"),
));
let participle = alt((
tag::<_, _, OracleError<'_>>("destroyed"),
tag("countered"),
tag("returned"),
tag("targeted"),
tag("revealed"),
tag("drawn"),
tag("copied"),
tag("sacrificed"),
tag("exiled"),
tag("discarded"),
));
if all_consuming(pair(
participle,
pair(tag(" "), bare_type.clone()),
))
.parse(rest)
.is_ok()
{
return Some(ObjectScope::CostPaidObject);
}
// CR 608.2c: bare anaphoric — "that <type>" / "the <type>" with no
// participle adjective in between. The type word must be the entire
// remainder (no trailing modifiers), which `all_consuming` enforces.
if all_consuming(value((), bare_type)).parse(rest).is_ok() {
return Some(ObjectScope::Anaphoric);
}References
- Composable building blocks — every new enum variant, parser arm, effect handler, or filter handles a category of cards, not a single card. No special cases dressed as primitives. (link)
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
- Avoid verbatim string equality for parsing Oracle phrases. Instead, decompose compound phrases into modular, reusable parsers and compose them using idiomatic combinator aggregates like nested
altandtagsequences.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes a scoping bug for bare anaphoric possessives, such as "that card's mana value," by correctly routing them to ObjectScope::Anaphoric instead of CostPaidObject. This ensures that effects like Yuriko's combat damage trigger correctly reference the revealed card rather than the trigger source. The implementation introduces a new possessive referent classifier and updates delayed trigger snapshotting logic. Review feedback identifies opportunities to improve the modularity of the object type parser to handle compound types and "token" objects, as well as extending the participle list to include "milled" and "discovered" for better sibling coverage.
| fn parse_possessive_object_type(input: &str) -> OracleResult<'_, ()> { | ||
| value( | ||
| (), | ||
| alt(( | ||
| tag("creature"), | ||
| tag("spell"), | ||
| tag("card"), | ||
| tag("permanent"), | ||
| tag("artifact"), | ||
| tag("enchantment"), | ||
| tag("planeswalker"), | ||
| tag("land"), | ||
| tag("battle"), | ||
| )), | ||
| ) | ||
| .parse(input) | ||
| } |
There was a problem hiding this comment.
[HIGH] parse_possessive_object_type fails to match common multi-word type phrases and the token object type. The previous suggestion of adding verbatim strings for every combination (e.g., "artifact creature") violates the repository rule against fragile matches. Instead, decompose the parser into modular, reusable parsers for constituent parts (subjects like "artifact", "creature") and compose them using idiomatic combinators. This prevents combinatorial explosion and improves maintainability while correctly capturing phrases like "that creature card" or "the sacrificed token".
fn parse_possessive_object_type(input: &str) -> OracleResult<'_, ()> {
let base_type = alt((
tag("artifact"),
tag("creature"),
tag("enchantment"),
tag("planeswalker"),
tag("land"),
tag("battle"),
tag("spell"),
tag("permanent"),
tag("token"),
));
value(
(),
tuple((
separated_list1(tag(" "), base_type),
opt(preceded(tag(" "), tag("card"))),
)),
)
.parse(input)
}References
- Composable building blocks — every new enum variant, parser arm, effect handler, or filter handles a category of cards, not a single card. (link)
- Avoid verbatim string equality for parsing Oracle phrases; decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
| fn parse_possessive_participle(input: &str) -> OracleResult<'_, ()> { | ||
| value( | ||
| (), | ||
| alt(( | ||
| tag("destroyed"), | ||
| tag("countered"), | ||
| tag("returned"), | ||
| tag("targeted"), | ||
| tag("revealed"), | ||
| tag("drawn"), | ||
| tag("copied"), | ||
| tag("sacrificed"), | ||
| tag("exiled"), | ||
| tag("discarded"), | ||
| )), | ||
| ) | ||
| .parse(input) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] parse_possessive_participle is missing several common participles used in possessive referents, such as milled and discovered. Why it matters: Sibling participles like "milled" (e.g., "the milled card's mana value") are part of the same class as "destroyed" or "revealed". Their omission causes these phrases to fall through, leading to inconsistent scope classification. Adding these constituent parts to the alt list ensures better coverage.
fn parse_possessive_participle(input: &str) -> OracleResult<'_, ()> {
value(
(),
alt((
tag("destroyed"),
tag("countered"),
tag("returned"),
tag("targeted"),
tag("revealed"),
tag("drawn"),
tag("copied"),
tag("sacrificed"),
tag("exiled"),
tag("discarded"),
tag("milled"),
tag("discovered"),
tag("chosen"),
tag("named"),
)),
)
.parse(input)
}References
- Sibling coverage: If a parser arm or string was extended, ensure plural / possessive / negated variants are covered. (link)
…rs#808) Three targeted fixes against `parse_possessive_participle` / `parse_possessive_object_type` in `oracle_quantity.rs`: 1. Decompose the object-type combinator. Replace the verbatim 9-string `alt(tag(...))` with `opt(parse_supertype_prefix) + alt(singular_type_words ∪ token)`. Reuses `oracle_nom::target::parse_supertype_prefix` (CR 205.4a) so composed forms like `"the sacrificed legendary creature's power"` parse via the supertype × type axis instead of the cartesian enumeration. Adds the `token` referent (CR 109.1 / CR 110.5) which is not a CR 205 card type but anchors real possessive references. 2. Extend the participle list. Add `milled` (Court of Cunning, Patchwork Automaton, Demilich class) and `discovered` (LCI Discover mechanic). These are the two missing participle-possessive cost / trigger-condition referents Gemini called out. 3. Lock the word-boundary invariant with regression tests. The determiner gate (`"that "` / `"the "`) in `classify_possessive_referent`, combined with `all_consuming((participle, tag(" "), object_type))`, already guarantees that player-possessives (`"an opponent's ..."`) cannot match the participle branch. New tests pin both halves of the contract — positive composition tests for the new participles + supertype + token cases, and negative tests for `"an opponent's"` and plural `creatures'` prefixes. 8 new tests in `parser::oracle_quantity::tests`. All 112 module tests pass; `cargo clippy --all-targets -- -D warnings` clean; coverage holds at 85.33% (29670 / 34770). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
parser: harden possessive participle classifier (#808 followup)
Summary
Fixes a runtime bug in Yuriko, the Tiger's Shadow's combat-damage trigger. The parser was mapping the bare-anaphoric possessive "that card's mana value" to
ObjectScope::CostPaidObject, whose fallback slot order (cost_paid_object → trigger-event source → effect_context_object) read the attacking Ninja's mana value instead of the revealed card's. Routed the bare-anaphoric class toObjectScope::Anaphoric(CR 608.2c) whose inverse slot order reads theeffect_context_object(the revealed-card snapshot stamped by the parentRevealTop → ChangeZonechain) first.Bug report (verbatim): "Yuriko, the Tiger's Shadow: Her on combat damage trigger doesn't do damage properly, it does her mana cost in damage instead of the revealed cards mana cost." (The trigger is technically lose-life, not damage; the reporter conflated the two.)
Class-not-card: refactored
is_event_context_referent: boolintoclassify_possessive_referent: Option<ObjectScope>so the possessive-split arm picks the scope per the prefix's grammatical role. Participle possessives ("the sacrificed creature's power" — CR 608.2k) still route toCostPaidObject. The fix moves 87 cards beyond Yuriko (Mana Drain, Dark Confidant #511, Reanimate, Calibrated Blast, Explosive Revelation, Twisted Justice, Vendetta, Devour Flesh, etc.) onto the correctAnaphoricruntime arm.delayed_trigger::snapshot_quantity_refextended to bakeAnaphoricagainst the parent's first object target alongsideCostPaidObjectandTarget, keeping Mana Drain's delayed mana refund correct after the parse-shape change.Files changed
crates/engine/src/parser/oracle_quantity.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/tests/integration/anaphoric_scope_allowlist_guard.rscrates/engine/tests/integration/yuriko_combat_damage.rs(new)crates/engine/tests/integration/main.rsCR references
Track
Developer
LLM
Model: claude-opus-4-7
Thinking: high
Verification
Independent fresh-context code review (no implementer conversation) covered nom-mandate compliance, CR-citation completeness, pattern coverage (Calibrated Blast / Reanimate / Vendetta spot-checked; Greater Good unregressed), logic placement, building-block reuse, bool-flag avoidance, snapshot baking soundness, test discrimination (Yuriko CMC 3 vs reveal CMC 4; Ingenious Infiltrator CMC 4 vs reveal CMC 2 — both gaps non-colliding), and untouched concurrent work — all 9 checks PASS.
Scope Expansion
The fix is a building-block correction that also unlocks correct runtime behavior for ~87 additional cards beyond Yuriko (Mana Drain, Dark Confidant #511, Reanimate, Calibrated Blast, Explosive Revelation, Twisted Justice, Vendetta, Devour Flesh, Cleric Class, Riddle of Lightning, Sin Prodder, Pain Seer, Erratic Explosion, Tribute to Hunger, et al.). The `Anaphoric` scope and the `effect_context_object` capture pipeline both pre-existed; this PR wires the parser to use them for the bare-anaphoric class. `delayed_trigger::snapshot_quantity_ref` gained an `Anaphoric` arm (13 lines) so Mana Drain's delayed mana refund keeps working — peer to the existing `CostPaidObject` / `Target` arms. Allowlist guard count moved 156 → 244 entries.
Validation Failures
None.
CI Failures
None.