cluster: declare-attackers per-attacker target legality#4805
cluster: declare-attackers per-attacker target legality#4805Daedalus-Icarus wants to merge 4 commits into
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 introduces per-attacker legal attack target tracking to support complex combat restrictions in multiplayer games, updating both the Rust engine and the React frontend. The review feedback identifies several critical Rust compilation errors caused by passing owned values instead of references to Vec::contains. Additionally, the feedback highlights a logic bug in the TypeScript target fallback, an inefficient O(N) computation for single-attacker targets, and a style guide violation due to missing mandatory CR annotations on new combat rule functions.
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.
| valid_attack_targets_by_attacker | ||
| .get(id) | ||
| .is_none_or(|targets| targets.contains(target)) |
There was a problem hiding this comment.
[CRITICAL] Compilation error due to passing owned AttackTarget to contains. Evidence: crates/engine/src/ai_support/candidates.rs:3586.
Why it matters: Passing an owned AttackTarget to Vec::contains causes a compilation error because contains expects a reference.
Suggested fix: Pass a reference to target instead.
| valid_attack_targets_by_attacker | |
| .get(id) | |
| .is_none_or(|targets| targets.contains(target)) | |
| valid_attack_targets_by_attacker | |
| .get(id) | |
| .is_none_or(|targets| targets.contains(&target)) |
References
- Do not replace
Option::is_none_orwithmap_or(true, ...)in Rust codebases that support Rust 1.82.0 or newer, as this will trigger Clippy'sunnecessary_map_orlint.
| let required_players: Vec<PlayerId> = must_attack_players_for_creature(state, obj) | ||
| .into_iter() | ||
| .filter(|player| attackable_players.contains(player)) | ||
| .collect(); |
There was a problem hiding this comment.
[CRITICAL] Compilation error due to passing owned PlayerId to contains. Evidence: crates/engine/src/game/combat.rs:3799.
Why it matters: Passing an owned PlayerId to Vec::contains causes a compilation error because contains expects a reference.
Suggested fix: Pass a reference to player instead.
| let required_players: Vec<PlayerId> = must_attack_players_for_creature(state, obj) | |
| .into_iter() | |
| .filter(|player| attackable_players.contains(player)) | |
| .collect(); | |
| let required_players: Vec<PlayerId> = must_attack_players_for_creature(state, obj) | |
| .into_iter() | |
| .filter(|player| attackable_players.contains(&player)) | |
| .collect(); |
| if let AttackTarget::Player(defending_pid) = target { | ||
| if has_attackable_non_goading_player && goading_players.contains(defending_pid) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
[CRITICAL] Compilation error due to passing owned PlayerId to contains. Evidence: crates/engine/src/game/combat.rs:3807.
Why it matters: Passing an owned PlayerId to contains causes a compilation error because contains expects a reference.
Suggested fix: Pass a reference to defending_pid instead.
| if let AttackTarget::Player(defending_pid) = target { | |
| if has_attackable_non_goading_player && goading_players.contains(defending_pid) { | |
| return false; | |
| } | |
| } | |
| if let AttackTarget::Player(defending_pid) = target { | |
| if has_attackable_non_goading_player && goading_players.contains(&defending_pid) { | |
| return false; | |
| } | |
| } |
| if required_players.is_empty() { | ||
| return true; | ||
| } | ||
| matches!(target, AttackTarget::Player(pid) if required_players.contains(pid)) |
There was a problem hiding this comment.
[CRITICAL] Compilation error due to passing owned PlayerId to contains. Evidence: crates/engine/src/game/combat.rs:3814.
Why it matters: Passing an owned PlayerId to contains causes a compilation error because contains expects a reference.
Suggested fix: Pass a reference to pid instead.
| matches!(target, AttackTarget::Player(pid) if required_players.contains(pid)) | |
| matches!(target, AttackTarget::Player(pid) if required_players.contains(&pid)) |
| export function getValidAttackTargetsForAttacker( | ||
| state: GameState | null, | ||
| attackerId: ObjectId, | ||
| ): AttackTarget[] { | ||
| if (!state) return []; | ||
| const wf = state.waiting_for; | ||
| if (wf.type !== "DeclareAttackers") return []; | ||
| return wf.data.valid_attack_targets_by_attacker?.[String(attackerId)] ?? wf.data.valid_attack_targets ?? []; | ||
| } |
There was a problem hiding this comment.
[HIGH] Incorrect fallback for attackers with no legal targets. Evidence: client/src/utils/combat.ts:78.
Why it matters: When valid_attack_targets_by_attacker is present, an attacker missing from the map has zero legal targets, but falling back to global targets incorrectly allows illegal attacks.
Suggested fix: Only fall back to global targets if the map itself is undefined, otherwise return an empty array.
export function getValidAttackTargetsForAttacker(
state: GameState | null,
attackerId: ObjectId,
): AttackTarget[] {
if (!state) return [];
const wf = state.waiting_for;
if (wf.type !== "DeclareAttackers") return [];
if (wf.data.valid_attack_targets_by_attacker) {
return wf.data.valid_attack_targets_by_attacker[String(attackerId)] ?? [];
}
return wf.data.valid_attack_targets;
}References
- Avoid using unnecessary optional chaining (
?.) or nullish coalescing (??) on properties that are already guaranteed to be defined through TypeScript type narrowing (e.g., after checking a discriminated union'stypefield).
| pub fn get_valid_attack_targets_for_attacker( | ||
| state: &GameState, | ||
| attacker_id: ObjectId, | ||
| ) -> Vec<AttackTarget> { | ||
| get_valid_attack_targets_by_attacker(state) | ||
| .remove(&attacker_id) | ||
| .unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
[HIGH] Inefficient O(N) computation for a single attacker's targets. Evidence: crates/engine/src/game/combat.rs:3851.
Why it matters: Computing valid targets for all battlefield permanents just to retrieve one attacker's targets is highly inefficient and scales poorly.
Suggested fix: Compute targets directly for the requested attacker_id using the helper functions.
| pub fn get_valid_attack_targets_for_attacker( | |
| state: &GameState, | |
| attacker_id: ObjectId, | |
| ) -> Vec<AttackTarget> { | |
| get_valid_attack_targets_by_attacker(state) | |
| .remove(&attacker_id) | |
| .unwrap_or_default() | |
| } | |
| pub fn get_valid_attack_targets_for_attacker( | |
| state: &GameState, | |
| attacker_id: ObjectId, | |
| ) -> Vec<AttackTarget> { | |
| let gates = CombatStaticGates::compute(state); | |
| let broad_targets = get_valid_attack_targets(state); | |
| let base_targets = | |
| base_valid_attack_targets_for_attacker_gated(state, attacker_id, &broad_targets, &gates); | |
| valid_attack_targets_for_attacker_from_base(state, attacker_id, &base_targets, &gates) | |
| } |
| fn attacker_is_generically_eligible_to_attack( | ||
| state: &GameState, | ||
| attacker_id: ObjectId, | ||
| gates: &CombatStaticGates, | ||
| ) -> bool { |
There was a problem hiding this comment.
[MED] Missing mandatory CR annotations on new combat rule functions. Evidence: crates/engine/src/game/combat.rs:3630.
Why it matters: Every rules-touching line of engine code must carry a verified CR annotation to maintain strict fidelity to the MTG Comprehensive Rules.
Suggested fix: Add appropriate CR annotations (e.g., CR 508.1a, CR 508.1c, CR 508.1d) to the new helper functions.
References
- Rule R6: Every rules-touching line of engine code must carry a comment of the form CR : . (link)
Daedalus-Icarus
force-pushed
the
cluster/attack-condition-bugs
branch
from
d51380a to
304ef8a
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Final declare-attackers validation still measures "if able" against global players instead of per-attacker legal targets. Evidence: crates/engine/src/game/combat.rs:2551. Why it matters: a goaded creature whose only non-goading opponent is excluded by a scoped CantAttack restriction is surfaced by get_valid_attack_targets_by_attacker as able to attack its goader, but declare_attackers rejects that legal declaration because another player is globally attackable; the same global check at crates/engine/src/game/combat.rs:2575 affects directed MustAttackPlayer. Suggested fix: drive goad and directed-must enforcement from the same per-attacker base target set used by valid_attack_targets_for_attacker_from_base, and add fixtures combining scoped CantAttack with goad and MustAttackPlayer.
[HIGH] AttackTargetPicker "Attack All" still submits targets that are not common to every selected attacker. Evidence: client/src/components/controls/AttackTargetPicker.tsx:165. Why it matters: when attacker A can only attack P2 while attacker B can attack P1/P2, the picker opens and still offers global P1 in all mode; choosing it submits A -> P1, reintroducing illegal client declarations instead of rendering engine-provided per-attacker legality. Suggested fix: render only commonTargets in all/bulk modes.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head a27771d. The backend blocker is fixed: final declare-attackers validation now uses the per-attacker legal target set for goad / must-attack checks, and Attack All is constrained to common per-attacker targets.
I am leaving final acceptance deferred because this PR includes frontend/client changes and is not by an FE-review-allowed author in the current sweep policy. Matt should handle the frontend acceptance path directly.
|
Backend re-reviewed at current head 47443c1. The only backend changes since my prior backend pass are compile/reference fixes in candidates.rs, combat.rs, and a test payload update in manabrew-compat; I do not see a new backend blocker. Still holding final acceptance under the current sweep policy because this PR includes frontend/client changes and is not by an FE-review-allowed author. No approval/enqueue from the sweep. |
Parse changes introduced by this PRBaseline pending for |
|
Backend status at current head 2a43f89: no new backend changes since the prior backend re-review. The new commit only changes AttackTargetPicker.tsx and client/pnpm-lock.yaml, so I am keeping the backend-clean status from the previous sweep and continuing to defer final acceptance to Matt for the frontend/client surface. |
Daedalus-Icarus
force-pushed
the
cluster/attack-condition-bugs
branch
from
2a43f89 to
10830eb
Compare
|
Backend status on current head I am still not approving/enqueueing this from the sweep: this PR remains mixed backend/frontend and currently reports |
Daedalus-Icarus
force-pushed
the
cluster/attack-condition-bugs
branch
from
10830eb to
a0e5a88
Compare
matthewevans
added
the
defer-fe
2 participants
Summary
Closes #4800
Fix declare-attackers legality to use per-attacker legal target sets end to end. This prevents the engine, AI, and client from proposing or submitting attack declarations that are globally plausible but illegal for a specific creature, addressing the
#4800/#4765combat-declaration failure class.Implementation method (required)
How was the engine/parser logic in this PR produced? Check exactly one:
/engine-implementerpipeline (plan → review-plan → implement → review-impl → commit)/engine-implementer— explain why belowIf you did not use
/engine-implementer, state why (e.g. frontend-onlychange, docs/CI/tooling change, release chore, or a fix too small to warrant
the 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 logic and tests around:
CR 508.1aCR 508.1bCR 508.1cCR 508.1dCR 701.15bCR 702.26bVerification
cargo fmt --allpnpm --dir client exec vitest --run src/utils/__tests__/combat.test.ts src/components/controls/__tests__/AttackTargetPicker.test.tsx2files,5tests.cd client && pnpm run type-checkcd client && pnpm exec eslint src/components/controls/AttackTargetPicker.tsx src/components/controls/__tests__/AttackTargetPicker.test.tsx src/utils/combat.ts src/components/board/ActionButton.tsxcargo test -p engine valid_attack_targets_by_attacker_filters_scoped_attack_lockouts --libcargo test -p engine scoped_legality --libcargo test --no-run -p manabrew-compat --features engine/proptest