Fix activated ability target and cost ordering - #6749
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughActivated abilities now select targets before paying costs, defer X-dependent target construction, stage activation triggers in an activation-local transaction, commit them at stack entry, and reject cancellation after an activation cost is committed. Tests cover ordering, replacement pauses, trigger settlement, and cancellation. ChangesActivated ability payment flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant Casting
participant TargetSelection
participant CostPayment
participant TriggerSession
participant Stack
Player->>Casting: announce activation
Casting->>TargetSelection: select or assign targets
TargetSelection->>TriggerSession: stage targeting events
TargetSelection->>CostPayment: continue at payment boundary
CostPayment->>TriggerSession: stage cost events
CostPayment->>Stack: push activated ability
Stack->>TriggerSession: commit pending triggers
Possibly related PRs
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/engine/src/game/engine_modes.rs (1)
262-269: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSurface interactive activation costs before pushing modal abilities
finish_activated_ability_at_payment_boundaryonly finalizes mana and mana-leg payment. The new direct modal path can skip the next unpaid interactive component, so a modal activated ability withSacrifice, discard, or exile costs can reach the stack unpaid.crates/engine/src/game/engine_modes.rs:262-269🤖 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/engine_modes.rs` around lines 262 - 269, Update the direct modal activation branch that constructs PendingCast before calling finish_activated_ability_at_payment_boundary. Ensure interactive costs such as Sacrifice, discard, and exile are surfaced and paid through the existing interactive-cost handling before the modal ability is pushed, while preserving the current mana and mana-leg finalization flow.crates/engine/src/game/casting.rs (1)
17250-17617: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMirror the interactive-cost dispatcher in the target-selected path
pay_activation_costs_after_target_selectiononly re-surfaces non-self exile and unattach. Collect evidence, craft materials, OneOf, return-to-hand, remove-counter, tap, and Waterbend still depend on the!has_effect_targetsbranch, so targeted activations can skip their cost choices beforefinish_activated_ability_at_payment_boundary.🤖 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.rs` around lines 17250 - 17617, Mirror the interactive-cost dispatch used in the no-target branch within pay_activation_costs_after_target_selection, so targeted activations surface every remaining interactive cost before finish_activated_ability_at_payment_boundary. Reuse the existing specialized handlers or surface_next_unpaid_interactive_activation_cost for collect evidence, craft materials, OneOf, return-to-hand, remove-counter, tap, and Waterbend, while preserving the existing non-self exile and unattach handling.crates/engine/src/game/casting_costs.rs (1)
2602-2708: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
mark_activation_cost_committed()incorrectly gated onSelectedNonSelf, leaving other resumed sacrifice completions uncommitted.At line 2695 the commit call is wrapped in
matches!(completion, PendingSacrificeCostCompletion::SelectedNonSelf) && pending.activation_ability_index.is_some(). That guard is appropriate for theremove_selected_non_self_sacrifice_costhousekeeping on the same line, but it should not also gate the commit flag — comparehandle_sacrifice_for_cost's own no-pause completion (Line 2965-2971), which commits unconditionally wheneveractivation_ability_index.is_some(). As written, a sacrifice cost that is interrupted by a replacement choice mid-payment and resumes throughfinish_sacrifice_for_costwith any completion kind other thanSelectedNonSelf(e.g. a self-sacrifice component of an activation cost) never marks the cost committed, even though the permanent has already been sacrificed — leavingCancelCastfree to be accepted after an irreversible cost was paid.🐛 Proposed fix
if matches!(completion, PendingSacrificeCostCompletion::SelectedNonSelf) && pending.activation_ability_index.is_some() { - pending.mark_activation_cost_committed(); pending.activation_cost = pending .activation_cost .take() .and_then(super::casting::remove_selected_non_self_sacrifice_cost); } + if pending.activation_ability_index.is_some() { + pending.mark_activation_cost_committed(); + }🤖 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 2602 - 2708, Update finish_sacrifice_for_cost so pending.mark_activation_cost_committed() runs whenever pending.activation_ability_index.is_some(), regardless of completion. Keep the SelectedNonSelf condition only around remove_selected_non_self_sacrifice_cost, matching the unconditional commit behavior in handle_sacrifice_for_cost.crates/engine/src/game/casting_targets.rs (1)
360-375: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPark target-declaration events before returning
DistributeAmong. Ifmaybe_pause_for_cast_distribution(...)pauses here, the precedingemit_targeting_eventsnever reaches the post-action pipeline on this action, so target-trigger observers can miss the event entirely.
crates/engine/src/game/casting_targets.rs:360-375crates/engine/src/game/casting_targets.rs:467-483🤖 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_targets.rs` around lines 360 - 375, The target-declaration events emitted by emit_targeting_events must be preserved when maybe_pause_for_cast_distribution returns DistributeAmong instead of being lost before the post-action pipeline. Update both affected sites in crates/engine/src/game/casting_targets.rs:360-375 and crates/engine/src/game/casting_targets.rs:467-483 so the events are parked on the paused action/state and released through the normal post-action path when distribution resumes; keep immediate emission unchanged when no pause occurs.
🧹 Nitpick comments (3)
crates/engine/src/game/triggers.rs (2)
736-742: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffJournal replay re-runs full layer flush + off-zone reconciliation per collected slice.
Every
collectclones the state and replays the whole journal; each replayedPrepareEventBatchre-executesflush_layersandreconcile_off_zone_keyword_triggersover the full board. With N payment slices this is O(N²) layer work for a single activation. Worth measuring on a wide board before this path becomes the default for all activations — caching the staged state between slices (invalidated on live-state mutation) would avoid the replay entirely.🤖 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/triggers.rs` around lines 736 - 742, Optimize rebuild_staged_state and the surrounding collect flow to avoid replaying the entire operation_journal for every payment slice: cache the staged GameState between slices and reuse it until the live state mutates, invalidating and rebuilding the cache at that point. Preserve journal ordering and existing PrepareEventBatch layer flushing and off-zone reconciliation behavior.
496-517: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing batched-zone-change index authority instead of re-deriving it here.
record_matched_batched_zone_change_replay/record_batched_zone_change_collected(Lines 1549-1574) already own "whichturn_zone_change_indexvalues a matched batched trigger claims". Re-deriving the sameZoneChangedscan in the session creates a second authority that can drift from the guard inbatched_zone_change_already_collected.♻️ Suggested consolidation
- let turn_zone_change_indices = matched - .trigger_events - .iter() - .filter_map(|event| match event { - GameEvent::ZoneChanged { record, .. } => Some(record.turn_zone_change_index), - _ => None, - }) - .collect(); + let turn_zone_change_indices = batched_zone_change_indices(&matched.trigger_events);and have
record_batched_zone_change_collectedconsume the same helper.🤖 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/triggers.rs` around lines 496 - 517, Consolidate batched zone-change index derivation by adding or reusing a shared helper owned by the existing `record_matched_batched_zone_change_replay`/`record_batched_zone_change_collected` flow. Update `record_matched_batched_zone_change_replay` to use that helper instead of locally scanning `matched.trigger_events`, and make `record_batched_zone_change_collected` and `batched_zone_change_already_collected` consume the same authority so all paths claim identical `turn_zone_change_index` values.crates/engine/src/game/casting_costs.rs (1)
4457-4700: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated target-settle-and-route boilerplate across 5 call sites.
The 3-4 line sequence "
pending.begin_activation_trigger_collection(); emit_targeting_events(...); finish_target_selected_activated_ability_at_payment_boundary(...)" is duplicated three times in this function (assigned/random/auto-select branches) and twice more inbegin_deferred_target_selection(Line 1591-1635). Consider extracting a small helper (e.g.settle_targets_and_route_to_payment(state, player, pending, targets, source_id, events)) shared by both functions to reduce duplication and the risk of the two copies drifting apart.🤖 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 4457 - 4700, Extract the repeated target-settlement sequence from push_activated_ability_to_stack and begin_deferred_target_selection into a shared helper, such as settle_targets_and_route_to_payment. The helper should begin the activation trigger collection, emit targeting events for the supplied targets, and call finish_target_selected_activated_ability_at_payment_boundary; replace all assigned, random, and auto-select call-site copies while preserving their existing target sources and routing behavior.
🤖 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 `@crates/engine/src/game/engine_priority.rs`:
- Around line 350-372: Update the cancellation path for WaitingFor::ManaPayment
so pending activation triggers staged by stage_pending_activation_trigger_events
are not lost when the cast is discarded. Before dropping the pending cast,
replay only the non-reverted staged events into the normal/shared trigger
collector, preserving consumed occurrences and excluding reverted events.
---
Outside diff comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 2602-2708: Update finish_sacrifice_for_cost so
pending.mark_activation_cost_committed() runs whenever
pending.activation_ability_index.is_some(), regardless of completion. Keep the
SelectedNonSelf condition only around remove_selected_non_self_sacrifice_cost,
matching the unconditional commit behavior in handle_sacrifice_for_cost.
In `@crates/engine/src/game/casting_targets.rs`:
- Around line 360-375: The target-declaration events emitted by
emit_targeting_events must be preserved when maybe_pause_for_cast_distribution
returns DistributeAmong instead of being lost before the post-action pipeline.
Update both affected sites in crates/engine/src/game/casting_targets.rs:360-375
and crates/engine/src/game/casting_targets.rs:467-483 so the events are parked
on the paused action/state and released through the normal post-action path when
distribution resumes; keep immediate emission unchanged when no pause occurs.
In `@crates/engine/src/game/casting.rs`:
- Around line 17250-17617: Mirror the interactive-cost dispatch used in the
no-target branch within pay_activation_costs_after_target_selection, so targeted
activations surface every remaining interactive cost before
finish_activated_ability_at_payment_boundary. Reuse the existing specialized
handlers or surface_next_unpaid_interactive_activation_cost for collect
evidence, craft materials, OneOf, return-to-hand, remove-counter, tap, and
Waterbend, while preserving the existing non-self exile and unattach handling.
In `@crates/engine/src/game/engine_modes.rs`:
- Around line 262-269: Update the direct modal activation branch that constructs
PendingCast before calling finish_activated_ability_at_payment_boundary. Ensure
interactive costs such as Sacrifice, discard, and exile are surfaced and paid
through the existing interactive-cost handling before the modal ability is
pushed, while preserving the current mana and mana-leg finalization flow.
---
Nitpick comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 4457-4700: Extract the repeated target-settlement sequence from
push_activated_ability_to_stack and begin_deferred_target_selection into a
shared helper, such as settle_targets_and_route_to_payment. The helper should
begin the activation trigger collection, emit targeting events for the supplied
targets, and call finish_target_selected_activated_ability_at_payment_boundary;
replace all assigned, random, and auto-select call-site copies while preserving
their existing target sources and routing behavior.
In `@crates/engine/src/game/triggers.rs`:
- Around line 736-742: Optimize rebuild_staged_state and the surrounding collect
flow to avoid replaying the entire operation_journal for every payment slice:
cache the staged GameState between slices and reuse it until the live state
mutates, invalidating and rebuilding the cache at that point. Preserve journal
ordering and existing PrepareEventBatch layer flushing and off-zone
reconciliation behavior.
- Around line 496-517: Consolidate batched zone-change index derivation by
adding or reusing a shared helper owned by the existing
`record_matched_batched_zone_change_replay`/`record_batched_zone_change_collected`
flow. Update `record_matched_batched_zone_change_replay` to use that helper
instead of locally scanning `matched.trigger_events`, and make
`record_batched_zone_change_collected` and
`batched_zone_change_already_collected` consume the same authority so all paths
claim identical `turn_zone_change_index` values.
🪄 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: e422104f-93f4-4afc-a695-fda8351e6144
📒 Files selected for processing (15)
crates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_targets.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_casting.rscrates/engine/src/game/engine_modes.rscrates/engine/src/game/engine_priority.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/triggers.rscrates/engine/src/game/visibility.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/cost_zone_pipeline.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/mogg_fanatic_target_before_cost.rs
There was a problem hiding this comment.
Actionable comments posted: 1
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/game/engine.rs (1)
92-101: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCheck
WaitingForhere too.state.pending_castonly coversManaPayment; the interactive cancel prompts keep thePendingCastinsideWaitingFor, so this guard can miss a committed activation and allow rollback after costs are already paid. Use the shared pending-cast accessor alongsidestate.pending_castbefore allowingCancelCast.🤖 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/engine.rs` around lines 92 - 101, Update ensure_assist_cancellation_is_allowed to also inspect the shared pending-cast accessor for a committed activation stored in WaitingFor, alongside the existing state.pending_cast check. Return the same EngineError::ActionNotAllowed result whenever either source indicates activation_cost_committed, preventing CancelCast after costs are paid.
🤖 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 `@crates/engine/tests/integration/mogg_fanatic_target_before_cost.rs`:
- Around line 95-98: Fix the `WaitingFor::PayCost` assertion by making the
`kind` binding and comparison use matching ownership: either remove `ref` and
clone the matched value, or dereference `kind` in the `assert_eq!` against
`PayCostKind::Sacrifice`. Keep the existing state pattern and sacrifice-prompt
validation unchanged.
---
Outside diff comments:
In `@crates/engine/src/game/engine.rs`:
- Around line 92-101: Update ensure_assist_cancellation_is_allowed to also
inspect the shared pending-cast accessor for a committed activation stored in
WaitingFor, alongside the existing state.pending_cast check. Return the same
EngineError::ActionNotAllowed result whenever either source indicates
activation_cost_committed, preventing CancelCast after costs are paid.
🪄 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: ace85766-3607-49f3-85d5-2f6cadff480f
📒 Files selected for processing (7)
crates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_targets.rscrates/engine/src/game/engine.rscrates/engine/src/game/triggers.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/mogg_fanatic_target_before_cost.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/casting.rs
- crates/engine/src/game/triggers.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/engine/tests/integration/mogg_fanatic_target_before_cost.rs`:
- Line 76: Update the assertion in the test to compare the owned kind value with
the owned PayCostKind::Sacrifice variant, removing the reference from the
expected operand while preserving the existing assertion.
🪄 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: b93a67cf-eb86-4a0e-bf61-4b926b03fb37
📒 Files selected for processing (1)
crates/engine/tests/integration/mogg_fanatic_target_before_cost.rs
matthewevans
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
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/game/casting.rs (1)
16631-16655: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnify the X-choice gate for sacrifice-any-number costs
RequiresChosenXcurrently acceptsfind_non_self_sacrifice_cost(...).count == u32::MAX, but the matching branch inhandle_activate_abilitydoes not. That can surface an activation as legal even though submission still fails withunresolved_x_target_construction_error()when X-dependent targets are involved. Mirror the same predicate in both paths, or factor it into one shared helper.🤖 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.rs` around lines 16631 - 16655, Unify the X-choice detection used by the RequiresChosenX branch with the corresponding logic in handle_activate_ability: ensure sacrifice-any-number costs identified by find_non_self_sacrifice_cost(...).count == u32::MAX are accepted in both paths. Prefer reusing or extracting a shared predicate so legality checks and activation handling cannot diverge.
🧹 Nitpick comments (2)
crates/engine/src/game/ability_utils.rs (1)
2948-2981: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate error message — reuse the new canonical helper.
resolve_multi_target_boundsstill hardcodes"Target count requires a resolved quantity before target selection"inline, even though this PR introducesunresolved_x_target_construction_error()specifically to centralize that exact message (used byTargetSlotBuildError::RequiresChosenX'sFromconversion). Leaving both in place risks the two copies drifting apart later.♻️ Proposed fix
if multi_target_needs_quantity_choice(state, ability, spec) { - return Err(EngineError::ActionNotAllowed( - "Target count requires a resolved quantity before target selection".to_string(), - )); + return Err(unresolved_x_target_construction_error()); }🤖 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/ability_utils.rs` around lines 2948 - 2981, Update resolve_multi_target_bounds to use the canonical unresolved_x_target_construction_error() helper when returning the RequiresChosenX/action-not-allowed error, removing the duplicated inline message while preserving the existing control flow.crates/engine/src/game/casting.rs (1)
17127-17150: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTarget slots are computed twice on the common (non-X) activation path.
build_target_slots_for_announcementis called here to derivehas_effect_targets. When the outcome isSlots(target_slots)and none of the X/loyalty/no-target detours below fire (the common case for most activated abilities, which have neither{X}costs nor sacrifice-any-number costs), execution falls through to Line 17676'sbuild_target_slots(state, &resolved)?, which re-walks the same ability chain against the samestateto recompute an identicalVec<TargetSelectionSlot>. Consider threading the already-computedtarget_slotsfrom theSlots(...)arm through to Line 17676 instead of discarding and rebuilding it.🤖 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.rs` around lines 17127 - 17150, Thread the successfully computed target slots from the `build_target_slots_for_announcement` match through the activation flow instead of retaining only `has_effect_targets`. Reuse that `Vec<TargetSelectionSlot>` at the later `build_target_slots(state, &resolved)?` site on the common non-X path, while preserving the existing `RequiresChosenX` handling and any detours that must rebuild or modify targets.
🤖 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 `@crates/engine/src/game/engine_tests.rs`:
- Around line 137-140: Replace the direct ensure_assist_cancellation_is_allowed
assertion in the cancellation test with an assist-cancel GameAction dispatched
through apply from the pending-target state after committing the activation
cost. Assert that apply returns EngineError::ActionNotAllowed with the existing
message, and verify the committed paid cost remains unchanged.
- Around line 115-141: Add a verified CR citation annotation to the test
assist_cancellation_rejects_committed_activation_held_by_waiting_for,
documenting the rule that an activated ability cannot be canceled after its
costs have been paid. Use the repository’s established citation format and place
it with the test’s existing documentation or annotations.
---
Outside diff comments:
In `@crates/engine/src/game/casting.rs`:
- Around line 16631-16655: Unify the X-choice detection used by the
RequiresChosenX branch with the corresponding logic in handle_activate_ability:
ensure sacrifice-any-number costs identified by
find_non_self_sacrifice_cost(...).count == u32::MAX are accepted in both paths.
Prefer reusing or extracting a shared predicate so legality checks and
activation handling cannot diverge.
---
Nitpick comments:
In `@crates/engine/src/game/ability_utils.rs`:
- Around line 2948-2981: Update resolve_multi_target_bounds to use the canonical
unresolved_x_target_construction_error() helper when returning the
RequiresChosenX/action-not-allowed error, removing the duplicated inline message
while preserving the existing control flow.
In `@crates/engine/src/game/casting.rs`:
- Around line 17127-17150: Thread the successfully computed target slots from
the `build_target_slots_for_announcement` match through the activation flow
instead of retaining only `has_effect_targets`. Reuse that
`Vec<TargetSelectionSlot>` at the later `build_target_slots(state, &resolved)?`
site on the common non-X path, while preserving the existing `RequiresChosenX`
handling and any detours that must rebuild or modify targets.
🪄 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: 6d00e0a6-23dd-48a6-92b3-c6fb63c83941
📒 Files selected for processing (8)
crates/engine/src/game/ability_utils.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_tests.rscrates/engine/tests/integration/greater_good_activation.rscrates/engine/tests/integration/mogg_fanatic_target_before_cost.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/tests/integration/mogg_fanatic_target_before_cost.rs
- crates/engine/src/game/engine.rs
- crates/engine/src/game/casting_costs.rs
| #[test] | ||
| fn assist_cancellation_rejects_committed_activation_held_by_waiting_for() { | ||
| let mut state = setup_game_at_main_phase(); | ||
| let mut pending = PendingCast::new( | ||
| ObjectId(1), | ||
| CardId(1), | ||
| ResolvedAbility::new(Effect::NoOp, vec![], ObjectId(1), PlayerId(0)), | ||
| ManaCost::NoCost, | ||
| ); | ||
| pending.activation_cost_committed = true; | ||
| state.waiting_for = WaitingFor::TargetSelection { | ||
| player: PlayerId(0), | ||
| pending_cast: Box::new(pending), | ||
| target_slots: vec![], | ||
| mode_labels: vec![], | ||
| selection: TargetSelectionProgress { | ||
| current_slot: 0, | ||
| selected_slots: vec![], | ||
| current_legal_targets: vec![], | ||
| }, | ||
| }; | ||
|
|
||
| assert!(matches!( | ||
| ensure_assist_cancellation_is_allowed(&state), | ||
| Err(EngineError::ActionNotAllowed(message)) if message == "Cannot cancel an activation after a cost is paid" | ||
| )); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Document the cancellation rule with a verified CR citation.
This new rules-ordering test has no CR annotation. Add a verified citation describing why an activated ability cannot be canceled after its costs have been paid.
🤖 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/engine_tests.rs` around lines 115 - 141, Add a
verified CR citation annotation to the test
assist_cancellation_rejects_committed_activation_held_by_waiting_for,
documenting the rule that an activated ability cannot be canceled after its
costs have been paid. Use the repository’s established citation format and place
it with the test’s existing documentation or annotations.
Sources: Coding guidelines, Path instructions
| assert!(matches!( | ||
| ensure_assist_cancellation_is_allowed(&state), | ||
| Err(EngineError::ActionNotAllowed(message)) if message == "Cannot cancel an activation after a cost is paid" | ||
| )); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Exercise cancellation through the production action path.
Calling ensure_assist_cancellation_is_allowed directly does not prove the live cancellation dispatcher consults this state. Drive an actual assist-cancel GameAction through apply from the pending-target state after a committed activation cost, then assert the rejection and that the paid cost remains intact.
🤖 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/engine_tests.rs` around lines 137 - 140, Replace the
direct ensure_assist_cancellation_is_allowed assertion in the cancellation test
with an assist-cancel GameAction dispatched through apply from the
pending-target state after committing the activation cost. Assert that apply
returns EngineError::ActionNotAllowed with the existing message, and verify the
committed paid cost remains unchanged.
Source: Path instructions
497d5ae to
8981408
Compare
1 participant
Summary by CodeRabbit