fix(engine): tap target defending player controls for equipment attack triggers (#6678)

[JacobWoodson]

Fixes #6678

Problem

Captain America's Shield's attack trigger — "Whenever equipped creature attacks, tap target creature defending player controls" — prompted for a target but never tapped it. Reported as a regression in 0.38.0.

Root cause

The trigger source is the Equipment, not the attacker. capture_combat_status finds the Shield absent from combat.attackers and records defending_player: None on its trigger-source snapshot.

Three sites resolved "defending player" with the shape:

trigger_source
    .map(|s| s.combat_status.defending_player)
    .unwrap_or_else(|| resolve_defending_player(state, source_id))

When trigger_source is Some but the captured defending_player is None, .map() yields Some(None), so .unwrap_or_else() never runs the fallback and the whole expression returns None. With no defending player resolvable, the chosen target failed the CR 608.2b resolution-time legality re-check and the ability silently fizzled — the prompt appeared, nothing tapped.

Introduced by the P04 resolution-frames refactor (777dbabf), matching the reported regression window.

The load-bearing site was an inline duplicate of the resolver in filter_inner_for_object (the target-validation path). A parallel duplicate in quantity.rs had the same defect.

Fix (CR 508.5 / 508.5a)

When an ability refers to both an attacking creature and a defending player, the defender is the player that creature is attacking (CR 508.5), individually determined per attacker (CR 508.5a). A captured None means "this source is not the attacker" (Equipment/Aura) — not "no defender" — so it must fall through to resolve_defending_player, which reads the attacker from the triggering event.

• filter.rs — route the inline ControllerRef::DefendingPlayer arm in filter_inner_for_object through the single-authority source_defending_player (mirroring how the sibling SourceChosenPlayer arm already delegates), eliminating the duplicate.
• filter.rs source_defending_player — .and_then().or_else() so a captured None falls through to the attacker fallback.
• quantity.rs source_defending_player_for_context — same fix.

This fixes the whole "equipped creature attacks → affect something the defending player controls" class (Greatsword of Tyr, Mjölnir Storm Hammer, Thunder Lasso, Heart-Piercer Bow, and the rest listed in the issue), not just the Shield.

Testing

• New discriminating integration test issue_6678_captain_america_shield_tap_defender.rs drives the real declare-attackers / trigger pipeline (not a synthetic trigger event). It fails on the prior build — and even with only the parallel-path fix — and passes once the inline arm is fixed.
• cargo test -p engine: 3830 integration + 498 lib tests pass, 0 failures. The self-attack path (Kibo, Uktabi Prince — captured Some(defender)) still passes.
• cargo clippy -p engine --tests: clean.
• CR annotations verified against docs/MagicCompRules.txt.

Acceptance criteria

1. ✅ Attacking with the equipped creature and choosing a creature the defending player controls leaves it tapped.
2. ✅ A discriminating runtime test drives the attack pipeline and failed against the current build.
3. ✅ The silent no-op is no longer reachable on this path — the target survives resolution, so the subject set is non-empty.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Corrected defending-player targeting for triggered effects involving attachments and similar sources.
  • Fixed effects that target creatures controlled by the defending player when resolving attack triggers.

• Tests

  • Added regression coverage for Captain America’s Shield, ensuring its attack trigger correctly taps a creature controlled by the defending player.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c34cf314-f36c-442e-a266-7d8f8070d10d

📥 Commits

Reviewing files that changed from the base of the PR and between de8dc15 and 21118eb.

📒 Files selected for processing (4)

• crates/engine/src/game/filter.rs
• crates/engine/src/game/quantity.rs
• crates/engine/tests/integration/issue_6678_captain_america_shield_tap_defender.rs
• crates/engine/tests/integration/main.rs

---

📝 Walkthrough

Walkthrough

The change updates defending-player resolution for triggered attachment sources and adds an integration regression test covering Captain America’s Shield tapping a defending creature after an equipped creature attacks.

Changes

Defending Player Resolution

Layer / File(s)	Summary
Fallback defending-player resolution crates/engine/src/game/filter.rs, crates/engine/src/game/quantity.rs	Filter and quantity resolution now fall back to the triggering attack when captured combat data contains no defending player.
Captain America’s Shield regression coverage crates/engine/tests/integration/issue_6678_captain_america_shield_tap_defender.rs, crates/engine/tests/integration/main.rs	Adds and registers an integration test that attaches the Shield, declares an attack, selects a defending creature, and verifies it becomes tapped.

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

Suggested labels: bug

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 clearly describes the core fix for equipment attack triggers tapping a creature controlled by the defending player.
Linked Issues check	✅ Passed	The code and regression test directly address issue #6678 by resolving the defending player correctly and tapping the selected target.
Out of Scope Changes check	✅ Passed	The changes stay within scope: engine resolution fixes and a targeted integration test for the reported regression.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches 💡 1

⚔️ Resolve merge conflicts 💡

• Resolve merge conflict in branch claude/github-issue-6678-375e8f

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[matthewevans]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Maintainer hold update for current head aa16be685a8978018c32c84fa58c2de0cdb3eb94: Card data has passed and the fresh parse-diff has been reviewed. The implementation review remains clean; the only remaining gate is the two still-running Rust test shards in the current CI run. Please let both complete successfully; we will rescan then for approval/enqueue.

[matthewevans]

Request changes — the quantity.rs fallback extends the fix without a discriminating runtime test.

🔴 Blocker

[MED] Missing discriminating runtime proof for the QuantityContext defending-player fallback. Evidence: crates/engine/src/game/quantity.rs:157-165 changes source_defending_player_for_context to fall through from a captured None; its current production consumers include the attachment-controller path at quantity.rs:4208-4210 and the damage-source-controller path at quantity.rs:4277-4279. The new Shield regression at crates/engine/tests/integration/issue_6678_captain_america_shield_tap_defender.rs:94-130 only exercises target legality through filter.rs, so restoring only the quantity.rs .and_then(...).or_else(...) fallback leaves that test green. Why it matters: the expanded quantity behavior can regress independently while the reported Shield path still passes. Suggested fix: add a focused attachment-trigger, current-combat runtime test that reaches one of those quantity consumers and fails when this fallback is reverted, or remove the unproven quantity.rs change.

✅ Clean

All CI checks are green and the current-head parse-diff reports no card-parse changes; neither establishes the independent quantity route above.

Recommendation: request changes — supply the focused runtime proof or narrow the patch.

[matthewevans]

Approved: the narrowed filter.rs fix delegates DefendingPlayer lookup to the existing source_defending_player authority, and the Captain America's Shield integration test exercises the real combat-trigger target-validation and resolution pipeline.

[JacobWoodson]

@matthewevans thanks — you're right that the quantity.rs fallback was unproven. I took the "remove the unproven change" option and narrowed the patch to the filter.rs fix that the Shield regression test actually discriminates.

Rationale for removing rather than adding a test: the two consumers you identified — AttachmentsOnLeavingObject (count filtered by defending-player controller) and damage_source_controller_matches (DefendingPlayer) — don't correspond to any real card in the "equipped creature attacks → defending player" class. A runtime test reaching either would be building for a phantom card, so restoring quantity.rs to baseline keeps the shipped change fully covered. If a real card ever needs that fallback, it can land with its own discriminating test.

The PR now touches exactly three files: the filter.rs DefendingPlayer resolution fix (routed through the single-authority source_defending_player), the discriminating Shield integration test, and its main.rs registration. Rebased onto the port-across-main commit and pushed as a fast-forward; Shield + Kibo (self-attack path) both pass on the ported tree.