Skip to content

fix(engine): hold priority for mana on opponent's turn (CR 117.1d, #4388)#4702

Closed
andriypolanski wants to merge 3 commits into
phase-rs:mainfrom
andriypolanski:fix/4388-mana-activation-opponents-turn
Closed

fix(engine): hold priority for mana on opponent's turn (CR 117.1d, #4388)#4702
andriypolanski wants to merge 3 commits into
phase-rs:mainfrom
andriypolanski:fix/4388-mana-activation-opponents-turn

Conversation

@andriypolanski

@andriypolanski andriypolanski commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Root cause: auto_pass_recommended treated standalone mana abilities (Gaea's Cradle, Itlimoc, basic lands) as non-meaningful priority actions. On an opponent's turn, the client auto-passed through priority windows before the player could tap those lands, even though the engine already exposed the actions in legal_actions_by_object.
  • Fix: When the priority player is not the active player and activatable_object_mana_actions is non-empty, do not recommend auto-pass (CR 117.1d).
  • Scope: Engine-only; no parser or frontend changes. Sacrifice-for-mana (KCI, Krark-Clan Ironworks — The game auto-passes in this board state when it shouldn't, because I can activate Krark-Clan Ir… #544) behavior unchanged. Auto-pass on your own turn with only mana available is unchanged.

Changed files

File Change
crates/engine/src/ai_support/mod.rs Hold priority when grouped mana is available on opponent's turn
crates/engine/tests/integration/issue_4388_mana_on_opponents_turn.rs Integration regression (legal actions + auto-pass + activation)
crates/engine/tests/integration/main.rs Register integration module

Test plan

  • cargo test -p engine --test integration issue_4388
  • cargo test -p engine auto_pass_holds_priority_for_grouped
  • cargo test -p engine auto_pass_does_not_skip_non_mana (existing behavior preserved)
  • Manual: on opponent's turn with priority, tap Gaea's Cradle / Itlimoc without Full Control enabled
  • Manual: on your own main phase with only mana available, auto-pass still advances as before

Rules

  • CR 117.1d — priority player may activate their mana abilities during another player's turn
  • CR 605.3a — mana abilities do not use the stack

…ase-rs#4388)

Auto-pass skipped priority windows on an opponent's turn before grouped-only
mana actions (Gaea's Cradle, Itlimoc, basic lands) could be tapped.

Co-authored-by: Cursor <cursoragent@cursor.com>

@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 #4388 by ensuring that auto-pass does not prematurely skip priority on an opponent's turn when the player has activatable mana abilities (such as Gaea's Cradle or basic lands), in accordance with CR 117.1d. Feedback on the changes highlights a performance and usability regression in the implementation: checking for activatable mana abilities when the player's hand is empty unnecessarily disables auto-pass and introduces performance lag. It is recommended to gate this check on whether the player has cards in hand and to update the corresponding tests to reflect this logic.

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/ai_support/mod.rs Outdated
Comment on lines +792 to +799
// CR 117.1d (issue #4388): On an opponent's turn the priority player may
// activate their own mana abilities (Gaea's Cradle, Itlimoc, basic lands).
// Those actions live only in `legal_actions_by_object`, not the flat list
// consumed here — without this guard auto-pass fires through the window
// before the frontend can offer a tap.
if state.active_player != player && !activatable_object_mana_actions(state).is_empty() {
return false;
}

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.

high

[HIGH] Severe usability and performance regression in auto-pass logic on opponent's turn. Evidence: crates/engine/src/ai_support/mod.rs:797.

Why it matters: Returning false early whenever any land is untapped on an opponent's turn completely disables auto-pass (even with an empty hand), forcing tedious manual passes on every step, while calling the expensive activatable_object_mana_actions (which can clone the state) on every priority check causes severe lag.

Suggested fix: Only hold priority if the player has cards in hand or other potential actions, and use a cheap, non-simulating check for untapped mana sources.

Suggested change
// CR 117.1d (issue #4388): On an opponent's turn the priority player may
// activate their own mana abilities (Gaea's Cradle, Itlimoc, basic lands).
// Those actions live only in `legal_actions_by_object`, not the flat list
// consumed here — without this guard auto-pass fires through the window
// before the frontend can offer a tap.
if state.active_player != player && !activatable_object_mana_actions(state).is_empty() {
return false;
}
if state.active_player != player && !state.players[player.0 as usize].hand.is_empty() && !activatable_object_mana_actions(state).is_empty() {
return false;
}

Comment on lines +95 to +98
assert!(
!auto_pass_recommended(runner.state(), &flat),
"CR 117.1d: auto-pass must not fire on opponent's turn when mana is activatable (#4388)"
);

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.

high

[HIGH] Test asserts a severe usability regression where auto-pass is blocked with an empty hand. Evidence: crates/engine/tests/integration/issue_4388_mana_on_opponents_turn.rs:95.

Why it matters: Enforcing that auto-pass is blocked on the opponent's turn when the player has only untapped lands and no cards in hand codifies a highly undesirable user experience.

Suggested fix: Update the test scenario to include a card in hand (e.g., an instant) to justify holding priority.

Co-authored-by: Cursor <cursoragent@cursor.com>

@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.

[HIGH] Opponent-turn ordinary mana now disables frontend auto-pass whenever any mana source is available. Evidence: crates/engine/src/ai_support/mod.rs:797 and client/src/game/controllers/gameLoopController.ts:110. Why it matters: the default game loop will stop on every opponent priority window for any untapped land or mana ability even when the only available action is standalone mana, regressing normal play and performance. Suggested fix: keep grouped mana actions exposed, but only block auto-pass for meaningful mana activations, such as the existing sacrifice-for-mana path, or when the user explicitly sets full control/phase stops.

Reviewed current head 68272d5250b3285674a3968944677cc53d6f2716.

@matthewevans matthewevans added the bug Bug fix label Jul 1, 2026
@matthewevans

Copy link
Copy Markdown
Member

Thanks for the contribution. This PR is currently test-only against current main; it does not include a production behavior change. We're closing standalone test-only PRs so the contribution queue stays focused on fixes that change behavior. If you want to continue on #4388, please reopen with the behavioral fix and focused regression coverage together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants