fix: gate attacks-alone triggers on zero co-attackers (closes #5159)#5162
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for the "attacks alone" trigger qualifier by encoding it as a zero-co-attacker gate (TriggerCondition::MinCoAttackers { minimum: 0 }) in both the native parser and the mtgish-import converter, accompanied by integration tests. The review feedback identifies violations of R1 due to using standard string methods (strip_prefix) instead of nom combinators in the parser, as well as violations of R6 regarding missing or incorrect CR citations (citing CR 508.3a instead of CR 506.5).
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.
| if attacks_alone { | ||
| def.condition = Some(TriggerCondition::MinCoAttackers { | ||
| minimum: 0, | ||
| filter: None, | ||
| }); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation on rules-touching code. Evidence: crates/engine/src/parser/oracle_trigger.rs:8365.
Why it matters: R6 requires every rules-touching line of engine code to carry a comment of the form CR <number>: <description> to ensure compliance and traceability. Suggested fix: Add a comment citing CR 506.5 above the condition block.
| if attacks_alone { | |
| def.condition = Some(TriggerCondition::MinCoAttackers { | |
| minimum: 0, | |
| filter: None, | |
| }); | |
| } | |
| if attacks_alone { | |
| // CR 506.5: A creature attacks alone if it's the only creature declared as an attacker. | |
| def.condition = Some(TriggerCondition::MinCoAttackers { | |
| minimum: 0, | |
| filter: None, | |
| }); | |
| } |
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for taking this on, but the current encoding does not implement the attacks alone gate.
TriggerCondition::MinCoAttackers is an at-least condition: the runtime counts same-controller co-attackers and checks co_attacker_count >= minimum. Setting minimum: 0 therefore always passes, including when the source attacks with one or more other creatures. That means the new parser/converter shape still allows the Agent 13 trigger to fire during multi-creature attacks, which is the bug this PR is meant to fix.
Rules evidence: CR 506.5 defines a creature as attacking alone only when it is attacking and no other creatures are. Please model this as an exact/maximum zero co-attacker gate, or add an appropriate parameterized condition shape, and keep the multi-attacker regression covering that runtime behavior. Also note the PR is currently DIRTY against base and still needs the parse-diff evidence before it can be considered for approval.
RealDiligent
force-pushed
the
fix/issue-5159-attacks-alone-trigger-gate
branch
from
a09ccd4 to
77f564b
Compare
|
Addressed the review in 77f564b (rebased onto current main): Encoding fix: attacks alone now uses Not(MinCoAttackers { minimum: 1 }), matching the Exalted synthesis path - zero same-controller co-attackers, not minimum: 0 which always passed. Parser: switched strip_attack_alone_qualifier to a nom tag combinator (R1). Rules cite: CR 506.5 (not 508.3a). Integration test issue_5159_attacks_alone_investigate unchanged - it already asserts multi-attacker silence. CI running. |
…rs#5159) Parse "attacks alone" into MinCoAttackers { minimum: 0 } so Sharon Carter / Exalted-class triggers fire only when a single creature attacks. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Parse changes introduced by this PR · 91 card(s), 13 signature(s) (baseline: main
|
|
Follow-up on your review (e10188a, rebased onto current main):
Integration tests cover solo vs multi-attacker investigate. CI re-running. |
Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off: attacks-alone now uses the existing Not(MinCoAttackers { minimum: 1 }) building block, has discriminating combat tests, and the parse diff is limited to the attacks-alone class.
Head branch was pushed to by a user without write access
Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off refreshed for current head: attacks-alone uses the existing Not(MinCoAttackers { minimum: 1 }) condition, parse diff is limited to the attacks-alone trigger class, and solo/multi-attacker runtime tests discriminate the fix.
2 participants
Summary
"attacks alone"on attack triggers intoTriggerCondition::MinCoAttackers { minimum: 0 }(CR 508.3a — zero co-attackers).WhenACreatureAttacksAlone.Closes #5159.
Test plan
trigger_samurai_or_warrior_attacks_aloneparser unit testissue_5159_solo_attack_investigates_onceissue_5159_multi_attack_does_not_investigate