fix: use per-attacker attack target legality

[Daedalus-Icarus]

Summary

Fixes #4765 by teaching declare-attackers to use per-attacker legal target sets instead of a single global target list. This prevents the client and AI from submitting attacks that are globally attackable in the step but illegal for a specific attacker, which is what caused the reported ObjectId(...) can't attack Player(...) failure.

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):

Targeted declare-attackers bugfix spanning engine waiting-state payloads, combat validation, AI candidate generation, and frontend selection logic. I validated the issue first and then implemented the narrow seam directly rather than running the full /engine-implementer pipeline.

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

Touched/added references include:

• CR 508.1a
• CR 508.1b
• CR 508.1c
• CR 508.1d
• CR 508.5
• CR 701.15b
• CR 702.26b
• CR 302.6

Verification

• Validated the reported issue body with:
  • gh issue view 4765 -R phase-rs/phase --json number,title,body,state,url,labels
• Added frontend regression coverage in:
  • client/src/utils/__tests__/combat.test.ts
• Added engine regression coverage in:
  • crates/engine/src/game/combat.rs
• Attempted Rust verification with:
  • cargo check -p engine --tests
  • cargo check -p engine --lib
  • cargo test -p engine valid_attack_targets_by_attacker_filters_scoped_attack_lockouts -- --exact
• Attempted frontend verification with:
  • CI=true pnpm test -- --run src/utils/__tests__/combat.test.ts

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[matthewevans]

Thanks. I did a backend pass; frontend/client acceptance remains deferred under current policy, and this PR is also currently conflicting with base.

Backend blockers:

1. Goad still allows non-player targets when a non-goading player is attackable. CR 701.15b requires the goaded creature to attack a player other than the goader if able; treating a planeswalker or battle target by its defending player does not satisfy that direct player-attack requirement. Please keep goad-satisfying targets to direct AttackTarget::Player(pid) targets when such a non-goading player is available.

2. Phase AI still chooses from the global valid_attack_targets list rather than the new valid_attack_targets_by_attacker map. A scoped attacker can be assigned a target that is globally legal but illegal for that attacker, and the validation fallback can then choose the empty declaration. Please thread the per-attacker target map through the combat chooser or select from engine-filtered legal declare-attackers candidates.

[matthewevans]

Backend re-review on c8faa0af63: the two backend blockers from my prior review look addressed:

• goad target legality now narrows to direct non-goading AttackTarget::Player targets when a non-goading player is attackable, and rejects the planeswalker case.
• Phase AI/search now threads valid_attack_targets_by_attacker and chooses through the per-attacker target helper instead of the global target list.

I’m not approving or enqueueing this from the sweep: this is still a mixed backend/frontend PR under the FE-defer policy (label already applied), and GitHub currently reports conflicts with base (DIRTY). Leaving FE/client acceptance and conflict resolution for Matt/direct handling.

[matthewevans]

Backend re-review on 94277615ad after the merge-from-main update. The earlier goad/per-attacker legality issues still look conceptually addressed, but the current head has a backend/AI blocker:

[MED] The new AI entrypoint now fails the Rust lint gate. CI is failing Rust lint (fmt, clippy, parser gate) with:

error: this function has too many arguments (8/7)
--> crates/phase-ai/src/combat_ai.rs:155:1
pub fn choose_attackers_with_targets_with_profile(...)

The new valid_attack_targets_by_attacker parameter is the right data to thread, but this public-ish AI helper has crossed the clippy limit. Please fold the attack-context inputs into a small parameter struct or otherwise reuse the existing declaration context shape so the per-attacker map does not just proliferate the function signature.

Frontend/client acceptance remains deferred to Matt under the current sweep policy; I also see the current frontend gate red in AttackTargetPicker.test.tsx, but I’m not treating frontend acceptance from this sweep.

[matthewevans]

Backend re-review on current head 104bd11d: the prior backend clippy blocker from 94277615 appears addressed and Rust lint is green.

I’m still not approving or enqueueing this from the sweep because this remains a mixed backend/frontend PR with frontend/client acceptance deferred under current policy (defer-fe stays), and the current GitHub check set still has failing Rust test shards plus the frontend gate. This needs either a clean current-head test state and Matt FE acceptance, or another author update if those failures are from the PR.

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Backend re-review on 892f97f7 found the new AI target-scoping change itself aligned with the engine's per-attacker target handling, but the PR is still not merge-ready.

CI is red in Rust tests (shard 2/2) on engine::game::combat::tests::goad_still_forces_redirect_when_an_attackable_non_goading_player_exists. The failing assertion is the PR's goad redirect regression test: declare_attackers now returns an error string containing must attack a different player directly if able, while the test asserts the older substring must attack a different player if able. Please make the diagnostic/test agree so the discriminating backend test passes.

Also noting the existing sweep policy state: this PR still carries defer-fe, so client/UI acceptance remains deferred to Matt even after the backend test is fixed.

[matthewevans]

Backend re-review on current head 318363e9ba: the latest diagnostic assertion fix addresses the Rust shard failure I called out on 892f97f7; I do not see a new backend blocker in the per-attacker target handling.

I’m still not approving or enqueueing this from the sweep because this remains a mixed backend/frontend PR under the FE-defer policy (defer-fe stays), and the current-head checks/parse-diff are still queued or stale after the new push. FE/client acceptance remains for Matt/direct handling.

[Daedalus-Icarus]

hi, @matthewevans, could you review my PR, plz?

[matthewevans]

Holding current head 61846e3329e3f986a83afb4b3987f40c960317d6 for current CI/parse-diff evidence. The current code pass looks clean for per-attacker attack target legality and appears to address the prior goad/AI-target feedback, but the required evidence is not current yet: checks are still pending and the parse-diff sticky predates this head.

[matthewevans]

Current-head backend/mixed re-review on 538aa2d2d1ecc429215d5087d95b5c9e5110938d: I do not see a remaining backend blocker in the per-attacker target legality, AI/search target threading, serialization surface, or production declare-attackers coverage. The parse-diff is current and reports no card-parse changes, and the current check set is green.

I’m still not approving or enqueueing this from the sweep because this remains a mixed backend/frontend PR with defer-fe; frontend/client acceptance is left for Matt/direct handling under the current policy.

[matthewevans]

Processed current head e42ea9686486dce606a366e9fe129b2e559091f9. GitHub now reports the branch clean and the current check set green; the backend per-attacker target handling remains acceptable from the sweep.

I am still not approving or enqueueing this PR because it remains a mixed backend plus frontend/client PR with defer-fe. Frontend/client acceptance is left for Matt/direct handling under the current policy.

[Daedalus-Icarus]

hi, @matthewevans, can your review my PR ,plz?

[matthewevans]

Frontend review on current head e42ea9686486dce606a366e9fe129b2e559091f9:

I agree this PR is solving a real bug class. The old frontend had only a global valid_attack_targets list and could default every selected attacker to a target that was legal for the step but illegal for that specific attacker. The new engine-authored valid_attack_targets_by_attacker payload is the right authority to thread through the client, and the core direction looks mergeable once the remaining frontend polish is addressed.

Please make these frontend follow-ups before merge:

1. Restore the useful rationale comments removed from AttackTargetPicker.tsx, updated for the new per-attacker legality behavior. The comments around the PeekTab/scroll-container layout and the assignment transform semantics were explaining non-obvious constraints; removing them makes this component harder to safely maintain.

2. Add component-level coverage for mixed per-attacker target legality. The current new tests cover client/src/utils/combat.ts, but AttackTargetPicker.test.tsx still renders without validTargetsByAttacker, so the picker UI path for common-target filtering, disabled illegal steppers, legality-aware spread, and final Distribute confirmation is not directly exercised.

3. Handle the empty common-target default mode. selectedAttackersNeedTargetPicker can open the picker when at least one attacker has multiple legal targets, but AttackTargetPicker starts in Attack All mode and renders only commonTargets. If selected attackers have no shared target, the dialog opens with no target buttons until the user manually switches to Distribute. Defaulting to Distribute when commonTargets.length === 0, or showing a translated empty-state affordance, would close that UX gap.

Once those are addressed, I think the frontend side is acceptable and this should be mergeable from the FE perspective, assuming the current green checks stay green.

[matthewevans]

Sweep note: still DEFER-FE on current head. I am not approving, enqueueing, or merging this from the automation sweep because the PR remains a mixed backend/frontend surface from a non-exempt author and frontend acceptance is reserved for Matt's direct handling under current policy.