Skip to content

fix(engine): Kozilek's Command castable with Eldrazi Temple restricted mana (#2011) - #2462

Merged
matthewevans merged 5 commits into
phase-rs:mainfrom
kiannidev:fix/2011-eldrazi-temple-kozilek-cast
Jun 5, 2026
Merged

fix(engine): Kozilek's Command castable with Eldrazi Temple restricted mana (#2011)#2462
matthewevans merged 5 commits into
phase-rs:mainfrom
kiannidev:fix/2011-eldrazi-temple-kozilek-cast

Conversation

@kiannidev

Copy link
Copy Markdown
Contributor

Summary

Fixes #2011: the game auto-passed on main phase when the only colorless source was an untapped Eldrazi Temple and Kozilek's Command ({X}{C}{C}) was in hand.

  • can_feasibly_pay_mana_cost: For costs with {X}, evaluate each affordable X (via max_x_value_excluding) on the concretized cost instead of leaving the symbolic {X} shard in the affordability check.
  • emit_source_rows: {T}: Add {C}{C} (and Mixed production) emit an atomic_combination so auto-tap plans one activation for both colorless requirements, including Colorless Eldrazi spend restrictions.

Test plan

  • cargo test -p engine --lib issue_2011_kozilek
  • cargo test -p engine --test integration issue_2011
  • In-game: main phase, only untapped Eldrazi Temple + Kozilek's Command in hand → Cast offered (no auto-pass); tap Temple for {C}{C}, choose X, complete cast

phase-rs#2011)

Affordability now evaluates choosable X values instead of leaving symbolic {X}
in the cost, and auto-tap treats multi-{C} activations as one atomic combination
so restricted Eldrazi Temple mana can pay {C}{C} in a single tap.
@kiannidev
kiannidev requested a review from matthewevans as a code owner June 5, 2026 13:09

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses issue #2011 by ensuring that multi-mana activations from a single source (such as Eldrazi Temple or Sol Ring) are surfaced as a single atomic combination, preventing the auto-tap planner from planning multiple taps of the same source. This is implemented in crates/engine/src/game/mana_sources.rs and verified with a new integration test suite. The review feedback points out that ManaProduction::Fixed should also be included in this match arm to provide sibling coverage for other multi-mana producers like Ravnica bounce lands or Castle Garenbrig.

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.

Comment thread crates/engine/src/game/mana_sources.rs Outdated
kiannidev and others added 2 commits June 5, 2026 06:14
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Break the Fixed | Colorless | Mixed production arm across lines so CI fmt passes.
@matthewevans

Copy link
Copy Markdown
Member

🤖 Architecture Review (automated)

Verdict: ⚠️ Changes requested

Seam: PASS — X-affordability lives in casting.rs and atomic-combination grouping in mana_sources::emit_source_rows; both are the right layers, and the {C}{C} bundle fix generalizes to the whole "one activation → multi-mana" class (Sol Ring, Mixed, bounce lands).
Idiomatic: PASS (mostly) — exhaustive-ish match with named ManaProduction arms, .then_some(), concretize_x clone-then-mutate. One CR annotation is mis-cited (below).
Value: Class-level — covers Fixed/Colorless/Mixed single-activation multi-mana and any {X}… cost with a restricted fixed remainder, not just Kozilek's Command. That breadth is also what introduces the regression.
Reconciled with existing reviews: Gemini (medium) "include ManaProduction::Fixed in the match arm" — REFUTED / already addressed: head mana_sources.rs:1011 already lists ManaProduction::Fixed { .. } | ManaProduction::Colorless { .. } | ManaProduction::Mixed { .. }. The Fixed sibling is present; no action needed.

Findings

  • [BLOCKER] crates/engine/src/game/mana_sources.rs:1008 — The new Fixed | Colorless | Mixed arm regresses an existing passing test. CI "Rust tests" fails on ai_support::tests::legal_actions_by_object_exposes_filter_land_with_payable_mana_sub_cost (mod.rs:1526): Skycloud Expanse ({1},{T}: Add {W}{U}, modeled as Fixed { colors:[White,Blue] }) is no longer surfaced as manually activatable in legal_actions_full. Before this PR Fixed{[W,U]} produced two per-color SourceRows; now it produces one row with mana_type: White, atomic_combination: Some([White,Blue]), and the downstream activatability/feasibility path no longer matches it. The change's blast radius extends past the issue Eldrazi Temple — The game autopasses when I have eldrazi temple untapped as my only colorless source and Kozilek's Comm… #2011 {C}{C} case to every multi-color Fixed filter land. Fix: make the bundled row still satisfy the per-object activatability surface (or restrict atomic grouping to the genuinely-needed multi-{C} / same-type case), then confirm the existing test is green. Do not modify that test to paper over it — it encodes correct behavior on main.
  • [MEDIUM] crates/engine/src/game/mana_sources.rs:1020 (annotation // CR 106.1b:) — CR mis-cite. Verified in docs/MagicCompRules.txt:410: CR 106.1b is "There are six types of mana: white, blue, black, red, green, and colorless." It does not state that one activation's mana must be grouped atomically. The sibling ChoiceAmongCombinations arm cites CR 605.3b + CR 106.1a for the same atomic-row rationale — use the rule that actually governs mana-ability production as a unit, not 106.1b. (Repo treats CR annotations as mandatory and accurate.)
  • [MEDIUM] crates/engine/src/game/casting.rs:8198 — Perf: the new wrapper replaces the top-level auto-tap fast path with (0..=max_x).any(|x| can_feasibly_pay_mana_cost_without_x(concretize(x))), and _without_x clones game state via can_pay_cost_after_auto_tap each iteration. Feasible spells short-circuit at X=0 (fixed {C}{C} shards minimize at X=0), so the common case is one clone — fine. The cost lands on infeasible X-spells with a large max_x: on a high-mana board max_x can be 20-40, so an uncastable X-spell triggers 20-40 state clones per legal_actions enumeration. Not catastrophic (bounded by max_x, only on the uncastable branch), but worth a guard: e.g. short-circuit when the fixed colored/colorless remainder is itself infeasible before sweeping X, since growing X only adds generic and never unlocks an unpayable colored shard. At minimum, confirm legal-action enumeration on a large board with an uncastable X-spell in hand doesn't regress.
  • [NIT] crates/engine/tests/integration/issue_2011_eldrazi_temple_kozilek_cast.rs + the in-casting.rs unit test partially overlap (both build Eldrazi Temple two-ability + Kozilek's Command). Acceptable, but the integration file is the higher-value pipeline test (drives legal_actions/auto_pass_recommended); the in-module unit test asserting can_feasibly_pay_mana_cost_without_x directly is a white-box check of a private helper — keep it only if you want the unit-level guard, otherwise the integration test covers the behavior.

CR verification performed: 601.2f ✓, 107.1b ✓, 117.1d ✓ (all found in docs/MagicCompRules.txt); 106.1b ✓ exists but does not support the cited claim (see MEDIUM above).

@matthewevans

Copy link
Copy Markdown
Member

Pushed maintainer follow-up commit 121adfff5 addressing the review findings:\n\n- kept the one-activation multi-mana model as atomic_combination, including Fixed, but changed legal_actions_full so atomic or mana-sub-cost land abilities surface as ActivateAbility instead of the simple TapLandForMana shortcut; this fixes the Skycloud Expanse regression without weakening the payment planner model\n- corrected the atomic-combination CR annotation to cite the mana-ability production unit plus mana type/color rules\n- removed the hot-path sweep over every possible X value; castability now probes the cheapest concrete X=0 cost once, since larger X only adds generic mana and cannot make an unpayable fixed remainder payable\n\nLocal verification: cargo fmt --all; Tilt test-engine, clippy, and card-data all green after the commit.

@matthewevans matthewevans added bug Bug fix ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow labels Jun 5, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer sign-off after follow-up commit 121adff: review findings addressed at the existing casting/mana-source/legal-action seams; local Tilt clippy, test-engine, and card-data are green.

@matthewevans
matthewevans enabled auto-merge June 5, 2026 13:50
auto-merge was automatically disabled June 5, 2026 13:56

Head branch was pushed to by a user without write access

@kiannidev
kiannidev requested a review from matthewevans June 5, 2026 13:59

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer sign-off on current head 3b9e283: same tree as the locally validated merge with origin/main; follow-up fixes remain in 121adff and local Tilt clippy, test-engine, and card-data were green after conflict resolution.

@matthewevans
matthewevans enabled auto-merge June 5, 2026 14:01
@matthewevans
matthewevans added this pull request to the merge queue Jun 5, 2026
Merged via the queue into phase-rs:main with commit 7999dfd Jun 5, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Eldrazi Temple — The game autopasses when I have eldrazi temple untapped as my only colorless source and Kozilek's Comm…

2 participants