fix(engine): activate a zero-card "discard your hand" cost with an empty hand (Lion's Eye Diamond)

[michiot05]

Tier: Frontier
Model: claude-opus-4-8

Closes #6494.

Summary

Lion's Eye Diamond — "Discard your hand, Sacrifice this artifact: Add three mana of any one color. Activate only as an instant." — could not be activated with an empty hand. Discarding your hand with no cards is a cost paid by doing nothing (CR 601.2h — a zero-card cost is not unpayable, it is trivially paid; CR 701.9a moves cards hand→graveyard only when cards exist), so LED is activatable empty-handed — its canonical competitive use (crack it in response holding zero cards for three mana). Instead the activation stalled forever on a dead WaitingFor::PayCost { kind: Discard, count: 0, choices: [], min_count: 0 }.

Root cause is runtime, not the parser — LED's cost parses faithfully as Composite[ Discard { count: HandSize(Controller) }, Sacrifice(SelfRef) ]. legal_actions correctly offered LED (cost_payability.rs computes 0 >= 0 → payable), but the interactive cost-surfacing paths resolved the same HandSize = 0 and unconditionally emitted a PayCost { Discard, count: 0 } — a prompt with nothing to select. Submitting the empty selection left the "not yet paid" sentinel unchanged and re-entered the same surfacing, re-emitting the dead prompt: an infinite stall. The divergence: payability short-circuits count == 0, the surfacing paths did not — unlike the sibling zero-quantity cost paths (e.g. mana_abilities::exile_cost_choice).

Class-level fix, not an LED special case: a resolved-count-0 non-self "discard from hand" leg is paid by doing nothing and the engine surfaces the next unpaid leg (here, the sacrifice) instead of a dead prompt. This is applied through a single shared authority so the mana-ability path and the activated-ability path resolve the same rule:

• New resolve_non_self_discard_requirement (game/casting.rs) — the tri-state authority for a non-self FromHand discard leg: Ok(None) (no such leg, or resolved count 0 → auto-paid), Err (fewer eligible cards than the required nonzero count → CR 601.2h unpayable), Ok(Some((count, eligible))) (interactive selection). It replaces the duplicated detect→resolve→error→PayCost blocks at the two activation cost-surfacing sites.
• discard_cost_choice (game/mana_abilities.rs, the LED/Diamond Lion mana-ability path) returns None on a resolved count of 0, mirroring the count == 0 skip its exile sibling already uses — the discard leg is skipped and the self-sacrifice + mana proceed to ChooseManaColor.

Class members fixed: Lion's Eye Diamond and Diamond Lion (mana-ability path), Bomat Courier ("{R}, Discard your hand, Sacrifice Bomat Courier: …", activated-ability path) — each now activatable with an empty hand. A fixed Discard { 1 } ("Discard a card") on an empty hand is unchanged: it resolves count 1, 0 >= 1 is false, so it stays unpayable and is never offered — the guard fires only on a resolved count of 0, never on count >= 1.

Deliberately out of scope

The spell additional-cost discard arm (pay_additional_cost_with_source) is intentionally left unchanged. A genuine "discard your hand" spell additional cost resolves to ≥1 while the spell is still in hand, so it never reaches count 0. The only real cards that reach that arm with count 0 are Firestorm / Abandon Hope / Nahiri's Wrath, whose "discard X cards" additional cost currently misparses to Discard { Fixed(0) } while the correct count is X — auto-paying there would silently cast them without the discard, masking a parser bug as a rules-incorrect free cast. That Discard { Fixed(0) } misparse of "discard X cards" is a separate parser issue and should be fixed at the parser seam; I did not paper over it at the cost-resolution layer. (Happy to file/take that as a follow-up.)

Implementation method (required)

Method: /engine-implementer — plan (/engine-planner) → /review-engine-plan (adversarial; corrected the seam to the mana-ability discard_cost_choice, tightened the class to resolved-count-0 only, and named the real corpus cards) → implement (tests-first RED→GREEN) → /review-impl (Maintainer-Simulation Gate; it flagged the spell-additional-cost arm as a rules-incorrect Firestorm-misparse mask — that arm and its synthetic test were reverted, leaving the two genuinely-reachable seams). Each step in a fresh agent context. Final read-only /review-impl on the retained diff: PASS.

Gate A

./scripts/check-parser-combinators.sh → Gate A PASS head=d633cf026 (Gate G PASS). No files under crates/engine/src/parser/ are touched — the change is pure runtime cost-resolution.

Anchored on

• game/mana_abilities.rs exile_cost_choice — the interactive non-self cost sibling in the same mana-ability surfacing prefix; discard_cost_choice now applies the identical resolved-count-0 skip, so a zero-quantity leg is treated as paid and the next leg (the self-sacrifice in continue_mana_ability_cost_payment) is surfaced.
• game/cost_payability.rs Discard arm — the payability side already short-circuits count == 0 (0 >= 0 → offered); this change brings the surfacing side into agreement, closing the payable-but-un-surfaceable gap that caused the stall.
• game/casting.rs find_non_self_discard / find_eligible_discard_targets — the existing FromHand discard-leg detector + eligible-card query, now composed once inside resolve_non_self_discard_requirement and reused at both activation surfacing sites (surface_next_unpaid_interactive_activation_cost and handle_activate_ability's begin_cost_payment).

Verification

• cargo fmt --all clean; cargo clippy -p engine --tests 0 warnings; cargo test -p engine --lib 17,631 passed / 0 failed.
• 7 new/updated tests (all through production entries — activate_mana_ability, handle_activate_ability — with verbatim Oracle text; LED/Diamond Lion are mana abilities and do not use the stack):
  • lions_eye_diamond_activates_empty_handed_and_produces_chosen_color — empty hand → result is ChooseManaColor (not PayCost{Discard}); LED is sacrificed (sole card in graveyard); handle_choose_mana_color(Red) → exactly 3 Red mana. The reported bug; revert-guard.
  • diamond_lion_activates_empty_handed_and_produces_chosen_color — same class, second real card (proves class-not-card).
  • lions_eye_diamond_discards_hand_and_then_produces_chosen_color (existing control) — 2 cards in hand → still surfaces PayCost{Discard,count:2}, both discarded, LED sacrificed, 3 mana (guard: the count == 0 skip must not over-fire on count > 0).
  • bomat_courier_activates_empty_handed_casting_path — Bomat Courier's verbatim ability, empty hand → no dead PayCost{Discard}, self-sacrifice fires, ability resolves.
  • resolve_discard_requirement_fixed_one_empty_hand_is_unpayable_err — Fixed Discard{1} + empty hand → Err (unpayable), not Ok(None); reach-guarded with a card in hand → Ok(Some((1,[card]))).
  • resolve_discard_requirement_fixed_two_with_three_eligible_offers_all — Discard{2} + 3 eligible → Ok(Some((2, 3 choices))) (interactive prompt still built for count > 0).
  • resolve_discard_requirement_source_card_scope_is_not_auto_paid — a SourceCard "discard this card" cost is FromHand-excluded (helper returns None, count resolves to 1) so it can never hit zero-count auto-pay; reach-guarded that the FromHand shape is detected.
• Fails before, passes after — reverting the discard_cost_choice resolved == 0 skip flips the LED/Diamond Lion tests (left: PayCost { Discard, count: 0, choices: [] }, right: ChooseManaColor); reverting the helper's count == 0 skip flips the Bomat test (a dead PayCost{Discard} is surfaced on the casting path). The control and boundary tests stay green across both reverts.
• CR annotations grep-verified against docs/MagicCompRules.txt: 601.2h, 701.9a, 601.2b, 605.1a, 106.1, 601.2a.

Claimed parse impact

None. No parser or type files are touched — this is a pure runtime cost-resolution fix; LED already parsed faithfully. The CI parse-diff artifact should report zero card-parse changes.

Scope Expansion

None. (The spell additional-cost arm was explicitly excluded — see Deliberately out of scope — to avoid masking the separate Discard { Fixed(0) } parser misparse.)

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed handling of empty-hand “discard from hand” costs by auto-completing the requirement without presenting a zero-card discard prompt.
  • Abilities now correctly proceed to subsequent cost steps (for example, sacrificing the source card and then choosing mana) instead of stalling on an invalid pay-cost interaction.
  • Non-self discard requirements correctly remain unpayable when there aren’t enough eligible cards.
  • Discard selection now accurately lists all eligible choices and avoids unintended auto-expansion or autopayment.

• Tests

  • Added regression coverage for these discard-cost and mana-activation scenarios.

[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: 194e46d6-09bf-4ae3-9ac9-47cbb3ede8f7

📥 Commits

Reviewing files that changed from the base of the PR and between d633cf0 and 7b0bea0.

📒 Files selected for processing (2)

• crates/engine/src/game/casting.rs
• crates/engine/src/game/mana_abilities.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/engine/src/game/casting.rs

---

📝 Walkthrough

Walkthrough

The change centralizes non-self FromHand discard resolution, treats resolved zero-card requirements as satisfied, and updates casting and mana-ability payment flows to skip zero-count discard prompts. Regression tests cover eligibility, selection mode, empty hands, and subsequent cost resolution.

Changes

Discard cost resolution

Layer / File(s)	Summary
Resolve non-self discard requirements crates/engine/src/game/casting.rs, crates/engine/src/game/casting_tests.rs	Adds shared FromHand discard detection and resolution, validates eligible-card counts, skips zero-card requirements, and tests fixed counts and scope filtering.
Continue casting cost payment crates/engine/src/game/casting_costs.rs, crates/engine/src/game/casting_tests.rs	Uses the shared resolver so empty-hand discard costs proceed to later cost legs, covered by a Bomat Courier activation test.
Skip zero-card mana-ability prompts crates/engine/src/game/mana_abilities.rs	Uses shared detection, suppresses zero-count prompts, restricts interactive prompts to chosen discards, and tests empty-hand Lion’s Eye Diamond variants.

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

Suggested labels: bug

Suggested reviewers: matthewevans, andriypolanski

🚥 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 matches the main fix: empty-hand zero-card discard costs now activate correctly, with Lion's Eye Diamond as a representative case.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/engine/src/game/mana_abilities.rs (1)

3566-3586: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Share the discard-leg resolver instead of keeping a second detector.
discard_cost_choice still re-derives the same non-self discard shape locally, and its extra selection: CardSelectionMode::Chosen gate makes it diverge from casting::find_non_self_discard rather than reuse it. Fold that selection constraint into a shared helper or adapter so the zero-count and payability rules stay aligned in one place.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/game/mana_abilities.rs` around lines 3566 - 3586, Update
discard_cost_choice to reuse the shared casting::find_non_self_discard resolver
instead of independently calling find_non_self_discard_cost and applying a
separate selection gate. Move the CardSelectionMode::Chosen constraint into the
shared helper or an adapter, preserving the existing zero-count handling and
ensuring discard payability rules remain aligned in one place.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/engine/src/game/mana_abilities.rs`:
- Around line 3566-3586: Update discard_cost_choice to reuse the shared
casting::find_non_self_discard resolver instead of independently calling
find_non_self_discard_cost and applying a separate selection gate. Move the
CardSelectionMode::Chosen constraint into the shared helper or an adapter,
preserving the existing zero-count handling and ensuring discard payability
rules remain aligned in one place.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 24e2fdc3-e3c0-4e60-a746-954001acee1e

📥 Commits

Reviewing files that changed from the base of the PR and between 6895ca3 and d633cf0.

📒 Files selected for processing (4)

• crates/engine/src/game/casting.rs
• crates/engine/src/game/casting_costs.rs
• crates/engine/src/game/casting_tests.rs
• crates/engine/src/game/mana_abilities.rs

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Changes requested — the claimed discard-cost authority is duplicated.

🔴 Blocker

[MED] Make the casting resolver/detector the sole authority for FromHand discard costs. Evidence: crates/engine/src/game/casting.rs:15243-15296 introduces find_non_self_discard/a resolver, while game/mana_abilities.rs:3566-3585,3683-3696 retains a parallel detector with a different CardSelectionMode::Chosen constraint. The two paths can diverge as cost forms grow. Reuse the casting authority, parameterizing the mana-specific selection path only if needed, and add a shared-seam test.

Recommendation: request changes; consolidate the detector before approval.

[michiot05]

Thanks — consolidated onto the single casting authority (head 7b0bea08f).

What changed

• casting::find_non_self_discard is now the sole detector. Its return is widened to (&QuantityExpr, Option<&TargetFilter>, CardSelectionMode) — it reports the selection mode instead of encoding a selection policy. The duplicate mana_abilities::find_non_self_discard_cost (with its separate CardSelectionMode::Chosen constraint) is deleted (grep-confirmed zero remaining references).
• discard_cost_choice now delegates the zero-count + payability rules to casting::resolve_non_self_discard_requirement, so the "zero-card discard paid by doing nothing" and "insufficient cards → unpayable" rules live in exactly one place, shared with the activation path. The only thing the mana path keeps is its one explicit divergence — the selection != CardSelectionMode::Chosen gate — as a single documented call-site line rather than a forked detector. The activation sites (2/3) stay selection-agnostic, so their behavior is unchanged.
• No lint suppression: the delegation is resolve_non_self_discard_requirement(...).unwrap_or_default() (the Err/insufficient-cards arm is unreachable here because cost_payability already gates activation on hand size, and its None fallback is the correct "no selection to surface" result — documented inline).

Shared-seam test (as requested): find_non_self_discard_is_sole_detector_mana_path_gates_on_chosen asserts the sole detector returns the selection mode for both a bare Discard{FromHand,Chosen} and inside an LED-shaped Composite, and returns it for a non-Chosen (Random) leg too (proving it no longer filters by selection); and that discard_cost_choice returns the interactive selection for the Chosen leg (through the shared resolver) but None for the Random leg (the mana gate). Revert-sensitive both ways.

Behavior-preserving: all 7 prior tests green with unchanged assertions; cargo clippy -p engine --tests 0 warnings; cargo test -p engine --lib 17,632 passed / 0 failed.

[matthewevans]

Approved: current head consolidates FromHand discard detection at the casting authority, correctly treats a resolved zero-card discard as paid, and retains discriminating runtime coverage for both mana and activated-ability paths.