Skip to content

ENGINE: Fix Foray amassed Army damage#5102

Open
davidomil wants to merge 1 commit into
phase-rs:mainfrom
davidomil:fix/foray-orcs-amassed-damage
Open

ENGINE: Fix Foray amassed Army damage#5102
davidomil wants to merge 1 commit into
phase-rs:mainfrom
davidomil:fix/foray-orcs-amassed-damage

Conversation

@davidomil

@davidomil davidomil commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes amassed Army referents so Foray of Orcs, Grishnakh, Brash Instigator, and Surrounded by Orcs use the Army selected by the current amass instruction. The resolver now records that Army after amass finalization, carries it through chained/reflexive abilities and target selection, and covers token/counter replacement pauses.

Implementation method (required)

How was the engine/parser logic in this PR produced? Check exactly one:

  • Produced via the /engine-implementer pipeline (plan -> review-plan -> implement -> review-impl -> commit)
  • Not /engine-implementer — explain why below

If you did not use /engine-implementer, state why (e.g. frontend-only
change, docs/CI/tooling change, release chore, or a fix too small to warrant
the pipeline):

N/A

Note

Any change to crates/engine/ game logic — parser, effects, resolver,
targeting, rules behavior — is expected to go through /engine-implementer.
The "not used" box is for changes that genuinely fall outside that scope.

CR references

  • CR 701.47a: amass chooses or creates an Army and puts counters on it.
  • CR 701.47c: "the amassed Army" refers to the Army chosen by the amass instruction.
  • CR 608.2c: later instructions in the same resolving ability can refer to objects established by earlier instructions.
  • CR 603.12: reflexive triggered abilities use the parent resolution context when they trigger.
  • CR 614.16: replacement effects can interrupt token/counter events before the amass instruction is complete.

Verification

  • cargo fmt --all — passed
  • cargo test -p engine --test integration mauhur_swarming_of_moria — passed, 9 tests
  • cargo clippy --all-targets -- -D warnings — passed
  • cargo test -p engine — passed
  • ./scripts/check-parser-combinators.sh — passed
  • cargo engine-inventory — passed
  • git diff --check — passed
  • git diff --name-only | rg '^(mtgish/|crates/mtgish-import/|data/mtgish-)' — no matches
  • Post-rebase on phase-rs/main 269e98b5e: cargo test -p engine --test integration mauhur_swarming_of_moria — passed, 9 tests

@davidomil davidomil requested a review from matthewevans as a code owner July 4, 2026 21:43

@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 support for referencing 'the amassed Army' (such as 'the amassed Army's power') in reflexive abilities per CR 701.47c. It introduces a new GameEvent::ArmyAmassed event, updates the parser and importer to handle demonstrative power and toughness quantities, and adds integration tests. The feedback suggests robustly handling multiple amass events by retrieving the most recent event using events.iter().rev().find_map rather than a strict uniqueness check, and notes a missing mandatory CR annotation on the event dispatch in amass.rs.

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 thread crates/engine/src/game/effects/mod.rs Outdated
Comment thread crates/engine/src/game/effects/amass.rs
@matthewevans

Copy link
Copy Markdown
Member

Holding review until the parser evidence is available.

This PR changes engine/parser surfaces, so the sweep needs the coverage-parse-diff sticky for the current head before completing review against the actual card-level blast radius. I’ll re-check once that evidence posts for b5f23497f2b0ac6ffd19c8e10e87d43132787e24.

@matthewevans matthewevans added the bug Bug fix label Jul 4, 2026
@davidomil davidomil force-pushed the fix/foray-orcs-amassed-damage branch from b5f2349 to fd81136 Compare July 4, 2026 21:56
@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 3 card(s), 3 signature(s) (baseline: main 269e98b5e26d)

1 card(s) · ability/DealDamage · field amount: the amassed Army's poweramassed Army's power

Examples: Foray of Orcs

1 card(s) · ability/GainControl · field target: non-legendary opponent controls creaturenon-legendary power ≤amassed Army's power opponent controls creature

Examples: Grishnákh, Brash Instigator

1 card(s) · ability/Mill · field count: the amassed Army's poweramassed Army's power

Examples: Surrounded by Orcs

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

Findings:

  • The current proof only covers the Foray of Orcs damage path, but the live coverage-parse-diff for this head shows this change also alters Grishnákh, Brash Instigator's target filter and Surrounded by Orcs' mill count. The parser/import change is intentionally class-wide (the amassed Army's power now maps to ObjectScope::Demonstrative), and the new integration tests at crates/engine/tests/integration/mauhur_swarming_of_moria.rs only assert the DealDamage.amount consumer. Please add runtime coverage for the other surfaced consumers before we land this: one test proving Grishnákh's target eligibility is bounded by the amassed Army's current power, and one test proving Surrounded by Orcs mills using that same amassed Army referent. Those paths exercise target-filter quantity resolution and mill-count quantity resolution, not just damage amount resolution.

The event/context plumbing direction looks reasonable, and the multi-amass case is a useful addition. I just need the proof boundary to match the actual card-level blast radius from the current parse-diff.

@davidomil davidomil force-pushed the fix/foray-orcs-amassed-damage branch from fd81136 to 52095ef Compare July 5, 2026 05:45
@davidomil

Copy link
Copy Markdown
Contributor Author

Updated this PR with the repo-local .claude/skills/engine-implementer/SKILL.md pipeline and force-pushed the rebuilt branch.

  • New head: 52095ef95e (ENGINE: Fix amassed Army referents)
  • Rebased on current phase-rs/main: 269e98b5e
  • Plan/review-plan loop: clean
  • Implementation review loop: Maintainer-Simulation Gate: PASS

Review items addressed:

  • The amassed Army event lookup now uses the most recent event with events.iter().rev().find_map(...) and carries CR 701.47c / CR 608.2c context.
  • The ArmyAmassed dispatch now happens only after amass finalization and includes CR annotations around the amass/replacement boundary.
  • Runtime proof now covers the full parse-diff blast radius:
    • foray_of_orcs_damages_target_from_the_army_amassed_after_swarming_of_moria
    • grishnakh_target_eligibility_uses_the_amassed_armys_current_power
    • surrounded_by_orcs_mills_target_player_using_the_selected_armys_current_power
  • Replacement and negative-path coverage was added for token replacement pauses, counter replacement pauses, Chatterfang-style extra tokens, and generic demonstrative scope.
  • No mtgish/, crates/mtgish-import/, or data/mtgish-* paths are included.

Verification run locally:

  • cargo fmt --all
  • cargo test -p engine --test integration mauhur_swarming_of_moria — 9 passed
  • cargo clippy --all-targets -- -D warnings
  • cargo test -p engine
  • ./scripts/check-parser-combinators.sh
  • cargo engine-inventory
  • git diff --check
  • git diff --name-only | rg '^(mtgish/|crates/mtgish-import/|data/mtgish-)' — no matches
  • Post-rebase: cargo test -p engine --test integration mauhur_swarming_of_moria — 9 passed

@matthewevans matthewevans added the defer-fe Frontend/client/UI PR deferred to Matt's direct review label Jul 5, 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.

Backend/parser re-review at 52095ef95e0321d904d8e335fe911b0a50e86f60: I do not have additional blocking findings from the automation sweep. The current parse-diff sticky is present for this head and shows the expected three-card amassed-Army blast radius, and the new runtime tests cover Foray of Orcs, Grishnakh, Surrounded by Orcs, token replacement, counter replacement, and generic demonstrative non-leakage.

I added defer-fe because this PR also changes client/src/adapter/types.ts; final client-surface acceptance is deferred to Matt under the current sweep policy, so I am not approving/enqueuing this from automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix defer-fe Frontend/client/UI PR deferred to Matt's direct review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants