Fix Twilight Prophet caster gaining 0 life from the revealed card's mana value#4698
Fix Twilight Prophet caster gaining 0 life from the revealed card's mana value#4698GildardoDev wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses Issue #1375 by routing 'that card's mana value' (and its synonym 'converted mana cost') to bind to ObjectScope::Demonstrative instead of ObjectScope::Target, ensuring correct resolution of anaphoric references for cards like Twilight Prophet. It includes comprehensive unit and integration tests to verify this behavior and prevent regressions on targeted effects like Living Armor. Feedback on the changes highlights a violation of the repository's parsing rules (R1) due to the verbatim string matching in is_that_card_mana_value_where_x, and suggests refactoring it into modular nom combinators while using the cleaned expression to handle trailing punctuation.
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.
| if is_that_card_mana_value_where_x(expression_lower.as_str()) { | ||
| return parse_event_context_quantity(where_x_expression); | ||
| } |
There was a problem hiding this comment.
Avoid verbatim string equality checks like is_that_card_mana_value_where_x for parsing Oracle phrases, as this 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.
Additionally, ensure that the cleaned expression is used instead of where_x_expression when calling quantity parsers to avoid parsing failures due to trailing periods.
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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
…ains the right life
41c8506 to
9d7392e
Compare
Parse changes introduced by this PR · 10 card(s), 8 signature(s) (baseline: main
|
Anchored on: #1375
Problem: Twilight Prophet's Ascend upkeep — "reveal the top card of your library and put it into your hand. Each opponent loses X life and you gain X life, where X is that card's mana value" — gives the caster the wrong life total: you gain 0 instead of the revealed card's mana value. (The "each opponent loses X" half happens to work, but only by a target-slot coincidence; the "you gain X" half is observably broken.)
Note on the report: #1375 was filed as "not triggering at my upkeep, I do have ascend." The trigger firing itself is actually fine on current
main— Ascend grants the city's blessing at ten or more permanents, the "if you have the city's blessing" intervening-if evaluates, and the ability resolves and reveals the card. The observable defect the player hit is this drain: the reveal happens but the caster's life doesn't change (gain 0), which reads as "nothing happened at upkeep." This PR fixes that root cause.Root cause: The "where X is that card's mana value" binding fell through to
parse_cda_quantity, which hard-maps "that card's mana value" toQuantityRef::ObjectManaValue { scope: ObjectScope::Target }. The upkeep trigger has no object target — the revealed card is an anaphoric reference, not a target (CR 115.10a). The chainedLoseLifecoincidentally finds the revealed card in a propagated target slot, but the deeper chainedGainLifehas an empty target slot, soObjectScope::Targetreads nothing there and X resolves to 0 for the caster.Fix: A nom guard in
parse_where_x_quantity_expressionroutes only the literal "that card's mana value" (and its "converted mana cost" synonym) throughparse_event_context_quantity, so the anaphoric referent bindsObjectScope::Demonstrative— resolved against the revealed card via the LKI-snapshottedeffect_context_object(CR 608.2c, CR 202.3). "that spell's mana value" (EventSource) and "that creature's/permanent's mana value" (Target, which is correct for targeted cards like Feeding Grounds) are deliberately excluded, and power/toughness is untouched. Class-general across the "that card's mana value" cards; the ambiguous demonstrative-sense creature references (e.g. Aether Mutation) are a separate antecedent-disambiguation problem and are intentionally not addressed here.Tests:
Verification: cargo fmt, cargo check -p engine, cargo clippy -p engine --lib -D warnings, ./scripts/check-parser-combinators.sh, and ./scripts/check-skill-doc.sh all clean. Both guard-toggle revert-fails proven. Regression green: quantity 675, mana_value 193, object_mana 10, plus the Dark Confidant and Yuriko integration tests. No new engine variant; zero net test-fixture change.
Closes #1375.
Model: claude-opus-4-8
Tier: Frontier