Skip to content

fix(engine): evaluate 'attacks alone' relative to the attacker, not the ability source#5378

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
dhgoal:fix/attacks-alone-observer-5328
Jul 9, 2026
Merged

fix(engine): evaluate 'attacks alone' relative to the attacker, not the ability source#5378
matthewevans merged 3 commits into
phase-rs:mainfrom
dhgoal:fix/attacks-alone-observer-5328

Conversation

@dhgoal

@dhgoal dhgoal commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Tier: Frontier
Model: claude-opus-4-8
Thinking: high

Closes #5328.

Summary

The "attacks alone" mechanic silently failed to fire when the observing permanent stayed back and a different creature attacked alone.

TriggerCondition::MinCoAttackers tallied co-attackers by excluding the ability source (source_id). "Attacks alone" is encoded as Not(MinCoAttackers { minimum: 1 }). When a non-attacking observer (an Exalted-style card — the issue's Agent 13, Sharon Carter / Black Widow Double Agent) is the ability source but is not in combat, excluding source_id from the tally is a no-op — so the lone attacker stays counted, MinCoAttackers returns true, and Not(...) reports "not alone." The ability never fires.

Fix: exclude the creature that actually fired the trigger, not the ability source. The per-attacker AttackersDeclared event (produced by matching_attack_events, and re-checked at resolution per CR 603.4) names exactly the triggering attacker; use it, falling back to source_id when the event names no single attacker (self-referential trigger paths — behavior unchanged). source_id still resolves the filter's source object, so Training's power-relative co-attacker comparison (CR 702.149a) is unaffected.

Reproduction (confirmed failing-before / passing-after)

A four-board probe isolates exactly one wrong quadrant on main:

board expected on main
observer stays back, 1 other attacks alone fires (1 clue) 0 clues — BUG
observer stays back, 2 others attack silent (0) 0 ✓
observer attacks alone fires (1) 1 ✓
observer + 1 other attack silent (0) 0 ✓

The new test other_creature_attacks_alone_fires_even_when_observer_stays_back is the regression pin (0 before this change, 1 after); two_other_creatures_attacking_do_not_fire_attacks_alone is the negative control.

Note for the maintainer: the reporter also mentioned possible over-triggering with multiple attackers; I could not reproduce that on any board (the multi-attacker quadrant is already correct, above). The confirmed defect is the under-triggering case named in the issue title ("not triggering"), which this fixes. Flagging the discrepancy transparently rather than claiming to cover the parenthetical.

Anchored on

  • crates/engine/src/game/triggers.rs — the TriggerCondition::MinCoAttackers arm in check_trigger_condition: the co-attacker filter previously read a.object_id != source_id, now excludes the event's triggering attacker.
  • The AttackersDeclared { attacker_ids, .. } event (from matching_attack_events, re-passed at resolution-time re-check per CR 603.4) — the source of the single triggering-attacker id.

Verification

  • cargo fmt --all — clean.
  • cargo clippy -p engine --tests -- -D warnings — clean.
  • No parser change (engine-only) → parser combinator gate N/A.
  • cargo test -p engine --test attacks_alone_observer_5328 — 2/2 pass (fail-on-revert).
  • cargo test -p engine --lib -- game::triggers:: — 353 passed; full integration suite — 1823 passed. Pre-existing related tests still green: issue_5159_attacks_alone_investigate, training_condition_counts_only_higher_power_coattackers, granted_ability_self_binding.

CR verified against docs/MagicCompRules.txt: 506.5, 508.1a, 603.2c, 603.4, 702.83b, 702.149a.

Scope Expansion

None.

…he ability source

Closes phase-rs#5328.

TriggerCondition::MinCoAttackers counted co-attackers by excluding the
ability SOURCE (source_id) from the tally. 'Attacks alone' is encoded as
Not(MinCoAttackers{minimum:1}); when a non-attacking observer (Exalted /
Agent 13, Sharon Carter) stays back and a DIFFERENT creature attacks
alone, source_id is not in combat, so excluding it is a no-op. The lone
attacker stays counted, MinCoAttackers returns true, and Not(...) makes
the 'attacks alone' ability silently fail to fire.

Exclude the creature that fired the trigger instead: read the single
attacker named by the per-attacker AttackersDeclared event (produced by
matching_attack_events and re-checked at resolution, CR 603.4), falling
back to source_id when the event names no single attacker (self-referential
paths, unchanged). source_id still resolves the filter's source object so
Training's power-relative co-attacker comparison (CR 702.149a) is
unaffected.

CR 506.5 (attacks alone = only creature declared) + CR 508.1a + CR 603.2c
+ CR 702.83b (Exalted). Adds tests/attacks_alone_observer_5328.rs pinning
both the fire-when-another-attacks-alone case (was 0 clues, now 1) and the
two-attackers negative control.
@dhgoal
dhgoal requested a review from matthewevans as a code owner July 8, 2026 17:11
@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 8, 2026
@superagent-security

Copy link
Copy Markdown

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@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 adds a new integration test file attacks_alone_observer_5328.rs to verify the "attacks alone" mechanic (Issue #5328). The tests ensure that the mechanic is evaluated relative to the attacking creature rather than the ability's source, verifying that the trigger fires correctly when a different creature attacks alone and does not fire when multiple creatures attack. There are no review comments, so I have no feedback to provide.

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

✓ No card-parse changes detected.

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

[MED] Move this regression into the shared integration test binary instead of adding a new top-level engine test file. Evidence: crates/engine/tests/attacks_alone_observer_5328.rs:1 adds a new file directly under crates/engine/tests/, while the repo's current engine-test rule requires new integration tests under crates/engine/tests/integration/ with a mod entry in crates/engine/tests/integration/main.rs (see the existing adjacent issue_5159_attacks_alone_investigate entry at crates/engine/tests/integration/main.rs:426). Why it matters: each top-level file becomes a separate engine test binary, increasing link/build cost and bypassing the shared integration harness this repo uses for new engine regressions. Suggested fix: move this test to crates/engine/tests/integration/issue_5328_attacks_alone_observer.rs, add mod issue_5328_attacks_alone_observer; to crates/engine/tests/integration/main.rs, and reuse the shared super::rules::run_combat/imports where practical.

I ignored the contributor-trust signal as requested. The runtime seam itself looks right to me: matching_attack_events expands non-batched Attacks triggers into singleton attacker events before MinCoAttackers is checked, and the filter still uses source_id for Training's source-relative power comparison. Separately, GitHub currently reports the branch as DIRTY/CONFLICTING, so it will also need to be brought current before enqueue.

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

Maintainer update resolved the main merge conflict and moved the new engine coverage into the existing integration test harness. The attacker-relative MinCoAttackers logic now preserves source-relative filter evaluation. CI is still pending on the updated head.

@matthewevans matthewevans removed their assignment Jul 9, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 9, 2026
Merged via the queue into phase-rs:main with commit 3b9e44c Jul 9, 2026
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contributor:flagged Contributor flagged for review by trust analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attack Alone — The "Attack Alone" mechanic is not triggering correctly, if you attack with multiple attackers in your c…

2 participants