Skip to content

Fix Twilight Prophet caster gaining 0 life from the revealed card's mana value#4698

Open
GildardoDev wants to merge 1 commit into
phase-rs:mainfrom
GildardoDev:fix/twilight-prophet-that-card-mana-value
Open

Fix Twilight Prophet caster gaining 0 life from the revealed card's mana value#4698
GildardoDev wants to merge 1 commit into
phase-rs:mainfrom
GildardoDev:fix/twilight-prophet-that-card-mana-value

Conversation

@GildardoDev

Copy link
Copy Markdown
Contributor

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" to QuantityRef::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 chained LoseLife coincidentally finds the revealed card in a propagated target slot, but the deeper chained GainLife has an empty target slot, so ObjectScope::Target reads nothing there and X resolves to 0 for the caster.

Fix: A nom guard in parse_where_x_quantity_expression routes only the literal "that card's mana value" (and its "converted mana cost" synonym) through parse_event_context_quantity, so the anaphoric referent binds ObjectScope::Demonstrative — resolved against the revealed card via the LKI-snapshotted effect_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:

  • Runtime (live-parsed, so the parser guard is load-bearing): P0 with the city's blessing reveals a mana-value-5 card at upkeep; asserts the caster gains 5 and each opponent loses 5. Reverting the guard makes the caster gain 0 → the gain assertion fails.
  • Runtime no-regression: a live-parsed "put X counters on target creature, where X is that creature's mana value" card still resolves X against its target.
  • Parser: "that card's mana value" → Demonstrative (revert → Target); "that spell's mana value" → EventSource preserved; "that creature's mana value" → Target preserved; full-card Twilight Prophet parse asserts both drains bind Demonstrative.

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

@GildardoDev GildardoDev requested a review from matthewevans as a code owner July 1, 2026 09:32

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +7043 to +7045
if is_that_card_mana_value_where_x(expression_lower.as_str()) {
return parse_event_context_quantity(where_x_expression);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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
  1. 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 alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

@GildardoDev GildardoDev force-pushed the fix/twilight-prophet-that-card-mana-value branch from 41c8506 to 9d7392e Compare July 1, 2026 09:49
@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 10 card(s), 8 signature(s) (baseline: main a3e09d5acd58)

4 card(s) · ability/Pump · field p/t: +target's mana value/+0+self mana value/+0

Examples: Cait Sith, Fortune Teller, Goblin Machinist, Stormchaser Chimera (+1 more)

3 card(s) · ability/LoseLife · field amount: target's mana valueself mana value

Examples: Nyla, Shirshu Sleuth, Pact Weapon, Twilight Prophet

1 card(s) · ability/GainLife · field amount: target's mana valueself mana value

Examples: Twilight Prophet

1 card(s) · ability/Pump · field p/t: +-1*target's mana value/+-1*target's mana value+-1*self mana value/+-1*self mana value

Examples: Putrid Cyclops

1 card(s) · ability/Pump · field p/t: +target's mana value/+-1*target's mana value+self mana value/+-1*self mana value

Examples: Erratic Mutation

1 card(s) · ability/Pump · field p/t: +target's mana value/+target's mana value+self mana value/+self mana value

Examples: Pact Weapon

1 card(s) · ability/PumpAll · field p/t: +target's mana value/+0+self mana value/+0

Examples: Surge to Victory

1 card(s) · ability/Token · field token: target's mana value× Clue (Artifact Clue)self mana value× Clue (Artifact Clue)

Examples: Nyla, Shirshu Sleuth

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Twilight Prophet — Not triggering at my upkeep, I do have ascend

1 participant