fix(engine): count non-tap combination mana toward castability and auto-pass

[kiannidev]

Summary

• Extend the castability gate to cover residual colored mana shards using activatable non-tap mana abilities (including AnyCombination producers like Vivi Ornitier).
• Prevent auto-pass from skipping priority when a spell is feasibly castable via manual mana activation, even when the simulation filter rejects the Auto CastSpell candidate.

Fixes #583

Test plan

• cargo test -p engine --test integration issue_583
• CI green on merge queue

[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]

review-impl: PR #2788 — non-tap combination mana toward castability/auto-pass

Verdict: REQUEST CHANGES. The seam and CR grounding are correct and the bug fix is real and discriminating, but the new colored-shard solver (a) reintroduces an unbounded-exponential search in the auto-pass/castability hot path, (b) double-counts a single source across the independent colored-shard and generic-capacity checks, and (c) silently under-covers most of the flexible-mana class it claims to handle via a _ => Exact collapse. None block the happy path, but at least two ship silently-incorrect for sub-classes the feature appears to cover.

Gemini hit its daily quota and posted no findings — reviewed in a clean worktree at PR head 08e73e97c.

Lens 1 — Correct seam (mostly good, one duplication)

• Right location, confirmed. Castability lives in the engine (can_feasibly_pay_mana_cost_without_x), auto-pass consumes it via ai_support, FE only renders — correct per "engine owns all logic." The fast auto-tap path (can_pay_cost_after_auto_tap, casting.rs:9379) still short-circuits, so the common case is untouched. CR 117.1d / 601.2g verified against docs/MagicCompRules.txt and accurately describe the code.
• [MED] Reinvents the existing shard-assignment solver. The codebase already has a mana-to-shard assignment engine in auto_tap_mana_sources_inner (casting_costs.rs:5232) operating on Vec<ManaSourceOption>, which handles flexible/filter-land/mixed-shard assignment. The PR adds a second, parallel assignment algorithm (can_profiles_cover_requirements / combination_assign) on a fresh ActivatableManaProfile representation. Two independent solvers for "can these sources pay these shards" will drift. Evidence: mana_sources.rs:192-282 vs casting_costs.rs:5232+. The non-tap gap is real (auto-tap can't auto-sacrifice/discard), but the shard-matching core should extend the existing solver, not duplicate it.
• [LOW] Iteration scaffold duplicates feasible_mana_capacity. activatable_mana_profiles_for_object (mana_sources.rs:89-159) copies the exact filter chain from feasible_mana_capacity (mana_sources.rs:774-797): same kind != Activated || !is_mana_ability guard, same can_activate_mana_ability_now, same activation_condition_satisfied, same build_resolved_from_def. Factor the shared "activatable mana abilities of an object" iterator.

Lens 2 — Idiomatic / PERF (the load-bearing concerns)

• [MED] Castability is NO LONGER polynomial on the non-tap path — unbounded O(2^P) backtracking. can_profiles_cover_requirements (mana_sources.rs:206-219) branches skip-or-use on every profile (cover(tail) || apply(head, …, tail)) with no memoization and no bound on P; combination_assign (257-282) adds a per-AnyCombination permutation search over requirements (R!-class). collect_activatable_mana_profiles is rebuilt per can_feasibly_pay_mana_cost_without_x call, and auto_pass_recommended → has_feasibly_castable_spell runs that for every spell in hand each priority window. The clone-in-loop is also present: each profile goes through can_activate_mana_ability_now, which does state.clone() + full activation simulation (mana_abilities.rs:1019) — so building profiles is already M × full-state-clone per spell, then the exponential search runs on top. Treasures dodge this (Tap component → auto-tap fast path), but the worst case is the uncastable colored spell: the recursion exhausts the entire 2^P × R! space before returning false, once per uncastable spell, every auto-pass tick. This is exactly the documented O(N!) mana-readiness cliff. Suggested fix: add memoization/greedy bipartite-matching, or cap P and bail conservatively; at minimum reuse the existing solver rather than a fresh exponential one.
• [MED] _ => Exact collapses the flexible-mana class to a single first-color set — silent under-coverage. In activatable_mana_profiles_for_object (mana_sources.rs:151), ChosenColor, AnyOneColorAmongPermanents, AnyInCommandersColorIdentity, OpponentLandColors, AnyTypeProduceableBy, ChoiceAmongExiledColors, DistinctColorsAmongPermanents all fall to Exact(resolve_mana_types_for_ability(...)), and resolve_mana_types_for_ability returns only the first color/type for those variants (effects/mana.rs:390, 435, 457). So a Reflecting Pool / Mox Amber / commander-identity / opponent-land source is treated as producing only its first color — the gate then falsely rejects spells those sources could actually pay. The PR's own framing is "build for the class of non-tap/combination mana," but genuine flexibility is only modeled for AnyOneColor / AnyCombination / ChoiceAmongCombinations. Per severity calibration this is a MED (false-negative castability for a sub-class that looks covered), not a nit. Suggested fix: map these to AnyOneColor/AnyCombination profiles using their full option set, or exhaustively match and handle each.

Lens 3 — Value / correctness

• Fixes the real bug; test is discriminating. Old code did if !residual_shards.is_empty() { return false; } (deleted at casting.rs:9420). The new test asserts can_cast_object_now == true for {U}{R} off a power-2 Vivi — would have failed before, passes after. The auto-pass assertion exercises the real legal_actions + auto_pass_recommended production path. Good.
• [MED] Colored-shard check and generic-capacity check double-count the SAME source. casting.rs:9420 (shard coverage) and casting.rs:9447-9454 (feasible_mana_capacity sum for generic) are evaluated independently against the same battlefield. A single power-2 Vivi produces 2 total mana, yet for {U}{R}{1} the shard check passes (2 mana → {U}{R}) and the generic check passes (feasible_mana_capacity(Vivi)=2 ≥ 1), reporting castable when the source physically yields only 2 of the needed 3. This extends the documented cross-source over-count (feasible_mana_capacity sum over-counts chain-sacrifice configurations #1235) to over-counting one source against itself. It's a false-positive (spell offered but unpayable — softer than the false-negative it fixes and consistent with the "permissive over-count" philosophy), but it's a new over-count axis the maintainer should see; add a colored+generic test (e.g. Vivi power 2 vs {U}{R}{1} should be uncastable) and net colored consumption out of the generic capacity.
• [LOW] Stale doc comment. can_feasibly_pay_mana_cost doc (casting.rs:9338-9343) still says "Colored-shard feasibility under non-tap sources is conservatively rejected … returns false" and points only at feasible_mana_capacity: colored-shard feasibility under non-tap mana sources #1234 — the PR just made that false. Update it.
• [LOW] Redundant split-second guard. has_feasibly_castable_spell (ai_support/mod.rs:14) checks stack_has_split_second, then calls can_cast_object_now which checks it again (casting.rs:8742). Harmless, removable.

Net: correct intent and seam, but the perf cliff (Lens 2) and the single-source double-count (Lens 3) should be addressed, and the _ => Exact collapse means the "class" coverage claim is only partially true.

[matthewevans]

Re-review (since 00:20Z) — PR #2788

Reviewed at head 0d8b6bc1 (commits c8c8891d harden + 0d8b6bc1 rustfmt landed since my request-changes). Ground-truthed via the .diff API and read the PR branch directly (fork; fetched pull/2788/head). No inline comments outstanding; Gemini still quota-blocked.

Verdict: REQUEST CHANGES (still). Three of my five prior findings are addressed — the _ => Exact under-coverage, the single-source double-count, and the LOW doc/split-second nits. The two load-bearing ones are not: the auto-pass/castability hot path is still unbounded-exponential, and the new solver is still a parallel re-implementation rather than an extension of the existing one. I also found a new correctness bug introduced by the combination assigner: a flexible source that produces more mana than the residual colored shards is rejected entirely.

Prior findings — resolution status

#	Finding	Status
1	MED PERF — O(2^P × R!) + clone-in-loop in the auto-pass hot path	NOT ADDRESSED
2	MED seam — parallel solver instead of extending auto_tap_mana_sources_inner	NOT ADDRESSED
3	MED _ => Exact collapses flexible-mana class to first color	ADDRESSED
4	MED single-source double-count (colored + generic)	ADDRESSED
5	LOW stale doc comment + redundant split-second guard	ADDRESSED

#3 ADDRESSED. profile_kind_from_production (mana_sources.rs:213-300) now explicitly matches ChosenColor, OpponentLandColors, AnyTypeProduceableBy, ChoiceAmongExiledColors, AnyInCommandersColorIdentity, AnyOneColorAmongPermanents and maps each to AnyOneColor { count, options } using mana_options_from_production (the full option set, CR-annotated per variant), not a single-color Exact. Reflecting Pool / Mox Amber / commander-identity sources now contribute their real flexibility. Good fix.

#4 ADDRESSED. can_cover_shards_with_activatable_mana now returns (covered, consumed_pips), and can_feasibly_pay_mana_cost_without_x does …feasible_mana_capacity(…).sum::<u32>().saturating_sub(shard_consumed) (casting.rs:9455-9458). The new test vivi_power_two_does_not_cover_colored_shards_plus_generic asserts {U}{R}{1} off a power-2 Vivi is uncastable — fails before, passes after. Discriminating. (Minor: the subtraction is global — total consumed netted against total capacity, not per-source — so it can conservatively under-count generic when shard coverage and generic capacity draw from disjoint sources. That direction is a false-negative consistent with the over-count philosophy, so LOW, but worth a comment.)

#5 ADDRESSED. The doc on can_feasibly_pay_mana_cost_without_x now points at can_cover_shards_with_activatable_mana / #583 / #2011 instead of the stale "conservatively rejected … returns false" text. The rewritten has_feasibly_castable_spell (ai_support/mod.rs:648-652) no longer re-checks split-second before calling can_cast_object_now, so the redundant guard is gone. c8c8891d also added genuine CR 106.6 value: mana_ability_allowed_for_payment + PaymentContext::Spell plumbing now respects spend restrictions (Eldrazi Temple, #2011). CR 106.6 / 117.1d / 601.2g all verify against docs/MagicCompRules.txt.

Findings on current head

[MED] Castability is still unbounded-exponential on the non-tap path; nothing changed here. assign_profiles_to_requirements (mana_sources.rs:427-453) still branches skip-or-use per object (O(2^P), P = activatable sources) and combination_assign (1138-1168) still does a per-requirement × per-color recursion with no memoization (R!-class, R = residual shards). No bound, no greedy/bipartite matching, no early infeasibility cut. Worst case remains the uncastable colored spell: the full 2^P × R! space is exhausted before returning false, and auto_pass_recommended → has_feasibly_castable_spell runs can_cast_object_now for every spell in hand each priority window. The clone-in-loop is also intact: activatable_mana_profiles_for_object (322-324) calls can_activate_mana_ability_now per mana ability, which still does state.clone() + full activation sim (mana_abilities.rs:1019), gated only by the cheap mana_ability_ready_without_simulation pre-check. So profile building is M × full-state-clone per spell before the exponential search even starts. This is the same O(N!) mana-readiness cliff the codebase already paid down once. Suggested fix: memoize on (object_index, requirements), or add a greedy bipartite-match fast path with the exponential search as fallback, and bound P.

[MED] Still a parallel shard-assignment solver, not an extension of the existing one. auto_tap_mana_sources_inner (casting_costs.rs) already assigns Vec<ManaSourceOption> to shards including flexible/filter-land rows. This PR's touch to casting_costs.rs is only a signature change (feasible_mana_capacity(… , None) at :5866) — the actual matching core is a second, independent algorithm on a fresh ActivatableManaProfile representation (mana_sources.rs:189-523). Two solvers for "can these sources pay these shards" will drift. The non-tap gap (auto-tap can't auto-sacrifice/discard) is real, but the shard-matching core should reuse the existing assigner's row model rather than duplicate it. (Unchanged from prior review — restating because it remains the architectural seam concern.)

[MED — NEW, introduced by this PR] combination_assign rejects a flexible source that produces MORE mana than the residual colored shards. mana_sources.rs:1138-1168. Success requires reaching count == 0 && requirements.is_empty() simultaneously: when requirements empties while count > 0 the function returns None (1150-1152), and there is no early "remaining requirements satisfied, ignore surplus" exit. Trace a power-3 Vivi paying {U}{R} (count=3, requirements=[{U},{R}]): fill {U} → combination_assign(2,[{R}]) → fill {R} → combination_assign(1,[]) → count != 0 && requirements.is_empty() → None; backtracking exhausts → None → can_cover_shards_with_activatable_mana returns (false, _). So any AnyOneColor/AnyCombination source whose producible count exceeds the remaining colored shards is treated as unable to pay them at all. This is the common late-game case — Vivi's power (and thus its mana) grows as you cast — so a power-3+ Vivi can no longer pay a 2-color spell. Why it matters: a false-negative castability for the exact card class the PR exists to support, and it's a regression relative to nothing only because the path was previously rejected wholesale (#1234), but it ships the feature silently-incorrect for the over-production sub-class. Suggested fix: in combination_assign, succeed as soon as requirements.is_empty() regardless of leftover count (surplus mana is just unused), returning consumed = count_used; the test gap is real — neither vivi_combination_… (exact, count==shards) nor vivi_single_blue_… (power 1) exercises count > shards. Add e.g. power-3 Vivi vs {U}{R} must be castable.

[LOW] Global (not per-source) generic netting. As noted under #4 above — saturating_sub(shard_consumed) nets total consumed against total capacity; harmless false-negative direction, but a per-source accounting would be tighter. Optional.

Net: the two MED architectural/perf findings from 00:20Z stand, and the new combination_assign over-production false-negative should be fixed before merge (it breaks the headline card class for power > residual-shards). The CR-106.6 restriction work and the double-count fix are solid additions.

[natefinch]

review-impl (independent, head a1013c18)

Reviewed the ground-truth .diff in an isolated worktree at PR head a1013c18. No Rust toolchain in this environment, so the findings below are confirmed by code-tracing rather than a live run. Gemini is quota-blocked; I confirmed/refuted @matthewevans's prior findings against the current head and they fold into mine.

VERDICT: request-changes

Confirmed addressed since the prior reviews (good)

• Flexible-mana _ => Exact collapse — profile_kind_from_production (mana_sources.rs:213-300) now maps ChosenColor / OpponentLandColors / AnyTypeProduceableBy / ChoiceAmongExiledColors / AnyInCommandersColorIdentity / AnyOneColorAmongPermanents to AnyOneColor { count, options } via their full option set. ✅
• Single-source double-count — can_cover_shards_with_activatable_mana returns (covered, consumed_pips) and the generic check does .sum::<u32>().saturating_sub(shard_consumed) (casting.rs:9451-9458), with a discriminating test (vivi_power_two_does_not_cover_colored_shards_plus_generic). ✅
• Seam/CR grounding correct: castability stays in the engine, the auto-tap fast path still short-circuits (casting.rs:9377) so the common case is untouched, and CR 106.6 / 117.1d / 601.2g verify against docs/MagicCompRules.txt.

Blocking finding

[HIGH] combination_assign rejects a flexible source that produces MORE mana than the residual colored shards — breaks the headline card class. Evidence: mana_sources.rs:1138-1168. Success requires count == 0 && requirements.is_empty() simultaneously; when requirements empties while count > 0 the if requirements.is_empty() { return None; } guard (1150-1152) rejects the surplus. Trace a power-3 Vivi paying {U}{R} (count=3, requirements=[{U},{R}]): assign {U} → combination_assign(2,[{R}]) → assign {R} → combination_assign(1,[]) → count != 0 && requirements empty → None; all branches exhaust → (false, 0) → can_feasibly_pay_mana_cost_without_x returns false. So any AnyOneColor/AnyCombination source whose producible count exceeds the remaining colored shards is treated as unable to pay them at all. Vivi's power (and thus its mana) grows as you cast, so a power-3+ Vivi can no longer pay a 2-color spell — the exact card the PR is named for, under common play. Why it matters: silent false-negative castability for the headline sub-class (over-production). Test gap: neither vivi_combination_… (count==shards) nor vivi_single_blue_… (power 1) exercises count > shards. Suggested fix: in combination_assign, succeed as soon as requirements.is_empty() regardless of leftover count (surplus is unused), and add a power-3 Vivi vs {U}{R} must-be-castable test.

Non-blocking but should be seen

[MED] Non-tap castability path is unbounded-exponential and clones state per source, in the auto-pass hot path. assign_profiles_to_requirements (mana_sources.rs:1110-1136) branches skip-or-use per object (O(2^P), P = activatable non-tap sources) and combination_assign recurses per-requirement × per-color with no memoization (R!-class). It runs once the auto-tap fast path fails (i.e., the uncastable colored spell), and auto_pass_recommended → has_feasibly_castable_spell calls can_cast_object_now for every spell in hand each priority window. Profile building also clones: activatable_mana_profiles_for_object → can_activate_mana_ability_now does state.clone() + full activation sim (mana_abilities.rs:1019), gated only by the cheap mana_ability_ready_without_simulation pre-check — so it's M × full-state-clone per spell before the search starts. Worst case (several mana rocks + uncastable colored spell) exhausts 2^P × R! per uncastable spell per tick. P/R are small in practice, but this is the documented mana-readiness cliff. Suggested fix: memoize on (object_index, requirements) or add a greedy bipartite-match fast path, and bound P.

[MED] Parallel shard-assignment solver rather than an extension of the existing one. auto_tap_mana_sources_inner (casting_costs.rs) already assigns sources to shards including flexible/filter-land rows; this PR adds a second, independent matcher on a fresh ActivatableManaProfile representation (mana_sources.rs:189-523). Two solvers for "can these sources pay these shards" will drift. The non-tap gap (auto-tap can't auto-sacrifice/discard) is real, but the shard-matching core should reuse the existing row model. (Architectural; not a merge blocker on its own.)

[LOW] Global (not per-source) generic netting. saturating_sub(shard_consumed) nets total consumed against total capacity, so it can under-count generic when shard coverage and generic capacity draw from disjoint sources. False-negative direction, consistent with the over-count philosophy — optional, worth a comment.

Net: the addressed double-count and CR-106.6 work are solid, but the combination_assign over-production false-negative breaks the named card class under common conditions and must be fixed (with a count>shards test) before merge.

[matthewevans]

Maintainer note: pushed f26e320 fixing the blocker from both reviews — combination_assign now succeeds once all shard requirements are satisfied regardless of leftover production (surplus mana is unused), and the missing over-production case is pinned by vivi_power_three_surplus_still_covers_two_shards (empirically fails on the prior head). The two MED architectural notes (exponential worst-case in the non-tap castability search; parallel shard solver vs auto_tap_mana_sources_inner) stand as documented follow-ups — both reviewers marked them non-blocking; a memoized/greedy fast path and solver unification are tracked for a dedicated pass given the mana-readiness perf history.

[matthewevans]

Maintainer sign-off: real discriminating fix for the non-tap combination-mana castability class (#583); blocker (over-production false negative) fixed in f26e320 with revert-verified test; CR 106.6 work validated by two reviewers; MED perf/architecture follow-ups documented on the PR.