fix(engine): harden crime ledger and unblockable views - #7098
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesCrime tracking and blocking-status projections
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AbilityActivation
participant StackPlacement
participant CrimeLedger
AbilityActivation->>AbilityActivation: evaluate flattened announced targets
AbilityActivation->>StackPlacement: place ability
StackPlacement->>CrimeLedger: record CrimeCommitted after placement
CrimeLedger-->>AbilityActivation: publish crime commitment
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/src/game/engine_stack.rs (1)
27-89: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist and commit the trigger crime after distribution selection completes.
finalize_trigger_target_selectioncomputescrime_candidatebefore storingtriggerback intostate.pending_trigger, but theDistributeAmongprompt path drops that value.PendingTriggerhas no savedcrime_candidatefield, so when a triggered ability divides damage or counters among multiple targets, the crime decision can be lost instead of being committed once the targets are announced on the stack. Thread the crime flag throughPendingTriggeror store/retrieve it from state before returningWaitingFor::DistributeAmong, then callcommit_crime_after_stack_placementfrom the trigger distribution resolution 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/engine_stack.rs` around lines 27 - 89, Preserve the crime_candidate computed in finalize_trigger_target_selection when returning WaitingFor::DistributeAmong by threading it through PendingTrigger or the pending state. In the trigger distribution-resolution path, retrieve that value and call commit_crime_after_stack_placement after the ability is placed on the stack, ensuring the crime is committed exactly once.crates/engine/src/game/casting_costs.rs (1)
1633-1696: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmit targeting events before returning from the deferred spell-cast target paths.
In
crates/engine/src/game/casting_costs.rs, both the Random and auto-select branches assign targets and setpending.crime_candidate, but they callsuper::casting::emit_targeting_events(...)only whenpending.activation_ability_index.is_some(). The spell path then returns throughfinish_pending_cost_or_cast, which does not emitBecomesTargetfor these targets.Call
super::casting::emit_targeting_eventsimmediately after assigning targets in both branches, before the activation/spell branch, so spells that reach deferred selection still fire target-related events.🤖 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 1633 - 1696, Update both the Random-target branch and the auto-select branch in the deferred target-selection flow to call super::casting::emit_targeting_events immediately after assigning targets and updating pending.ability, before checking pending.activation_ability_index. Remove the activation-only placement so both deferred spells and activated abilities emit targeting events exactly once before their respective completion paths.
🧹 Nitpick comments (1)
crates/engine/src/game/derived_views.rs (1)
29-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the creature-only restriction.
The PR restricts
cant_be_blockedtoCoreType::Creature(Line 915). This matches a listed PR objective ("Restricting the derived view to creatures"), which implies a prior review found this projection could leak onto non-creature permanents. The provided integration test (temporary_cant_be_blocked_view.rs) only exercises two creature recipients; it does not cover the negative case of a non-creature permanent carrying a bareCantBeBlockedstatic.Add a test that installs a bare
CantBeBlockedstatic on a non-creature battlefield permanent and asserts it does not appear inviews.cant_be_blocked. This guards the specific restriction this PR claims to add.Also applies to: 419-423, 895-923, 945-945
🤖 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/derived_views.rs` at line 29, Add a regression test alongside the existing temporary cant-be-blocked view tests that places a bare CantBeBlocked static on a non-creature battlefield permanent, then assert that permanent is absent from views.cant_be_blocked. Keep the existing creature coverage unchanged and target the creature-only projection logic around cant_be_blocked.
🤖 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/types/game_state.rs`:
- Around line 5607-5611: Replace the boolean crime_candidate state in the
relevant action/state definitions and all references at the noted locations with
an explicit Unknown, NotCrime, and Crime representation, using
Option<CrimeCandidate> or an equivalent enum. Define serde deserialization for
legacy boolean data so existing false values map consistently to the intended
state, and update classification and stack-placement logic to preserve Unknown
until classification completes. Add a production-pipeline regression covering a
pending action after target declaration through both successful and failed stack
placement.
In `@crates/engine/tests/integration/crime_tracking.rs`:
- Around line 16-24: Extend the test around record_crime_committed and
start_next_turn to record crimes for both players, while preserving the
repeated-crime assertion for PlayerId(0). After advancing the turn, assert that
each player’s crimes_committed_this_turn is zero, covering both per-player
one-crime-per-turn and reset invariants.
- Around line 1-2: Correct the module annotation for CR 700.13 so it only
describes when a player commits a crime. Remove the unsupported claims about
ledger durability and turn-boundary resets, and separately document those engine
invariants with the applicable CR citation whose rule body governs the reset
behavior.
- Around line 30-40: Update the test around record_crime_committed and
resolve_and_apply_ledger_edit to construct and apply one CrimeCommitted edit
successfully, then replay that exact same edit and assert
CrimeCommittedPreconditionMismatch while keeping the expected turn count
unchanged. Ensure the initial application passes the reach guard before
asserting the replay failure.
- Around line 16-17: Add a scenario-runner integration test in the
crime-tracking tests that performs a real crime action through casting,
activation, and trigger/stack placement rather than calling
record_crime_committed or resolve_and_apply_ledger_edit directly. Assert the
ledger has no commitment before successful placement, exactly one after the
successful action, and remains unchanged when the action fails; preserve
existing direct unit coverage while ensuring the production pipeline and failure
path are exercised.
---
Outside diff comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 1633-1696: Update both the Random-target branch and the
auto-select branch in the deferred target-selection flow to call
super::casting::emit_targeting_events immediately after assigning targets and
updating pending.ability, before checking pending.activation_ability_index.
Remove the activation-only placement so both deferred spells and activated
abilities emit targeting events exactly once before their respective completion
paths.
In `@crates/engine/src/game/engine_stack.rs`:
- Around line 27-89: Preserve the crime_candidate computed in
finalize_trigger_target_selection when returning WaitingFor::DistributeAmong by
threading it through PendingTrigger or the pending state. In the trigger
distribution-resolution path, retrieve that value and call
commit_crime_after_stack_placement after the ability is placed on the stack,
ensuring the crime is committed exactly once.
---
Nitpick comments:
In `@crates/engine/src/game/derived_views.rs`:
- Line 29: Add a regression test alongside the existing temporary
cant-be-blocked view tests that places a bare CantBeBlocked static on a
non-creature battlefield permanent, then assert that permanent is absent from
views.cant_be_blocked. Keep the existing creature coverage unchanged and target
the creature-only projection logic around cant_be_blocked.
🪄 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: cbd12db9-aef2-4c8f-b8d8-2e8f470c7323
📒 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
1c9c18b to
1858865
Compare
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
1 participant
Follow-up to #7093 addressing post-merge review findings: enforce the one-per-turn crime ledger invariant, make crime zone classification exhaustive, precompute unblockable static restrictions for derived views, restrict that view to creatures, and cover the static-only frontend badge path.
Summary by CodeRabbit
New Features
Bug Fixes