feat: prompt for sacrificial mana sources - #6801
Conversation
📝 WalkthroughWalkthroughThis change adds explicit mana-source selection with output provenance, sacrificial-mana-aware automatic payment, new engine and client actions, AI handling, persisted payment preferences, localized UI, wire-protocol updates, and integration coverage. ChangesSacrificial mana selection
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Parse changes introduced by this PR✓ No card-parse changes detected. |
d6afe79 to
db4c458
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/ai_support/mod.rs (1)
996-1037: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ActivateManaSourcenever reaches the mana-priority hold checksgrouped_mana_requires_priorityandbeneficial_mana_tap_trigger_holdare fed byactivatable_mana_actions_for_playerviaactivatable_object_mana_actions, and that sweep only emitsTapLandForManaandActivateAbility. The newActivateManaSourcebranch in those checks is dead code; only the separateclassify_flat_priority_actionpath can see that action shape.🤖 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/ai_support/mod.rs` around lines 996 - 1037, The ActivateManaSource branch in grouped_mana_requires_priority is unreachable because activatable_mana_actions_for_player only emits TapLandForMana and ActivateAbility. Remove that dead branch and keep the existing handling for the two action variants that the sweep can produce; apply the same cleanup to beneficial_mana_tap_trigger_hold if it contains the corresponding branch.
🧹 Nitpick comments (5)
crates/server-core/src/game_action_payload_guard.rs (1)
568-570: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd direct regression coverage for
ActivateManaSource.The supplied tests exercise
TapLandForMana, but not the newly guarded action. Add oversized/invalid and valid-boundaryActivateManaSourceselections to ensure this wire-level protection remains enforced.As per path instructions, tests must exercise the failure path the fix prevents.
🤖 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/server-core/src/game_action_payload_guard.rs` around lines 568 - 570, Add direct regression tests for the ActivateManaSource branch of game-action payload validation, covering oversized or invalid selections that must be rejected and valid boundary selections that must be accepted. Ensure the tests invoke the wire-level guard path through ActivateManaSource rather than only testing TapLandForMana.Source: Path instructions
crates/engine/src/game/mana_sources.rs (2)
455-489: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant whole-board recomputation in the staleness gate.
The initial
activatable_mana_source_selections(state, player).contains(selection)check recomputes every activatable mana selection for every object the player controls, purely to test membership. The code immediately after computescurrent_mana_source_optionsscoped toselection.source.object_idand filters for an equalmanual_selection_for_option— sinceManaSourceSelectionequality includessource(pinning the object), no selection could pass the first check without also matching the second, so the first check can't change the outcome and is pure overhead. This function is invoked once per mana-related candidate during interaction payload projection (project_action_payloadcalls it for everyTapLandForMana/ActivateManaSourcecandidate), so on boards with many mana sources this multiplies into repeated O(battlefield) rescans on a hot UI-refresh path.♻️ Suggested fix
pub(crate) fn live_mana_source_option_for_selection( state: &GameState, player: PlayerId, selection: &ManaSourceSelection, ) -> Result<ManaSourceOption, EngineError> { - if !activatable_mana_source_selections(state, player).contains(selection) { - return Err(EngineError::ActionNotAllowed( - "Mana source selection is stale or no longer legal".to_string(), - )); - } - let aura_sources = taps_for_mana_trigger_sources(state); let gates = mana_abilities::ManaActivationGates::compute(state); let object_id = selection.source.object_id; let options = current_mana_source_options(state, player, object_id, &aura_sources, &gates);🤖 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_sources.rs` around lines 455 - 489, Remove the redundant activatable_mana_source_selections membership check from live_mana_source_option_for_selection. Rely on the object-scoped current_mana_source_options filtering and existing no-match error to reject stale or illegal selections, while preserving the ambiguity check and its error behavior.
417-453: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCentralize deferred-output detection on
ManaProductionIn
crates/engine/src/game/mana_sources.rs:421-450, replace the hand-maintainedmatches!()allow-list with an exhaustive helper onManaProduction(for examplehas_deferred_output). That makes any new flexible-color variant a compile-time update instead of silently falling back to a concrete color.🤖 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_sources.rs` around lines 417 - 453, Replace the hand-maintained Effect::Mana matches! allow-list in manual_selection_for_option with an exhaustive ManaProduction helper such as has_deferred_output. Invoke it on the production value for the selected ability, preserving the existing Colorless and DeferredColorChoice assignments when deferred output is detected; implement the helper exhaustively on ManaProduction so new flexible variants require an explicit compile-time update.Source: Path instructions
crates/engine/src/game/casting_costs.rs (1)
8657-8677: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider an exhaustive
matchonCastPaymentModeinstead of if-equality.The dispatch here (and the sibling
manual_payment/auto_payment_needs_inputchecks above) testspayment_modewith==and falls through unconditionally tofinalize_automatic_mana_paymentfor everything else. A futureCastPaymentModevariant would silently inheritAutobehavior instead of failing to compile. As per path instructions forcrates/engine/**/*.rs, "Use exhaustivematchexpressions without wildcard fallbacks when matching known enums, allowing the compiler to detect missing variants."🤖 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/casting_costs.rs` around lines 8657 - 8677, Replace the equality-based CastPaymentMode dispatch in this block and the sibling manual_payment and auto_payment_needs_input checks with exhaustive match expressions. Keep the existing AutoExceptSacrificialMana and Auto behavior unchanged, explicitly handle every current CastPaymentMode variant, and omit wildcard fallbacks so future variants cause compile-time errors.Source: Path instructions
crates/engine/src/game/interaction.rs (1)
1999-2027: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing
MAX_INTERACTION_LIST_LENguard forManaSourceSelection's offered actions.Every other list-shaped arm in
direct_choice_projection(e.g.UntapChoice,RespondToPrecastCopyShortcut) checks candidate count againstMAX_INTERACTION_LIST_LENbefore building the action list. This newManaSourceSelectionarm builds oneActivateManaSourceperoptionsentry with no such check, breaking that invariant on boards with many sacrifice-mana sources.🛡️ Proposed fix
WaitingFor::ManaSourceSelection { player, options, .. } => { if *player != semantic_owner { return Err(InteractionReasonCode::InvalidAuthorityState); } + if options.len() >= MAX_INTERACTION_LIST_LEN { + return Err(InteractionReasonCode::PayloadTooLarge); + } let mut actions = options .iter() .cloned() .map(|selection| GameAction::ActivateManaSource { selection }) .collect::<Vec<_>>(); actions.push(GameAction::BackToManaPayment); actions }🤖 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/interaction.rs` around lines 1999 - 2027, Update the WaitingFor::ManaSourceSelection arm in direct_choice_projection to validate options.len() against MAX_INTERACTION_LIST_LEN before constructing actions, returning the same established limit error used by the other list-shaped arms when exceeded; preserve the existing ActivateManaSource entries and BackToManaPayment action for valid sizes.
🤖 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.
Inline comments:
In `@client/src/components/mana/ManaPaymentUI.tsx`:
- Around line 627-633: Update ManaSourceSelectionUI to call
useCanActForWaitingState() and return null when it indicates the client cannot
act, matching the adjacent mana-payment prompt while preserving the existing
waitingFor type check and selection UI behavior for the acting player.
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 8657-8677: In the AutoExceptSacrificialMana branch after
auto_tap_non_sacrificial_mana_sources, check
mana_ability_cost_payment_is_paused(state) and return the existing
paused-payment result before evaluating pool payment or constructing
ManaSourceSelection. Preserve the in-flight waiting_for state instead of
entering fallback selection when auto-tapping suspends payment.
In `@crates/engine/src/types/actions.rs`:
- Around line 259-267: Move the ActivateManaSource and BackToManaPayment
variants to the end of the GameAction enum in actions.rs, preserving their
definitions and behavior while leaving all existing variants in their current
declaration order so GameActionKind-derived ordering remains stable.
---
Outside diff comments:
In `@crates/engine/src/ai_support/mod.rs`:
- Around line 996-1037: The ActivateManaSource branch in
grouped_mana_requires_priority is unreachable because
activatable_mana_actions_for_player only emits TapLandForMana and
ActivateAbility. Remove that dead branch and keep the existing handling for the
two action variants that the sweep can produce; apply the same cleanup to
beneficial_mana_tap_trigger_hold if it contains the corresponding branch.
---
Nitpick comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 8657-8677: Replace the equality-based CastPaymentMode dispatch in
this block and the sibling manual_payment and auto_payment_needs_input checks
with exhaustive match expressions. Keep the existing AutoExceptSacrificialMana
and Auto behavior unchanged, explicitly handle every current CastPaymentMode
variant, and omit wildcard fallbacks so future variants cause compile-time
errors.
In `@crates/engine/src/game/interaction.rs`:
- Around line 1999-2027: Update the WaitingFor::ManaSourceSelection arm in
direct_choice_projection to validate options.len() against
MAX_INTERACTION_LIST_LEN before constructing actions, returning the same
established limit error used by the other list-shaped arms when exceeded;
preserve the existing ActivateManaSource entries and BackToManaPayment action
for valid sizes.
In `@crates/engine/src/game/mana_sources.rs`:
- Around line 455-489: Remove the redundant activatable_mana_source_selections
membership check from live_mana_source_option_for_selection. Rely on the
object-scoped current_mana_source_options filtering and existing no-match error
to reject stale or illegal selections, while preserving the ambiguity check and
its error behavior.
- Around line 417-453: Replace the hand-maintained Effect::Mana matches!
allow-list in manual_selection_for_option with an exhaustive ManaProduction
helper such as has_deferred_output. Invoke it on the production value for the
selected ability, preserving the existing Colorless and DeferredColorChoice
assignments when deferred output is detected; implement the helper exhaustively
on ManaProduction so new flexible variants require an explicit compile-time
update.
In `@crates/server-core/src/game_action_payload_guard.rs`:
- Around line 568-570: Add direct regression tests for the ActivateManaSource
branch of game-action payload validation, covering oversized or invalid
selections that must be rejected and valid boundary selections that must be
accepted. Ensure the tests invoke the wire-level guard path through
ActivateManaSource rather than only testing TapLandForMana.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 16fae288-209b-458e-9464-bac5d4c6143a
⛔ Files ignored due to path filters (2)
client/src/adapter/generated/interaction/index.tsis excluded by!**/generated/**crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gz
📒 Files selected for processing (62)
client/src/adapter/__tests__/manaSourceSelectionWireTypes.test.tsclient/src/adapter/types.tsclient/src/components/board/__tests__/PermanentCard.test.tsxclient/src/components/mana/ManaPaymentUI.tsxclient/src/components/mana/__tests__/ManaPaymentUI.test.tsxclient/src/components/settings/PreferencesModal.tsxclient/src/game/__tests__/castPaymentMode.test.tsclient/src/game/castPaymentMode.tsclient/src/game/controllers/aiController.tsclient/src/game/waitingForRegistry.tsclient/src/hooks/__tests__/useKeyboardShortcuts.test.tsxclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/de/settings.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/en/settings.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/es/settings.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/fr/settings.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/it/settings.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pl/settings.jsonclient/src/i18n/locales/pt/game.jsonclient/src/i18n/locales/pt/settings.jsonclient/src/network/__tests__/protocol.test.tsclient/src/network/protocol.tsclient/src/pages/GamePage.tsxclient/src/stores/preferencesStore.tsclient/src/viewmodel/__tests__/cardActionChoice.test.tscrates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/filter.rscrates/engine/src/ai_support/mod.rscrates/engine/src/ai_support/payment_continuation.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/costs.rscrates/engine/src/game/derived.rscrates/engine/src/game/effects/prepare.rscrates/engine/src/game/engine.rscrates/engine/src/game/interaction.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_sources.rscrates/engine/src/game/replay.rscrates/engine/src/game/scenario.rscrates/engine/src/types/action_stable_order.rscrates/engine/src/types/actions.rscrates/engine/src/types/game_state.rscrates/engine/src/types/interaction.rscrates/engine/src/types/mana.rscrates/engine/src/types/mod.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/sacrificial_mana_choice.rscrates/manabrew-compat/src/lib.rscrates/phase-ai/src/decision_kind.rscrates/phase-ai/src/policies/discard_payoff.rscrates/phase-ai/src/policies/draw_payoff.rscrates/phase-ai/src/search.rscrates/server-core/src/client_message_wire_guard.rscrates/server-core/src/game_action_payload_guard.rscrates/server-core/src/protocol.rscrates/server-core/tests/game_action_payload_guard.rs
| export function ManaSourceSelectionUI() { | ||
| const { t } = useTranslation("game"); | ||
| const waitingFor = useGameStore((s) => s.waitingFor); | ||
| const gameState = useGameStore((s) => s.gameState); | ||
| const dispatch = useGameStore((s) => s.dispatch); | ||
|
|
||
| if (waitingFor?.type !== "ManaSourceSelection") return null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Gate the selection dialog to the acting player.
ManaSourceSelectionUI renders actionable controls for every client with this waiting state. Reuse useCanActForWaitingState() and return null when it is false, as the adjacent mana-payment prompt does.
Proposed fix
export function ManaSourceSelectionUI() {
const { t } = useTranslation("game");
const waitingFor = useGameStore((s) => s.waitingFor);
const gameState = useGameStore((s) => s.gameState);
const dispatch = useGameStore((s) => s.dispatch);
+ const canAct = useCanActForWaitingState();
- if (waitingFor?.type !== "ManaSourceSelection") return null;
+ if (waitingFor?.type !== "ManaSourceSelection" || !canAct) return null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function ManaSourceSelectionUI() { | |
| const { t } = useTranslation("game"); | |
| const waitingFor = useGameStore((s) => s.waitingFor); | |
| const gameState = useGameStore((s) => s.gameState); | |
| const dispatch = useGameStore((s) => s.dispatch); | |
| if (waitingFor?.type !== "ManaSourceSelection") return null; | |
| export function ManaSourceSelectionUI() { | |
| const { t } = useTranslation("game"); | |
| const waitingFor = useGameStore((s) => s.waitingFor); | |
| const gameState = useGameStore((s) => s.gameState); | |
| const dispatch = useGameStore((s) => s.dispatch); | |
| const canAct = useCanActForWaitingState(); | |
| if (waitingFor?.type !== "ManaSourceSelection" || !canAct) return null; |
🤖 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 `@client/src/components/mana/ManaPaymentUI.tsx` around lines 627 - 633, Update
ManaSourceSelectionUI to call useCanActForWaitingState() and return null when it
indicates the client cannot act, matching the adjacent mana-payment prompt while
preserving the existing waitingFor type check and selection UI behavior for the
acting player.
| if payment_mode == CastPaymentMode::AutoExceptSacrificialMana { | ||
| auto_tap_non_sacrificial_mana_sources(state, player, cost, events, object_id); | ||
| if pending_cost_is_payable_from_pool(state, player) { | ||
| return finalize_automatic_mana_payment(state, player, events); | ||
| } | ||
| let options = super::mana_sources::activatable_mana_source_selections(state, player) | ||
| .into_iter() | ||
| .filter(|selection| { | ||
| selection.penalty == super::mana_sources::ManaSourcePenalty::Sacrifices | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| if options.is_empty() { | ||
| return enter_payment_step(state, player, None, events); | ||
| } | ||
| return Ok(WaitingFor::ManaSourceSelection { | ||
| player, | ||
| options, | ||
| convoke_mode: None, | ||
| }); | ||
| } | ||
| finalize_automatic_mana_payment(state, player, events) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "mana_ability_cost_payment_is_paused" crates/engine/src/game/casting_costs.rs -B6 -A2Repository: phase-rs/phase
Length of output: 3573
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '8628,8695p' crates/engine/src/game/casting_costs.rs
printf '\n---\n'
sed -n '10490,10580p' crates/engine/src/game/casting_costs.rs
printf '\n---\n'
sed -n '2360,2390p' crates/engine/src/game/casting_costs.rs
printf '\n---\n'
sed -n '12455,12870p' crates/engine/src/game/casting_costs.rsRepository: phase-rs/phase
Length of output: 25372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "auto_tap_non_sacrificial_mana_sources|finalize_automatic_mana_payment|enter_payment_step|mana_ability_cost_payment_is_paused" crates/engine/src/game/casting_costs.rs -n -A3 -B3Repository: phase-rs/phase
Length of output: 10146
Preserve the paused payment cursor here.
auto_tap_non_sacrificial_mana_sources can still suspend on a replacement-choice cost move, so this branch needs the same early mana_ability_cost_payment_is_paused(state) return used elsewhere. Otherwise the fallback can overwrite the in-flight waiting_for state with ManaSourceSelection and lose the suspended payment.
🤖 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/casting_costs.rs` around lines 8657 - 8677, In the
AutoExceptSacrificialMana branch after auto_tap_non_sacrificial_mana_sources,
check mana_ability_cost_payment_is_paused(state) and return the existing
paused-payment result before evaluating pool payment or constructing
ManaSourceSelection. Preserve the in-flight waiting_for state instead of
entering fallback selection when auto-tapping suspends payment.
| /// CR 605.3a: Activate one exact engine-authored mana-source capability. | ||
| /// Unlike the legacy land-only action, this covers mana abilities on every | ||
| /// permanent type and preserves the selected output provenance. | ||
| ActivateManaSource { | ||
| selection: ManaSourceSelection, | ||
| }, | ||
| /// Return from a sacrificial-mana choice to the exact saved payment state | ||
| /// without re-planning or mutating the mana pool. | ||
| BackToManaPayment, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Append these action variants instead of inserting them.
cmp_stable orders by declaration-derived GameActionKind; this insertion shifts every later existing discriminant. That changes AI/legal-action tie-breaking and replay ordering for unrelated actions. Move both variants to the end of GameAction (or pin discriminants).
🤖 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/types/actions.rs` around lines 259 - 267, Move the
ActivateManaSource and BackToManaPayment variants to the end of the GameAction
enum in actions.rs, preserving their definitions and behavior while leaving all
existing variants in their current declaration order so GameActionKind-derived
ordering remains stable.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes