fix(engine): hold priority for mana on opponent's turn (CR 117.1d, #4388)#4702
fix(engine): hold priority for mana on opponent's turn (CR 117.1d, #4388)#4702andriypolanski wants to merge 3 commits into
Conversation
…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>
There was a problem hiding this comment.
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.
| // 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; | ||
| } |
There was a problem hiding this comment.
[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.
| // 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; | |
| } |
| assert!( | ||
| !auto_pass_recommended(runner.state(), &flat), | ||
| "CR 117.1d: auto-pass must not fire on opponent's turn when mana is activatable (#4388)" | ||
| ); |
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
[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.
|
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. |
Summary
auto_pass_recommendedtreated 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 inlegal_actions_by_object.activatable_object_mana_actionsis non-empty, do not recommend auto-pass (CR 117.1d).Changed files
crates/engine/src/ai_support/mod.rscrates/engine/tests/integration/issue_4388_mana_on_opponents_turn.rscrates/engine/tests/integration/main.rsTest plan
cargo test -p engine --test integration issue_4388cargo test -p engine auto_pass_holds_priority_for_groupedcargo test -p engine auto_pass_does_not_skip_non_mana(existing behavior preserved)Rules