Skip to content

feat(parser): recover trailing subtype/quoted-ability after a for-each dynamic pump#5621

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
minion1227:minion_dynamic_pump_trailing
Jul 11, 2026
Merged

feat(parser): recover trailing subtype/quoted-ability after a for-each dynamic pump#5621
matthewevans merged 1 commit into
phase-rs:mainfrom
minion1227:minion_dynamic_pump_trailing

Conversation

@minion1227

Copy link
Copy Markdown
Contributor

CR 205.1b + CR 604.1

Bug

A dynamic "gets +N/+M for each X" pump can carry a trailing conjunct, but the for each branch of parse_continuous_gets_has only recovered a trailing keyword clause — a trailing type-addition or quoted-ability grant was dropped:

  • Reaper's Scythe — "Equipped creature gets +1/+1 for each soul counter on this Equipment and is an Assassin in addition to its other types." → dropped AddSubtype(Assassin) (kept only the dynamic P/T).
  • Rakdos Riteknife — "Equipped creature gets +1/+0 for each blood counter on this Equipment and has "{T}, Sacrifice a creature: ..."" → dropped the granted ability.

Fix

After the dynamic pump and the existing trailing-keyword recovery, also run the existing parse_additive_type_clause_modifications (the same additive-type authority added in #5548) and parse_quoted_ability_modifications over the description, extending the modification list. Both scanners no-op when their pattern is absent, so:

  • keyword-only for-each pumps (Claws of Valakut, Ethereal Armor, Nyxborn Hydra, Thran Power Suit) are unchanged (regression-tested);
  • cards whose dynamic pump falls back to a fixed-pump path elsewhere (Avatar Destiny, Machinist's Arsenal) are untouched — no double-counting, verified live.

Tests

  • dynamic_for_each_pump_recovers_trailing_subtype_addition — Reaper's Scythe → AddDynamicPower + AddSubtype("Assassin").
  • dynamic_for_each_pump_recovers_trailing_quoted_ability — Rakdos Riteknife → AddDynamicPower + GrantAbility.
  • dynamic_for_each_pump_trailing_keyword_unchanged — regression: keyword-only pump keeps AddDynamicPower + AddKeyword(FirstStrike) and gains no spurious subtype/grant.
  • Full parser::oracle_static::tests (1053 passed, 0 failed).

CR verification

  • CR 205.1b — "in addition to its other types" type-addition (verified in docs/MagicCompRules.txt).
  • CR 604.1 — granted (quoted) abilities.

AI-contributor notes

  • Rules-correct: the equipped creature now gains the trailing subtype / granted ability alongside the dynamic pump.
  • Reuses existing authorities (parse_additive_type_clause_modifications, parse_quoted_ability_modifications) rather than adding a parallel path; pattern-absent no-op keeps the blast radius to cards that actually carry the trailing conjunct.
  • Verified: live-parsed the full for-each class before/after; cargo fmt, CI-exact clippy -p engine --all-targets --features proptest -D warnings, and the full oracle_static module are green.

🤖 Generated with Claude Code

…h dynamic pump

CR 205.1b + CR 604.1: A dynamic "gets +N/+M for each X" pump can carry a trailing
conjunct, but the for-each branch of `parse_continuous_gets_has` only recovered a
trailing keyword clause — a trailing type-addition or quoted-ability grant was
dropped:

- Reaper's Scythe — "Equipped creature gets +1/+1 for each soul counter on this
  Equipment and is an Assassin in addition to its other types." dropped the
  Assassin subtype.
- Rakdos Riteknife — "Equipped creature gets +1/+0 for each blood counter on this
  Equipment and has \"{T}, Sacrifice a creature: ...\"" dropped the granted ability.

Fix: after the dynamic pump and trailing-keyword recovery, also run the existing
`parse_additive_type_clause_modifications` (the same additive-type authority used
elsewhere) and `parse_quoted_ability_modifications` over the description. Both
scanners no-op when their pattern is absent, so keyword-only for-each pumps (Claws
of Valakut, Ethereal Armor, Nyxborn Hydra, Thran Power Suit) are unchanged, and
cards whose dynamic pump falls back to a fixed pump elsewhere are untouched.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@minion1227 minion1227 requested a review from matthewevans as a code owner July 11, 2026 23:20
@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!

@github-actions

Copy link
Copy Markdown

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

1 card(s) · static/Continuous · field mods: add dynamic power, add dynamic toughnessadd dynamic power, add dynamic toughness, add subtype Assassin

Examples: Reaper's Scythe

1 card(s) · ability/PutCounter · added: PutCounter (counter=1 blood, kind=activated, target=granting object)

Examples: Rakdos Riteknife

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

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

Approving. This is exactly the shape a parser fix should have.

The claim matches the measurement. You said two cards; the coverage-parse-diff sticky says two cards, and they are the two you named:

  • Reaper's Scythe · mods: add dynamic power, add dynamic toughness…, add subtype Assassin
  • Rakdos Riteknife · PutCounter (counter=1 blood, kind=activated, target=granting object) — added

Zero collateral on the general pool, against a baseline (1c3eee9dd9cb) that is one commit off current main. That is the whole point of the "both scanners no-op when their pattern is absent" argument, and the pool proves it rather than asserting it — the keyword-only for-each pumps (Claws of Valakut, Ethereal Armor, Nyxborn Hydra, Thran Power Suit) are untouched, and so are the cards that fall back to the fixed-pump path.

CR verification (grep-verified against docs/MagicCompRules.txt):

  • CR 205.1b (line 1400) — "This rule applies to effects that use phrases such as 'in addition to its other types'." Precisely the Reaper's Scythe clause.
  • CR 604.1 (line 2663) — static abilities. Correct for the quoted grant.

Right seam. You extended the existing authorities (parse_additive_type_clause_modifications from #5548, parse_quoted_ability_modifications) rather than building a parallel recovery path in the for-each branch. Four lines, two existing building blocks, no new vocabulary. That's the composable-building-block rule working as intended.

The tests are discriminating, which is the part I care most about: both new tests assert the trailing modification is present, and on main it is dropped — so they fail if the fix is reverted. And dynamic_for_each_pump_trailing_keyword_unchanged guards the no-op claim from the other side, asserting the keyword-only path gains no spurious subtype or grant. A test that can only pass because the fix is there, plus a test that catches the fix over-reaching. That's the pair.

Nice work — this and #5612 are both clean.

@matthewevans matthewevans added this pull request to the merge queue Jul 11, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 11, 2026
Merged via the queue into phase-rs:main with commit 1832824 Jul 11, 2026
13 checks passed
@matthewevans matthewevans added the quality For high-quality minimal to no-churn PRs label Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix quality For high-quality minimal to no-churn PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants