Skip to content

fix(parser): keep amass after 'lose life and …' compound (#5341)#5371

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
andriypolanski:fix/5341-dreadhorde-invasion-amass
Jul 8, 2026
Merged

fix(parser): keep amass after 'lose life and …' compound (#5341)#5371
matthewevans merged 2 commits into
phase-rs:mainfrom
andriypolanski:fix/5341-dreadhorde-invasion-amass

Conversation

@andriypolanski

Copy link
Copy Markdown
Contributor

Summary

Dreadhorde Invasion's upkeep trigger claimed coverage support but only resolved LoseLifeamass Zombies 1 was dropped by the parser, so the Army never arrived. This fix teaches the bare-" and " clause splitter to treat amass as an imperative clause starter so LoseLife + Amass compound instructions both survive.

Fixes #5341.

Root cause

Coverage for

At the beginning of your upkeep, you lose 1 life and amass Zombies 1.

marked the upkeep clause supported: true, but parse_details / card-data only emitted LoseLife. The bare-" and " splitter in oracle_effect/sequence.rs did not recognize amass as a clause start, so the whole chunk stayed one clause and the LoseLife half greedily swallowed the Amass remainder.

Standalone amass (Herald of the Dreadhorde, Dreadhorde Twins) already worked — only the compound "lose … life and amass …" shape was broken.

Changes

Parser — bare-and clause starter

  • Added tag("amass ") to starts_bare_and_clause_lower (CR 701.47a + CR 608.2c).
  • Trailing space mirrors other transitive keyword actions (create , sacrifice ): amass always takes a subtype + count.
  • Covers the class of "lose N life and amass [subtype] N" compounds, not only this card.

Tests

  • Splitter: bare_and_splits_lose_life_and_amass
  • Effect chain: effect_chain_lose_life_and_amass_keeps_both_clauses
  • Trigger parse: dreadhorde_invasion_upkeep_lose_life_and_amass
  • Runtime (GameScenario + real upkeep advance): dreadhorde_invasion_upkeep_amasses_zombie_army — controller loses 1 life and gets a Zombie Army token with one +1/+1 counter

Out of scope

  • Regenerating card-data.json / coverage so shipped card data reflects the Amass sub_ability (needs the usual card-data pipeline after merge).
  • The second ability (Zombie token power ≥6 attacks → lifelink) was already parsed correctly and is unchanged.

Test plan

  • cargo fmt --all
  • cargo test -p engine --lib bare_and_splits_lose_life_and_amass
  • cargo test -p engine --lib effect_chain_lose_life_and_amass
  • cargo test -p engine --lib dreadhorde_invasion_upkeep_lose_life_and_amass
  • cargo test -p engine --test dreadhorde_invasion_amass
  • After merge: regenerate card-data and confirm coverage children include Amass under the upkeep Phase trigger

Risk notes

  • The splitter change is additive: amass was never treated as a noun-phrase continuation, so recognizing it as a clause start only unlocks previously dropped conjuncts.
  • Existing solo-amass and LoseLife-only cards are unaffected.

@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 resolves issue #5341 by adding support for splitting compound clauses containing 'amass' (e.g., 'you lose 1 life and amass Zombies 1') in the parser. This ensures that both the life loss and the amass effects are processed instead of the amass effect being swallowed. The changes include parser updates, unit tests, and a new runtime integration test for Dreadhorde Invasion, all fully annotated with the relevant Comprehensive Rules (CR) references. I have no feedback to provide as the implementation is correct, idiomatic, and adheres to the repository's style guide.

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.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

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

1 card(s) · ability/Amass · added: Amass (count=1, subtype=Zombie)

Examples: Dreadhorde Invasion

@matthewevans matthewevans self-assigned this Jul 8, 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.

Parser and runtime evidence are clean: the bare and splitter now keeps Amass as a second instruction, the trigger parser preserves the sub-ability, the runtime test resolves Dreadhorde Invasion through the scenario pipeline, and the parse-diff is limited to the expected Amass gain for Dreadhorde Invasion.

@matthewevans matthewevans added the bug Bug fix label Jul 8, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 8, 2026
@matthewevans matthewevans removed their assignment Jul 8, 2026
Merged via the queue into phase-rs:main with commit cf48287 Jul 8, 2026
13 checks passed
andriypolanski added a commit to andriypolanski/phase that referenced this pull request Jul 8, 2026
…hase-rs#5340 branch

The Witch's Oven test commit reverted tag("amass ") and its parser/trigger
regressions from phase-rs#5371. Restore them from origin/main so Dreadhorde Invasion
and the lose-life-plus-amass class keep both clauses.

Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans added a commit to philluiz2323/phase that referenced this pull request Jul 8, 2026
…phase-rs#5340) (phase-rs#5379)

* test(engine): pin Witch's Oven Food token counts for ConditionInstead (phase-rs#5340)

* fix: restore phase-rs#5341 amass splitter accidentally dropped from phase-rs#5340 branch

The Witch's Oven test commit reverted tag("amass ") and its parser/trigger
regressions from phase-rs#5371. Restore them from origin/main so Dreadhorde Invasion
and the lose-life-plus-amass class keep both clauses.

Co-authored-by: Cursor <cursoragent@cursor.com>

* fix(PR-5379): correct Witch's Oven CR reference

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
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.

Dreadhorde Invasion — Upkeep trigger doesn't amass

2 participants