Skip to content

fix(parser): DamageDone trigger leaves valid_target=null, so match_damage_done skips the target/owne - #3469

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
ntindle:fix/who-misparse-2-damagedone-recipient-gating
Jun 16, 2026
Merged

fix(parser): DamageDone trigger leaves valid_target=null, so match_damage_done skips the target/owne#3469
matthewevans merged 2 commits into
phase-rs:mainfrom
ntindle:fix/who-misparse-2-damagedone-recipient-gating

Conversation

@ntindle

@ntindle ntindle commented Jun 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes a parser misparse affecting 2 card(s) in the Doctor Who Commander precons.

Root cause: DamageDone trigger leaves valid_target=null, so match_damage_done skips the target/owner check and fires on damage to any object/player; the 'to a creature' / 'to its owner' qualifier is dropped.

Cards corrected

  • The Beast, Deathless Prince
  • Strax, Sontaran Nurse

Fix

Implemented WHO misparse cluster #2 (DamageDone trigger recipient-gating) surgically on upstream/main per the approved v3 plan. All 51-card "deals [combat] damage to a [type]" object-recipient class and the "to its owner" relational class now parse and gate correctly.

WHAT CHANGED (5 files):

  1. trigger_matchers.rs — Step 0a: added a player-arm guard to match_damage_done's TargetRef::Player arm; Step 0b: identical guard in matching_combat_damage_to_player_sources (the aggregate combat-damage-to-a-player funnel). Both reject a type-bearing (object-only) valid_target on a player recipient, closing the player_matches_filter _ => true leak across BOTH delivery paths (per-event for the 37 SelfRef cards; aggregate for the 14 non-SelfRef listeners).
  2. types/ability.rs — new parameterless TriggerCondition::DamagedPlayerIsEventSourceOwner leaf (The Beast).
  3. triggers.rs — evaluator arm reading GameEvent::DamageDealt: true when target==Player(p) and the damage source object's owner==p.
  4. oracle_trigger.rs — two new nom combinators: parse_object_recipient_filter ("to a creature/permanent/planeswalker", with a peek(tag("or "))-based terminator guard that declines "creature or player"/"creature or opponent") and parse_damage_to_its_owner (relational "to its owner" with a word-boundary guard). Wired into BOTH the subject-led dispatch (owner relation → P/T gate → bare object → player axis) and the source-led second site (Step 4b).
  5. Questing Beast snapshot updated: "to a planeswalker" now correctly scopes valid_target=Typed([Planeswalker]) (was null) — a correct class improvement.

DEVIATION FROM PLAN (necessary correction): The plan's Step 0a/0b used !is_player_scope_damage_filter(vt) as the player-arm guard. That over-rejected the mixed "a player or planeswalker" recipient (Or{Player,Planeswalker}), regressing Hunter's Insight (a delayed-trigger card whose combat-damage-to-a-player firing broke). I introduced a dedicated building-block predicate damage_recipient_filter_can_match_player (the player-arm dual of is_player_scope_damage_filter) that recurses through Or/And so a mixed disjunction still qualifies via its player-scope leg. Added a matcher regression test (mixed_player_or_planeswalker_recipient_fires_on_player). This is the correct, more-general gate and is what the plan's intent required.

VERIFICATION (cargo run directly; worktree not under Tilt):

  • cargo fmt --all --check: clean (exit 0).
  • cargo clippy -p engine --all-targets -D warnings: clean.
  • cargo test -p engine: 12139 lib tests + 1065 integration tests pass, 0 failures (including previously-failing hunters_insight and snapshot_questing_beast after the fix).
  • Added building-block tests: 8 parser tests (combinator-level + full-card Strax/The Beast/Lowland/Mirri/Greven/Crovax/Flesh Reaver), 7 matcher tests (per-event SelfRef gate, planeswalker recipient, player-scope no-regression, mixed Or recipient, aggregate non-SelfRef Step 0b regression guard, The Beast aggregate smoke), 1 condition evaluator test (DamagedPlayerIsEventSourceOwner: owner/non-owner/object/absent).
  • Card-data regenerated and verified: Strax valid_target=Typed([Creature]) condition=null; The Beast valid_target=null condition=DamagedPlayerIsEventSourceOwner damage_kind=CombatOnly; Lowland/Mirri/Greven scoped; Crovax/Flesh Reaver stay unscoped (terminator guard); Questing Beast/Deus/Coastal Piracy/Taii Wakeen player-recipient + P/T-gate cards unregressed. 55 DamageDone triggers now carry object-typed valid_target; 1 carries the owner relation.

PARSER DIFF GATE: My HEAD diff to oracle_trigger.rs contains ZERO string-dispatch patterns (the disjunction check uses peek(tag("or ")), the alphanumeric word-boundary check mirrors the file's existing parse_enters_tapped_state_rider idiom). check-parser-combinators.sh exits non-zero ONLY because it compares against an ancient base commit (#904, a far ancestor) and flags many PRE-EXISTING lines in oracle.rs/oracle_static/* that are not mine — verified each flagged oracle_trigger line is "no (pre-existing)" and absent from my diff.

CR ANNOTATIONS (all verified against docs/MagicCompRules.txt before writing): CR 120.1 ("An object that deals damage is the source of that damage"), CR 120.3 (recipient is player or permanent), CR 102.2 (opponent is the other player), CR 108.3 (owner definition), CR 603.4 (intervening-if double-check).

MULTI-AGENT SAFETY: reverted the incidental crates/engine/data/known-tokens.toml regeneration (a side-effect of gen-card-data.sh picking up unrelated DB drift, not part of this fix). Final tree = exactly the 5 load-bearing files. No commit/push performed. Strax's Grenades "Choose a player at random" clause left untouched (cluster #5 / PR #3452 scope).

NOT alreadyCorrect: both lead cards and the class were confirmed still misparsed on upstream/main before edits.

Files changed

  • crates/engine/src/game/trigger_matchers.rs
  • crates/engine/src/game/triggers.rs
  • crates/engine/src/parser/oracle_trigger.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/tests/integration/snapshots/integration__oracle_parser__snapshot_questing_beast.snap

CR references

  • CR 120.1
  • CR 120.3
  • CR 102.2
  • CR 108.3
  • CR 603.4

Verification

  • cargo fmt --all — pass
  • ./scripts/check-parser-combinators.sh <upstream/main merge-base 91283eafc> — pass
  • cargo clippy -p engine --all-targets -- -D warnings — pass
  • cargo test -p engine — pass
  • cargo run --profile tool --features cli --bin oracle-gen -- data --filter "the beast, deathless prince|strax, sontaran nurse" — pass
    Cards confirmed re-parsed correctly: The Beast, Deathless Prince, Strax, Sontaran Nurse

🤖 Generated with Claude Code

@ntindle
ntindle requested a review from matthewevans as a code owner June 16, 2026 05:41
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@matthewevans matthewevans self-assigned this Jun 16, 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.

DamageDone trigger valid_target fix. Adversarial review: ENQUEUE-READY. +978 is test-dominated (~150 lines production across 4 files). Right seam — populates valid_target at parse (TriggerDefinition), gates at match (match_damage_done); the new Player-arm guard is the symmetric dual of the pre-existing Object-arm guard (player-typed filters previously leaked through player_matches_filter's _=>true). One justified enum variant (DamagedPlayerIsEventSourceOwner — a recipient↔source-owner relation no TargetFilter encodes). CR 120.1/120.3/102.2/108.3/603.4 all grep-verified. Questing Beast snapshot change is a latent-bug fix (valid_target null→Typed([Planeswalker])). Strong discriminating pipeline tests (aggregate + self-ref paths). No scope creep beyond the DamageDone cluster.

@matthewevans matthewevans added the bug Bug fix label Jun 16, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jun 16, 2026
@matthewevans matthewevans removed their assignment Jun 16, 2026
Merged via the queue into phase-rs:main with commit 17c660c Jun 16, 2026
10 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