fix(engine): don't force a MustAttack carrier to attack itself (#5952)

[andriypolanski]

Closes #5952

Summary

Discord report: Fumiko the Lowblood appears to "goad" herself — her static "Creatures your opponents control attack each combat if able." was forcing Fumiko (a creature you control) to attack.

The parse is correct: the static is a MustAttack with affected: Typed { controller: Opponent }. The bug is in runtime enforcement, not the AST.

Root cause: the must-attack logic had a has_local_must_attack fast-path that reported a requirement for any creature merely carrying a MustAttack static — checking only sd.mode == StaticMode::MustAttack and ignoring the static's affected filter. Fumiko carries an opponent-scoped MustAttack, so she satisfied "local must-attack" against herself (CR 508.1d + CR 109.5).

Fix: check_static_ability / check_static_ability_sources are already the single authority — they run the full applicability predicate (affected filter, condition, per-player gates) against each carrier and return the carrier id, correctly handling both an intrinsic self static (Juggernaut's SelfRef) and a remote scope (Fumiko's Opponent). Remove the redundant, buggy has_local_must_attack duplicate and route both call sites through that authority, dropping the now-unused obj parameter from must_attack_sources_gated.

Changes

• crates/engine/src/game/combat.rs — delete has_local_must_attack; the enforcement bool (creature_must_attack_with_attackable_players_gated) and the source collector (must_attack_sources_gated) now rely solely on check_static_ability / check_static_ability_sources, which honor the static's affected filter. Drop the unused obj parameter from must_attack_sources_gated.
• crates/engine/tests/integration/fumiko_the_lowblood_5952.rs (+ main.rs mod) — runtime regression through the real parse → layer → combat-enforcement pipeline: Fumiko and her controller's other creatures are NOT forced during their own combat, while a creature an opponent of Fumiko's controller controls IS forced (positive control).

Test Plan

• cargo fmt --all -- --check
• cargo clippy -p engine --all-targets --features engine/proptest -- -D warnings
• cargo test -p engine (full suite: 21,174 pass)
• cargo test -p engine --test integration fumiko
• cargo test -p engine --test integration curse_of_the_nightly_hunt_must_attack (sibling cross-permanent MustAttack, still green)

Out of scope

The classifier also noted local card data collapses "Bushido X" to Bushido 1. That is a separate parser issue explicitly scoped out of this report and is not addressed here.

Summary by CodeRabbit

• Bug Fixes

  • Corrected forced-attack (MustAttack) handling for Fumiko the Lowblood, ensuring opponent-scoped behavior is enforced only when the appropriate static gate is active.
  • Updated combat constraint evaluation so attacker-declaration validation reflects the correct “must attack” targets per controller context.

• Tests

  • Added an integration regression suite for the Fumiko the Lowblood MustAttack case, including scenarios for both controllers and validation of empty vs. required attacker declarations.
  • Extended the integration test manifest to include the new module.

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[superagent-security]

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

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Request changes — the runtime predicate is corrected, but the declaration authority still reintroduces the same unscoped local-static test.

🔴 Blocker

crates/engine/src/game/combat.rs:3341 still treats any MustAttack carried by the candidate as a requirement via active_static_definitions(...).any(|sd| sd.mode == StaticMode::MustAttack), without applying sd.affected. The PR correctly removes that predicate from creature_must_attack_with_attackable_players_gated, but AttackDeclarationConstraints::build feeds both validate_attack_declaration (combat.rs:3877-3925) and AI completion, so an empty declaration in Fumiko's controller's combat still has required == 1 and is rejected. The current regression only calls creature_must_attack; it does not exercise this production declaration path.

Evidence: Fumiko's Oracle text is “Creatures your opponents control attack each combat if able.” The scoped affected filter must therefore be applied before adding MustAttackGeneric; the unfiltered local arm is exactly the old carrier-vs-beneficiary bug at a second enforcement seam.

Please replace the active_static_definitions local arm in AttackDeclarationConstraints::build with the same check_static_ability(..., static_target_ctx(cid)) applicability query used by this patch, and add a registered runtime regression through validate_attack_declaration/the declare-attackers action showing that Fumiko and her controller's other creature can be omitted during P0's combat while P1's creature remains required during P1's combat.

✅ Clean

The changed creature_must_attack_with_attackable_players_gated and source collector do use check_static_ability / check_static_ability_sources, and the parse-diff artifact for this head reports no card-parse changes.

Recommendation: request changes; update the remaining declaration/AI constraint seam and cover the real validator before re-review.

[matthewevans]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Warning

Review limit reached

@matthewevans, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 32 seconds

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0a8debc0-67d2-4e98-aaa4-6363e7128338

📥 Commits

Reviewing files that changed from the base of the PR and between 76a5e6f and 54a6304.

📒 Files selected for processing (1)

• crates/engine/tests/integration/main.rs

📝 Walkthrough

Walkthrough

Combat must-attack source reporting and creature eligibility now share gated StaticMode::MustAttack evaluation. New Fumiko the Lowblood integration tests verify controller and opponent scoping during attacker declaration.

Changes

Must-Attack Gating

Layer / File(s)	Summary
Unify must-attack combat evaluation crates/engine/src/game/combat.rs	Removes the local intrinsic helper and uses gated static ability checks for must-attack sources, creature eligibility, attacker constraints, and display wiring.
Add Fumiko regression coverage crates/engine/tests/integration/fumiko_the_lowblood_5952.rs, crates/engine/tests/integration/main.rs	Adds scenarios verifying Fumiko’s controller is not forced to attack, opposing creatures are forced when applicable, and attacker declarations are validated accordingly.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main fix: creatures carrying MustAttack are no longer forced to attack themselves.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[matthewevans]

Approved after current-head review. The MustAttack applicability check now consistently uses the scoped static-ability authority, and the registered regression covers both predicate and declaration enforcement.