Skip to content

fix(engine): parse and resolve Power Leak's dynamic damage prevention#5742

Merged
matthewevans merged 8 commits into
phase-rs:mainfrom
rykerwilliams:fix/power-leak-dynamic-prevention
Jul 15, 2026
Merged

fix(engine): parse and resolve Power Leak's dynamic damage prevention#5742
matthewevans merged 8 commits into
phase-rs:mainfrom
rykerwilliams:fix/power-leak-dynamic-prevention

Conversation

@rykerwilliams

Copy link
Copy Markdown
Contributor

Summary

Power Leak ({1}{U}, Enchantment — Aura, old-school): "Enchant enchantment\nAt the beginning of the upkeep of enchanted enchantment's controller, that player may pay any amount of mana. This Aura deals 2 damage to that player. Prevent X of that damage, where X is the amount of mana that player paid this way."

This card was flagged under misparse-backlog category #11 ("replacement/instead effect mis-modeled"), but that framing doesn't match the card's Oracle text — there's no "would...instead" clause anywhere in it. Investigation found three distinct, real bugs, plus a separate runtime bug discovered while making the parser fixes actually work end-to-end:

Parser fixes:

  1. The "amount of mana paid this way" where-X recognizer only matched one exact phrasing ("the total amount..."), not Power Leak's/Errant Minion's possessive-pronoun variant or Liege of the Hollows' "they paid this way" variant — the latter had no parse warning at all, since it silently fell through to an unbindable literal-string placeholder.
  2. Power Leak's "deals 2 damage... Prevent X of that damage" was modeled as two separate sequential effects. Since a prevention effect must exist before the damage event it prevents (CR 615.4), and the "prevent the next N" shield this produced is a depleting shield that leaks unconsumed capacity onto later, unrelated damage the same turn, this needed collapsing into one DealDamage with a computed max(2-X, 0) amount — the same "later text modifies an earlier effect" pattern the engine already uses for Destroy's cant_regenerate flag (CR 608.2c).
  3. The "at the beginning of the upkeep of enchanted X's controller" trigger-scope recognizer only matched "creature"/"permanent" nouns, so Power Leak's trigger fired on every player's upkeep instead of just the enchanted permanent's controller's. Fixed by delegating to the existing shared Enchant-noun combinator, incidentally fixing 6 sibling cards (Curse Artifact, Cursed Land, Erosion, Feedback, Power Taint, Warp Artifact).

A separate runtime bug, discovered because the parser fixes alone didn't make Power Leak resolve correctly: TargetFilter::TriggeringPlayer resolves to the wrong player whenever ability resolution pauses mid-chain on a player choice (e.g. Power Leak's own "may pay any amount of mana") and then resumes. Root cause: stack::resolve_top unconditionally clears the live trigger-event context once its stack entry leaves the stack, regardless of whether the ability chain actually finished or merely paused into a continuation — so by the time a paused chain resumes, the context needed to resolve "that player" is gone and resolution falls back to the ability's own controller instead. Confirmed via a real A/B test: Warp Artifact (same trigger shape, no payment pause) correctly damages the enchanted controller; Power Leak (same shape, but with a payment pause) incorrectly damaged the Aura's own controller instead.

Fixed by extending an existing type (ResolvingTriggerContext, already used by one other pause mechanism) with its one missing field, giving the paused-continuation carrier a single snapshotted field of that type, and wiring the continuation-resume path to push that snapshot as the live context before resuming and restore it afterward — mirroring a save/restore pattern already shipped for an analogous pause type. Two other, narrower pre-existing mechanisms solve the same conceptual problem for two other pause types and were deliberately left untouched (documented in place); consolidating all three is a real, identified follow-up, deferred here to keep this fix's blast radius on the reported bug.

Test plan

  • Parser tests: Power Leak/Errant Minion's PreventDamage fold to a computed DealDamage amount with no leftover node; Liege of the Hollows' token count binds to the shared X variable instead of a dead placeholder; the 5 existing Join Forces cards' "total amount" binding is unregressed; Warp Artifact/Cursed Land/Feedback's trigger scoping generalizes correctly.
  • Runtime tests: a stolen/damaged creature's owner-of-record resolves correctly across the accept-and-pay pause (fails pre-fix with damage landing on the Aura's controller instead); the decline path and the no-pause control path are both covered as siblings.
  • A dedicated regression test for a nested-pause carry-over edge case in the continuation-merge path, with a live-revert-and-rerun proof.
  • Full cargo test -p engine --lib: 16491 passed, 0 failed.
  • cargo clippy -p engine --lib --tests -- -D warnings clean.
  • cargo fmt --all.
  • Parser combinator gate clean.
  • All CR citations verified against the Comprehensive Rules text.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K

rykerwilliams added a commit to rykerwilliams/phase that referenced this pull request Jul 13, 2026
…fixed, note deferred B/C consolidation follow-up

@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 implements CR 608.2 by snapshotting and replaying the resolving ability's trigger-event context across interactive pauses, ensuring correct resolution of filters like TriggeringPlayer. It also introduces a parser optimization to fold the 'deal N damage, then prevent X' idiom into a single computed-amount node, generalizes the 'amount of mana paid this way' parser, and expands Aura phase trigger scoping to all enchant types. Feedback recommends correcting a CR annotation format in choose_from_zone.rs to comply with Rule R6, and updating ResolvingTriggerContext::capture to check current_trigger_events for complete correctness under batched triggers.

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 +111 to +112
// Forced field-threading for the new plural-event list (CR 603.2c);
// mechanism-B behavior is otherwise unchanged.

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.

medium

The CR annotation in this comment does not follow the mandatory CR <number>: <description> format specified in the repository style guide (Rule R6). Please reformat it to ensure strict adherence and consistency across the codebase.

Suggested change
// Forced field-threading for the new plural-event list (CR 603.2c);
// mechanism-B behavior is otherwise unchanged.
// CR 603.2c: Forced field-threading for the new plural-event list;
// mechanism-B behavior is otherwise unchanged.
References
  1. Every rules-touching line of engine code must carry a comment of the form CR <number>: <description> (regex CR \d{3}(\.\d+[a-z]?)?). (link)

Comment on lines +6646 to +6648
(state.current_trigger_event.is_some()
|| state.current_trigger_match_count.is_some()
|| state.die_result_this_resolution.is_some())

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.

medium

For complete correctness and defensive programming under batched triggers, ResolvingTriggerContext::capture should also check if current_trigger_events is non-empty. This ensures that a live trigger context is captured even if the singular current_trigger_event is not set.

        (state.current_trigger_event.is_some()
            || !state.current_trigger_events.is_empty()
            || state.current_trigger_match_count.is_some()
            || state.die_result_this_resolution.is_some())
References
  1. Verify code functionality, handle edge cases, and ensure appropriate checks exist before accessing or ignoring state.

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 9 card(s), 5 signature(s) (baseline: main bd30ee969e91)

7 card(s) · trigger/Phase · field valid target: parent target's controller

Examples: Curse Artifact, Cursed Land, Erosion (+4 more)

2 card(s) · ability/DealDamage · field amount: 2max(-1*X+2, 0)

Examples: Errant Minion, Power Leak

2 card(s) · ability/PreventDamage · removed: PreventDamage (amount=Next(1), scope=AllDamage, target=any target)

Examples: Errant Minion, Power Leak

1 card(s) · ability/Token · added: Token (token=X× +1/+1 Green Squirrel (Creature Squirrel))

Examples: Liege of the Hollows

1 card(s) · ability/create · removed: create

Examples: Liege of the Hollows

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

rykerwilliams added a commit to rykerwilliams/phase that referenced this pull request Jul 14, 2026
@matthewevans matthewevans self-assigned this Jul 14, 2026
@matthewevans
matthewevans force-pushed the fix/power-leak-dynamic-prevention branch from 116412d to f0af1d5 Compare July 14, 2026 07:21

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocked — the new behavior has no production-pipeline regression test.

🔴 Blocker

  • crates/engine/src/game/effects/pay.rs:1861 tests the context restoration by constructing a ResolvedAbility, calling resolve_ability_chain, and manually clearing the trigger context at lines 159–166. That reaches the new drain code, but it bypasses both the production parser and stack::resolve_top—the two boundaries this PR changes and relies on.
  • Power Leak’s official Oracle text is: “At the beginning of the upkeep of enchanted enchantment's controller, that player may pay any amount of mana. This Aura deals 2 damage to that player. Prevent X of that damage, where X is the amount of mana that player paid this way.” The current parser tests validate AST shape, and the helper test validates a synthesized chain, but neither proves that the real parsed trigger is scoped to the enchanted controller, survives the stack/pause boundary, and applies max(2 − X, 0) to that player.

✅ Clean

  • The PendingContinuation snapshot/restore is at the shared continuation seam, and the parser diff’s Power Leak / Errant Minion / Liege scope is explained by the reviewed Oracle forms.

Recommendation: add a registered integration test that drives the real parsed Power Leak through an upkeep trigger, accepts a nonzero mana payment, and asserts the enchanted controller—not the Aura controller—takes exactly 2 − X damage. Include a decline or X ≥ 2 sibling case if the fixture makes it cheap; then re-request review.

@matthewevans matthewevans added the bug Bug fix label Jul 14, 2026
@matthewevans matthewevans removed their assignment Jul 14, 2026
@rykerwilliams

Copy link
Copy Markdown
Contributor Author

Added crates/engine/tests/integration/power_leak_dynamic_prevention.rs, addressing the specific gap flagged above: it parses Power Leak's real Oracle text, advances a two-player scenario to the enchanted permanent's controller's actual upkeep, and drives the real WaitingFor::OptionalEffectChoice → PayAmountChoice → SubmitPayAmount sequence through stack::resolve_top — no hand-built ability chain, no manual trigger-context clearing.

Three cases:

  • Pay 1 mana → the enchanted controller (not the Aura's controller) loses exactly 1 life (max(2-1,0)).
  • Decline → the enchanted controller loses the full 2.
  • Pay 3 mana → the enchanted controller loses 0 (clamped at the upper edge).

Live-reverted the drain_pending_continuation fix locally to confirm this new test actually catches the original bug: with the fix disabled, the first case fails with the enchanted controller taking 0 damage and the Aura's own controller taking it instead — the exact defect this PR closes. Restored and re-verified all three tests + the full --lib/--test integration suites are green.

The existing pay.rs helper tests are left in place as defense-in-depth; this is additive.

Re-requesting review.

@matthewevans matthewevans self-assigned this Jul 15, 2026
rykerwilliams and others added 7 commits July 15, 2026 00:33
Power Leak ("that player may pay any amount of mana. This Aura deals 2
damage to that player. Prevent X of that damage, where X is the
amount of mana that player paid this way") had three distinct parser
bugs, none of which matched the misparse-backlog's original
"replacement effect" framing for this card:

- The where-X recognizer for "the amount of mana paid this way" only
  matched one exact phrasing ("the total amount..."), not Power Leak's
  or Errant Minion's possessive-pronoun variant, or Liege of the
  Hollows' silently-broken "they paid this way" variant (which had no
  parse warning at all, since it fell through into a literal-string
  Variable that could never bind).
- The card's "This Aura deals 2 damage... Prevent X of that damage"
  was modeled as two separate sequential effects. Since prevention
  effects must exist before the damage event they prevent (CR 615.4),
  and PreventDamage's "prevent the next N" shield is a depleting
  shield that leaks unconsumed capacity onto later, unrelated damage
  this turn, this shape was more than a simple wrong-target bug — it
  needed collapsing into a single DealDamage with a computed
  max(2-X, 0) amount, the same "later text modifies an earlier
  effect" pattern the engine already uses for Destroy's
  cant_regenerate flag (CR 608.2c).
- The "at the beginning of the upkeep of enchanted X's controller"
  trigger-scope recognizer only matched "creature"/"permanent" nouns,
  so Power Leak's trigger fired on every player's upkeep instead of
  just the enchanted permanent's controller's. Fixed by delegating to
  the existing shared Enchant-noun combinator, incidentally fixing 6
  sibling cards (Curse Artifact, Cursed Land, Erosion, Feedback,
  Power Taint, Warp Artifact).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K
…on resume

Discovered while making Power Leak's parser fixes actually work at
runtime: TargetFilter::TriggeringPlayer (and the other 11
is_pure_event_context_filter variants, plus PlayerFilter/QuantityRef
readers of the same fields) resolves to the wrong player whenever
ability resolution pauses mid-chain on a player choice and then
resumes via PendingContinuation. stack::resolve_top unconditionally
clears current_trigger_event and its siblings once the stack entry
that started a resolution leaves the stack (CR 603.7c cleanup),
regardless of whether that resolution's ability chain actually
finished or merely paused into a continuation — so by the time a
paused chain resumes, the context needed to resolve "that player" is
already gone, and resolution falls back to the ability's own
controller instead.

Confirmed via a real A/B test: Warp Artifact (same trigger shape, no
payment pause) correctly damages the enchanted permanent's controller;
Power Leak (same shape, but with a "may pay any amount of mana" pause
first) incorrectly damaged the Aura's own controller instead.

Fixed by extending the existing ResolvingTriggerContext type (already
used by one other pause mechanism) with its one missing field, giving
PendingContinuation a single snapshotted field of that type captured
at construction time, and wiring drain_pending_continuation to push
that snapshot as the live context before resuming a paused chain and
restore it afterward — mirroring the save/restore pattern already
shipped for WaitingFor::UnlessPayment's identical pause/resume shape.

Two other, narrower pre-existing mechanisms solve the same conceptual
problem for two other pause types and were deliberately left
untouched (documented at ResolvingTriggerContext's definition);
consolidating all three is a real, identified follow-up, deferred
here to keep this fix's blast radius on the reported bug rather than
bundling a deletion of currently-working code into a user-facing fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K
…resolution

The prior commit's PendingContinuation.trigger_context fix (restoring
the enclosing trigger's event context across a paused/resumed ability
chain) had a real side effect on CR 603.12 reflexive triggers: "may
[cost]. When you do, [effect using an EventContextAmount 'that many'
bound]" abilities now saw the enclosing trigger's own event-amount
(e.g. an attack trigger's attacker count) win over the reflexive's own
resolution-local count (e.g. the actual number of Treasures
sacrificed), because that enclosing event is correctly restored before
the reflexive's target slots get built.

Confirmed via CI regression: Swashbuckler Extraordinaire's "Whenever
you attack, you may sacrifice one or more Treasures. When you do, up
to that many target creatures gain double strike" started offering
only 1 target slot (the attacker count) instead of 2 (the sacrifice
count).

Fixed by suppressing only the two "read the enclosing trigger's own
event" tiers of the EventContextAmount cascade for the entire dynamic
extent of a reflexive's target-slot construction and count-freezing
(CR 603.12: a reflexive's own "that many" is resolution-local to the
ability that created it, never the enclosing trigger), via a
thread-local scope mirroring an existing analogous mechanism in the
same file. Every other tier of the cascade is untouched, and the
scope is set at exactly one call site, so no other consumer of this
shared resolution cascade is affected.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K
Addresses review feedback on this PR: the existing runtime tests for
the TriggeringPlayer-across-pause fix (in game/effects/pay.rs) drove a
hand-built ability chain through resolve_ability_chain directly,
bypassing both the real Oracle parser and the real stack::resolve_top
boundary — the two seams this PR's fixes actually depend on.

Adds a new integration test driving Power Leak's real parsed Oracle
text through a genuine upkeep trigger (real GameRunner phase
advancement, real OptionalEffectChoice/PayAmountChoice/SubmitPayAmount
round-trip), asserting the enchanted permanent's controller — never
the Aura's own controller — takes exactly max(2-X, 0) damage, at three
points: a nonzero payment, a full decline, and an overpayment that
fully prevents the damage.

The pre-existing pay.rs helper tests are left in place as
defense-in-depth; this test is additive, closing the specific gap the
review identified.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K
Co-authored-by: Rajah James <rajah.james@grantek.com>
Co-authored-by: Rajah James <rajah.james@grantek.com>
Co-authored-by: Rajah James <rajah.james@grantek.com>
@matthewevans
matthewevans force-pushed the fix/power-leak-dynamic-prevention branch from 9ee7d22 to 3391235 Compare July 15, 2026 07:41
Keep the reflexive trigger-context isolation while restoring the EventContextAmount order: batched match count, then die result, then scalar trigger event.\n\nCo-authored-by: Rajah James <rajah.james@grantek.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verdict: Approved

🔴 Blocker

  • None.

🟡 Non-blocking

  • Gemini's plural current_trigger_events capture concern is addressed at crates/engine/src/types/game_state.rs; its CR-format concern is not valid because compound CR citations are the repository convention.

✅ Clean

  • The registered Power Leak integration test drives the real parsed upkeep trigger through stack::resolve_top, the optional-payment pause, and SubmitPayAmount, including decline and full-prevention siblings.
  • The current parse-diff matches the claimed 9-card scope: seven Aura phase triggers, Power Leak/Errant Minion's computed damage, and Liege of the Hollows' X-token binding.
  • The maintainer fix restores EventContextAmount precedence—batched match count, die result, then scalar trigger event—and the fresh CI run is green on c0fba2f.

Recommendation: approve and enqueue.

@matthewevans
matthewevans added this pull request to the merge queue Jul 15, 2026
@matthewevans matthewevans removed their assignment Jul 15, 2026
Merged via the queue into phase-rs:main with commit ed5de28 Jul 15, 2026
13 checks passed
rykerwilliams added a commit to rykerwilliams/phase that referenced this pull request Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants