fix(engine): track committed crimes - #7093
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe engine adds turn-scoped crime tracking and defers crime commitment until stack placement. It also exposes static ChangesCant-be-blocked derived view
Crime tracking
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant casting_targets
participant casting_costs
participant GameState
participant Ledger
Player->>casting_targets: Declare targets
casting_targets->>GameState: Store crime_candidate
casting_targets->>Player: Emit BecomesTarget events
casting_targets->>casting_costs: Continue payment flow
casting_costs->>GameState: Place action on stack
casting_costs->>Ledger: Record CrimeCommitted
Ledger->>GameState: Set crimes_committed_this_turn
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/engine/src/game/casting_tests.rsast-grep timed out on this file crates/engine/src/game/triggers.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthewevans
enabled auto-merge
August 8, 2026 03:43
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/engine/src/game/casting_targets.rs (1)
220-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmit target events for deferred spell target selection.
These direct paths emit
BecomesTargetevents. The equivalent random and automatic paths incrates/engine/src/game/casting_costs.rs:1528-1727emit them only for activated abilities. A spell that defers target selection, such as an X-dependent spell, therefore misses its target-declaration events.Emit the events for every deferred announced target set before
finish_pending_cost_or_cast. Keep the activation trigger collection behavior separate. Add a spell regression test for this deferred path.As per path instructions, engine behavior must remain CR-faithful; the supplied
crates/engine/src/game/casting_costs.rs:1528-1727context shows the unhandled deferred spell path.🤖 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 220 - 246, Update the deferred target-selection flow around auto_select_targets_for_ability and the corresponding random-target path so BecomesTarget events are emitted for every deferred announced target set, including spells, before finish_pending_cast_cost_or_pay. Keep activation-trigger collection restricted to activated abilities, and add a regression test covering a deferred spell target-selection path such as an X-dependent spell.Source: Path instructions
crates/engine/src/game/casting_costs.rs (1)
5232-5246: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix:
crime_candidateis dropped when activation-cost payment pauses.Both branches build a new
PendingCastwithPendingCast::new(...)and copy several continuation fields (activation_cost,activation_ability_index,pending_loyalty_activation_player,activation_target_selection,activation_trigger_collection) onto it. Neither branch copiescrime_candidate, the function's own parameter that already holds the correct value computed from the ability's assigned targets.
PendingCast::newdefaultscrime_candidatetofalse. When resumed,finish_activated_ability_at_payment_boundary(Line 4357) readspending.crime_candidatefrom this reconstructed object and forwards it topush_activated_ability_to_stack, which commits it at Line 5475. An activation whose targets did commit a crime, but whose cost includes an interactive sub-cost (for example an ambiguous sacrifice or discard) that pauses payment, will never have that crime committed once payment resumes.🐛 Proposed fix: carry `crime_candidate` through both pause continuations
let mut pending_interactive = PendingCast::new(source_id, CardId(0), resolved.clone(), ManaCost::NoCost); pending_interactive.activation_cost = Some(cost.clone()); pending_interactive.activation_ability_index = Some(ability_index); pending_interactive.pending_loyalty_activation_player = pending_loyalty_activation_player; pending_interactive.activation_target_selection = target_selection; pending_interactive.activation_trigger_collection = activation_trigger_collection.clone(); + pending_interactive.crime_candidate = crime_candidate; if let Some(waiting_for) = surface_next_unpaid_interactive_activation_cost(let mut pending = PendingCast::new(source_id, CardId(0), resolved.clone(), ManaCost::NoCost); pending.activation_cost = remaining_cost; pending.activation_ability_index = Some(ability_index); pending.pending_loyalty_activation_player = should_record_loyalty .then_some(player) .or(pending_loyalty_activation_player); pending.activation_target_selection = target_selection; pending.activation_trigger_collection = activation_trigger_collection.clone(); + pending.crime_candidate = crime_candidate; if let Some(pending) = attach_pending_cast_to_cost_move(state, Box::new(pending)) {Also applies to: 5301-5324
🤖 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 5232 - 5246, Copy the function’s crime_candidate value onto both reconstructed PendingCast instances before invoking the interactive-cost payment flow. Update the continuation blocks around PendingCast::new and surface_next_unpaid_interactive_activation_cost so finish_activated_ability_at_payment_boundary receives the original candidate and preserves crime commitment after payment resumes.crates/engine/src/game/engine_stack.rs (1)
27-89: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winCommit the crime before draining deferred triggers after distribution.
The
DistributeAmongcompletion branch forstate.pending_triggerfinalizes the pending trigger and returns priority after sibling drains, but it never callscommit_crime_after_stack_placement. The pending trigger is already committed whenfinalize_pending_trigger_entryis called, so invoke the commit after that succeeds instead of leaving this path missing the stack-placement commit.🤖 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_stack.rs` around lines 27 - 89, Update the DistributeAmong completion path for state.pending_trigger to call commit_crime_after_stack_placement immediately after finalize_pending_trigger_entry succeeds, before draining deferred sibling triggers and returning priority. Reuse the existing pending-trigger controller/ability context and preserve the current failure handling.
🤖 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/board/__tests__/PermanentCard.test.tsx`:
- Line 264: Add a test fixture in the PermanentCard test cases where
derived.cant_be_blocked contains the object ID while temporary_cant_be_blocked
has no matching entry, then assert that CantBeBlockedBadge renders for that
state. Keep the existing temporary-attribution fixtures unchanged.
In `@crates/engine/src/game/casting_tests.rs`:
- Around line 22391-22401: Update the declaration and chained-payment tests
around targets_commit_crime, pay_and_push, and the related cases to capture the
crime ledger counter before declaration, then assert declaration leaves it
unchanged. After pay_and_push, count GameEvent::CrimeCommitted events and assert
exactly one event and exactly one corresponding ledger increment, including the
additional test locations identified by the comment.
- Around line 22367-22370: Add an end-to-end regression test in the casting
tests that uses handle_cast_spell to initiate casting, performs target
selection, pauses for interactive payment through the production
WaitingFor/PendingCast flow, then resumes and places the spell on the stack.
Assert the crime ledger has no commitment before stack placement and exactly one
commitment afterward; cover the failure path through this same pipeline rather
than calling emit_targeting_events, targets_commit_crime, pay_and_push, or
constructing ResolvedAbility directly.
In `@crates/engine/src/game/casting.rs`:
- Around line 658-666: Update the Zone match in the is_some_and classifier to
remove the wildcard arm and explicitly list every currently nonqualifying Zone
variant with false, while preserving the Battlefield, Stack, and Graveyard
opponent checks. This makes future Zone additions require an explicit
classification decision.
In `@crates/engine/src/game/combat.rs`:
- Around line 1335-1339: Introduce has_cant_be_blocked_static_from_precomputed
alongside has_cant_be_blocked_static, accepting the already collected block
restrictions and reusing block_restriction_statics_against_from_precomputed
without recollecting them. Update derive_views to collect restrictions once per
calculation and call the new helper for each battlefield object, preserving the
existing CantBeBlocked check.
In `@crates/engine/src/game/derived_views.rs`:
- Around line 913-915: Restrict the `views.cant_be_blocked` insertion in the
battlefield-object loop to objects whose type includes `CoreType::Creature`,
while preserving the existing `has_cant_be_blocked_static` check. Add a
regression case covering an affected permanent that loses its creature type and
verify it is excluded from `cant_be_blocked`.
In `@crates/engine/src/game/ledger.rs`:
- Around line 326-345: Update the ResolvedLedgerEdit::CrimeCommitted handling to
require expected_turn_count == 0 before applying the edit, rejecting any
non-zero precondition with the existing invariant error path. Preserve the
player lookup and successful transition to a count of 1, and add a replay test
covering a second CrimeCommitted command.
In `@crates/engine/src/types/game_state.rs`:
- Around line 5607-5611: Update the documentation for the crime_candidate field
to accurately cover activated abilities by adding the CR 602.2b bridge alongside
CR 601.2c, and cite CR 603.3d if triggered abilities also use this field. Ensure
every cited rule has a verified “CR <number>: <description>” annotation
describing the field’s behavior, or narrow the documentation to spell casts if
activations are not supported.
- Around line 5607-5611: Preserve an explicit “unclassified” state for
PendingCast.crime_candidate instead of treating the serde default and
constructor value false as “not a crime.” Update the field and all PendingCast
construction, restoration, and ledger-commit paths so classification is
represented with Option<bool> or an equivalent status, and only commit the
ledger after a definitive classification; add a guarded restoration test if
needed to verify unclassified state does not commit.
In `@crates/engine/src/types/resolved_commands.rs`:
- Around line 3197-3200: Update the CrimeCommitted validation arm in the
resolved ledger validator to accept only expected_turn_count == 0, rejecting
every nonzero value. Apply the same check in apply_resolved_ledger_edit so
direct callers cannot bypass validation, and add a replay test covering
expected_turn_count = 1.
In `@crates/engine/tests/integration/crime_tracking.rs`:
- Around line 9-21: The test crime_ledger_edit_is_turn_scoped currently bypasses
the production flow by calling record_crime_committed directly. Replace those
direct calls with GameAction scenarios that cast or activate a targeting action,
asserting no crime is committed before successful stack placement and exactly
one afterward; include a failed payment or canceled action path as applicable.
Preserve the existing start_next_turn reset assertion.
---
Outside diff comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 5232-5246: Copy the function’s crime_candidate value onto both
reconstructed PendingCast instances before invoking the interactive-cost payment
flow. Update the continuation blocks around PendingCast::new and
surface_next_unpaid_interactive_activation_cost so
finish_activated_ability_at_payment_boundary receives the original candidate and
preserves crime commitment after payment resumes.
In `@crates/engine/src/game/casting_targets.rs`:
- Around line 220-246: Update the deferred target-selection flow around
auto_select_targets_for_ability and the corresponding random-target path so
BecomesTarget events are emitted for every deferred announced target set,
including spells, before finish_pending_cast_cost_or_pay. Keep
activation-trigger collection restricted to activated abilities, and add a
regression test covering a deferred spell target-selection path such as an
X-dependent spell.
In `@crates/engine/src/game/engine_stack.rs`:
- Around line 27-89: Update the DistributeAmong completion path for
state.pending_trigger to call commit_crime_after_stack_placement immediately
after finalize_pending_trigger_entry succeeds, before draining deferred sibling
triggers and returning priority. Reuse the existing pending-trigger
controller/ability context and preserve the current failure handling.
🪄 Autofix
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: 058a696f-c25a-412a-b529-22e602bec0e4
📒 Files selected for processing (25)
client/src/adapter/types.tsclient/src/components/board/PermanentCard.tsxclient/src/components/board/__tests__/PermanentCard.test.tsxcrates/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/combat.rscrates/engine/src/game/derived_views.rscrates/engine/src/game/engine_stack.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/ledger.rscrates/engine/src/game/planeswalker.rscrates/engine/src/game/quantity.rscrates/engine/src/game/triggers.rscrates/engine/src/game/turns.rscrates/engine/src/game/visibility.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rscrates/engine/src/types/player.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/crime_tracking.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/temporary_cant_be_blocked_view.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit