Skip to content

fix(parser): recognize "dealt damage to another creature this turn" intervening-if#5726

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
real-venus:fix/dealt-damage-to-creature-intervening-if
Jul 13, 2026
Merged

fix(parser): recognize "dealt damage to another creature this turn" intervening-if#5726
matthewevans merged 1 commit into
phase-rs:mainfrom
real-venus:fix/dealt-damage-to-creature-intervening-if

Conversation

@real-venus

Copy link
Copy Markdown
Contributor

Bug: Wolverine, Best There Is ("At the beginning of each end step, if Wolverine dealt damage to another creature this turn, put a +1/+1 counter on him") parsed with condition: null — the intervening-if was swallowed, so the counter was placed unconditionally.

Fix: Extend the dealing-direction source-dealt-damage condition combinator's target axis with "another creature" / "a creature" (Typed{Creature, [Another]}), alongside the existing player/opponent targets. The clause now lifts to TriggerCondition::QuantityComparison over QuantityRef::DamageDealtThisTurn(SelfRef → another creature) ≥ 1; the runtime damage-record target matcher already handles creature targets. A pure-addition extension of one existing combinator — no new variant, no runtime change. Verified locally: both new parser tests pass, 96 dealt-family sibling tests stay green, clippy clean.

CR 120.1 (source of damage), CR 603.4 (intervening-if).

@real-venus
real-venus requested a review from matthewevans as a code owner July 13, 2026 07:31

@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 extends the parser in crates/engine/src/parser/oracle_nom/condition.rs to support parsing damage dealt to creatures (e.g., "another creature", "a creature") in addition to players and opponents, and adds corresponding unit tests. The review feedback suggests renaming the extended function parse_source_dealt_damage_to_opponent_this_turn to reflect its broader scope, and adding sibling coverage for planeswalker targets to ensure completeness.

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 +403 to +404
// player/opponent/creature this turn" — the *dealing* direction (source = the
// ability's own permanent). "it" is the source anaphor for triggered-ability

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

[MEDIUM] Misleading function name after extending target types. Evidence: crates/engine/src/parser/oracle_nom/condition.rs:403. Why it matters: The function now parses players, opponents, and creatures, making the "to_opponent" suffix in the name misleading and harder to find. Suggested fix: Rename the function to parse_source_dealt_damage_this_turn and update its call sites.

References
  1. Pillar 1: Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. Clean, accurate naming is essential for maintainability. (link)

Comment on lines +427 to +434
value(
TargetFilter::Typed(TypedFilter::creature().properties(vec![FilterProp::Another])),
tag("another creature"),
),
value(
TargetFilter::Typed(TypedFilter::creature()),
alt((tag("a creature"), tag("creature"))),
),

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

[MEDIUM] Missing sibling coverage for planeswalker targets. Evidence: crates/engine/src/parser/oracle_nom/condition.rs:427. Why it matters: To ensure composable building blocks (Pillar 3) and complete sibling coverage (L2), the parser should also support damage dealt to planeswalkers, which are also damageable permanents. Suggested fix: Add similar parser arms for "another planeswalker" and "a planeswalker" / "planeswalker".

References
  1. Pillar 3: 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)

@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown

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

1 card(s) · trigger/Phase · field condition: sum damage dealt this turn (self -> another creature) [ungrouped] ≥ 1

Examples: Wolverine, Best There Is

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

@matthewevans matthewevans self-assigned this Jul 13, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 13, 2026

@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.

Request changes — the parser extension is on the existing damage-history seam, but this behavioral fix needs an end-to-end trigger test and the current head fails Rust lint.

🔴 Blocker

  • crates/engine/src/parser/oracle_trigger_tests.rs:1501-1503 fails clippy::doc_lazy_continuation in the current CI run: the doc lines following Wolverine's are interpreted as an unindented list. Please make the comment a valid paragraph (or use normal // comments) and rerun the required lint gate.
  • crates/engine/src/parser/oracle_trigger_tests.rs:1505-1544 proves only the parsed TriggerDefinition. This change governs an intervening-if's trigger detection and resolution, so add a GameScenario/actual apply-path regression: Wolverine deals damage to another creature → its end-step trigger puts the counter on Wolverine, plus a negative case where it dealt no qualifying damage and no counter is placed. Parser-shape tests alone would still pass if that runtime path stopped evaluating DamageDealtThisTurn(SelfRef → Another creature).

🟡 Non-blocking

  • crates/engine/src/parser/oracle_nom/condition.rs:400 still names the now-broader helper parse_source_dealt_damage_to_opponent_this_turn; rename it to describe its source-dealt-damage scope.

✅ Clean

  • The parse-diff artifact for this head shows exactly the intended one-card condition change (Wolverine), and the new Another filter is evaluated source-relatively by the existing damage-record matcher.

Recommendation: request changes — fix CI and add the discriminating runtime regression, then rebase onto current main for a fresh review.

@matthewevans matthewevans removed their assignment Jul 13, 2026
@real-venus
real-venus force-pushed the fix/dealt-damage-to-creature-intervening-if branch from 33613ab to ed8a005 Compare July 13, 2026 09:17
@matthewevans matthewevans self-assigned this Jul 13, 2026
…ntervening-if

Wolverine, Best There Is ("At the beginning of each end step, if Wolverine
dealt damage to another creature this turn, put a +1/+1 counter on him")
parsed with condition: null -- the intervening-if was swallowed, so the
counter was placed unconditionally.

Extend the dealing-direction source-dealt-damage condition combinator
(renamed parse_source_dealt_damage_this_turn) target axis with "another
creature"/"a creature" (Typed{Creature, [Another]}), alongside the existing
player/opponent targets. The clause lifts to TriggerCondition::QuantityComparison
over QuantityRef::DamageDealtThisTurn(SelfRef -> another creature) >= 1; the
runtime damage-record target matcher already handles creature targets.

Tests: 2 parser AST tests + a GameScenario runtime regression (Wolverine deals
combat damage to a creature -> end-step trigger places the counter; negative:
damage to a player only -> no counter).

CR 120.1 (source of damage), CR 603.4 (intervening-if).
@matthewevans
matthewevans force-pushed the fix/dealt-damage-to-creature-intervening-if branch from ed8a005 to cb09bcd Compare July 13, 2026 09:28

@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.

Approved — the current head resolves the prior blockers and is ready for the merge queue.

🔴 Blocker

  • None.

🟡 Non-blocking

  • Gemini's planeswalker suggestion is intentionally not expanded here: the official Scryfall exact-phrase searches return no cards, while the current parse artifact is exactly Wolverine.

✅ Clean

  • The helper was renamed for its widened source-damage scope.
  • The current parse artifact changes exactly Wolverine's intervening-if condition.
  • The paired GameScenario regressions prove both the qualifying creature-damage path and the non-vacuous player-damage negative path through end-step trigger resolution.
  • All required current-head GitHub checks are green.

Recommendation: approve and enqueue through the merge queue.

@matthewevans
matthewevans added this pull request to the merge queue Jul 13, 2026
@matthewevans matthewevans removed their assignment Jul 13, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 13, 2026
@matthewevans matthewevans self-assigned this Jul 13, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 13, 2026
@matthewevans matthewevans removed their assignment Jul 13, 2026
Merged via the queue into phase-rs:main with commit 823c29c Jul 13, 2026
13 checks passed
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