fix(engine): qualify zone-change occurrences by turn - #7075
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 (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughZone-change records now store turn identity with their per-turn index. Trigger deduplication uses both values. Replay validates turn identity. Persistence loading migrates legacy keys and reconciles live zone-change occurrences across resolution formats. ChangesZone-change provenance
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PersistenceLoader
participant ResolutionStateWire
participant GameState
participant TriggerState
PersistenceLoader->>ResolutionStateWire: decode persisted resolution
ResolutionStateWire->>GameState: reconcile zone-change occurrences
GameState->>TriggerState: restore turn/index trigger keys
TriggerState-->>GameState: validate replay references
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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/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 |
|
Generated for head 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: 2
🧹 Nitpick comments (3)
crates/engine/src/types/game_state.rs (2)
7819-7877: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the guard chain with an
Orderingmatch to remove theunreachable!panic from the persistence path.The four
Some(...)guards partitionrecorded_turnagainstcurrent_turn, so the trailingSome(_) => unreachable!(...)arm is dead today. It is still a panic inside a deserialization boundary. If a future edit narrows one guard, restore fails with a process panic instead of anErr. Matching onrecorded_turn.cmp(¤t_turn)makes the partition compiler-checked and drops the arm.♻️ Proposed restructure
+ use std::cmp::Ordering; let (recorded_turn, index) = match recorded_turn { Some(0) if current_turn != 0 => reconcile_current_turn_zone_changed_record( record, current_turn, &fingerprint, occurrences, )?, - Some(turn) if turn < current_turn => { + Some(turn) => match turn.cmp(¤t_turn) { + Ordering::Less => { let index = index.ok_or_else(|| { "prior-turn ZoneChanged record is missing its occurrence index".to_string() })?; (turn, index) - } - Some(turn) if turn > current_turn => { + } + Ordering::Greater => { return Err("ZoneChanged record is stamped from a future turn".to_string()); - } - Some(turn) if turn == current_turn => { + } + Ordering::Equal => { if let Some(index) = index { ... } - } + } + }, ... - Some(_) => unreachable!("future turns return above"), };🤖 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/game_state.rs` around lines 7819 - 7877, Refactor the recorded_turn handling to match on recorded_turn.cmp(¤t_turn) using Ordering variants, while preserving the existing behavior for missing, prior, current, and future turns. Remove the guard-based Some(...) partition and the trailing unreachable!("future turns return above") arm, ensuring future-turn cases return the same Err without any panic path.Source: Coding guidelines
7935-7957: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe live-carrier allow-list is stringly typed and has no drift guard.
LIVE_EVENT_CARRIER_FIELDSlistsGameStatefield names as string literals. If a later change adds a live field that carries aZoneChangedevent, reconciliation silently skips it and that event restores with a stale or defaulted occurrence key. Nothing fails at compile time.The paired test
persisted_zone_change_traverses_direct_queue_and_stack_carriersassertskeys.len() >= 9. That aggregate count does not prove each listed carrier was visited: one carrier holding several events satisfies it while another carrier contributes nothing. Assert per carrier instead, so a dropped or misspelled field name fails the test.Consider deriving the list from a single typed source, or add a test that serializes a state with exactly one event per carrier and asserts the visited count equals
LIVE_EVENT_CARRIER_FIELDS.len().🤖 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/game_state.rs` around lines 7935 - 7957, The LIVE_EVENT_CARRIER_FIELDS allow-list and its reconciliation test lack protection against missing or misspelled carriers. Update persisted_zone_change_traverses_direct_queue_and_stack_carriers to construct a state with exactly one ZoneChanged event in every carrier named by LIVE_EVENT_CARRIER_FIELDS and assert each carrier is visited, with the total visited count equal to LIVE_EVENT_CARRIER_FIELDS.len(); if feasible, replace the string list with a typed single source of truth.Source: Path instructions
crates/engine/src/game/effects/token.rs (1)
1165-1170: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider validating
command.entry_turnagainststate.turn_number, matching the zone-change replay guard.
apply_resolved_zone_changenow rejects a replay whosezone_change_record.recorded_turn_numberdiffers fromstate.turn_number(RecordedTurnMismatch). This token-birth applier records an equivalent ledger row through the same authority, but performs no turn check.record_zone_changestampsstate.turn_numberunconditionally, so a replay that runs against a state at a different turn writes a ledger row with a turn the command never recorded, and no error is raised. The command already carriesentry_turn, so the comparison is available.🛡️ Proposed guard
+ if command.entry_turn != state.turn_number { + return Err(ResolvedTokenCreationReplayInvariantError::EntryTurnMismatch { + expected: command.entry_turn, + found: state.turn_number, + }); + } let mut entry_record = state .objects .get(&object_id) .expect("the token was materialized above") .snapshot_for_zone_change(object_id, None, Zone::Battlefield); crate::game::restrictions::record_zone_change(state, &mut entry_record);🤖 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/effects/token.rs` around lines 1165 - 1170, Validate the token-birth command’s entry turn against state.turn_number before creating the ledger record in the token applier containing entry_record and record_zone_change. Reject mismatches using the same RecordedTurnMismatch behavior as apply_resolved_zone_change, and only call record_zone_change after validation succeeds.
🤖 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 8010-8026: Update validate_restored_zone_change_replay_keys to
handle valid prior-turn batched replay keys: either ensure
batched_zone_change_trigger_fired is cleared when the turn boundary discards
zone_changes_this_turn, or skip validated legacy entries whose recorded turn
predates state.turn_number. Preserve strict ledger validation for current-turn
keys.
- Around line 7734-7766: Ensure legacy batched trigger migration runs before
every ResolutionStateWire deserialization, including trusted envelopes, raw
saves, and GameStateDecode::decode_persisted_resolution_state. Prefer adding it
at the shared persistence chokepoint, or invoke
migrate_legacy_batched_zone_change_trigger_fired at the start of
reconcile_persisted_zone_change_occurrences before
reindex_persisted_batched_zone_change_trigger_keys validates three-field tuples,
so historical two-field entries gain their recorded turn.
---
Nitpick comments:
In `@crates/engine/src/game/effects/token.rs`:
- Around line 1165-1170: Validate the token-birth command’s entry turn against
state.turn_number before creating the ledger record in the token applier
containing entry_record and record_zone_change. Reject mismatches using the same
RecordedTurnMismatch behavior as apply_resolved_zone_change, and only call
record_zone_change after validation succeeds.
In `@crates/engine/src/types/game_state.rs`:
- Around line 7819-7877: Refactor the recorded_turn handling to match on
recorded_turn.cmp(¤t_turn) using Ordering variants, while preserving the
existing behavior for missing, prior, current, and future turns. Remove the
guard-based Some(...) partition and the trailing unreachable!("future turns
return above") arm, ensuring future-turn cases return the same Err without any
panic path.
- Around line 7935-7957: The LIVE_EVENT_CARRIER_FIELDS allow-list and its
reconciliation test lack protection against missing or misspelled carriers.
Update persisted_zone_change_traverses_direct_queue_and_stack_carriers to
construct a state with exactly one ZoneChanged event in every carrier named by
LIVE_EVENT_CARRIER_FIELDS and assert each carrier is visited, with the total
visited count equal to LIVE_EVENT_CARRIER_FIELDS.len(); if feasible, replace the
string list with a typed single source of truth.
🪄 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: a27d1d15-53d8-4a4c-bdcc-1a658ced3374
📒 Files selected for processing (19)
crates/engine/src/game/derived_views.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/filter.rscrates/engine/src/game/game_object.rscrates/engine/src/game/meld.rscrates/engine/src/game/merge.rscrates/engine/src/game/restrictions.rscrates/engine/src/game/stack.rscrates/engine/src/game/triggers.rscrates/engine/src/game/triggers_dedup_regression_tests.rscrates/engine/src/game/zones.rscrates/engine/src/types/game_state.rscrates/engine/src/types/resolution.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_zone_change.rscrates/engine/tests/integration/issue_3277_captain_nghathrod_eliminated_opponent.rscrates/engine/tests/integration/issue_5332_gandalf_trigger_doubling.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/madame_null_integration.rs
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
2e258e2 to
860b58b
Compare
1 participant
Fixes #7065.
Zone-change occurrence identity now includes its recording turn, preventing a deferred trigger from aliasing the reset per-turn index after the next-turn transition. The change preserves legacy serialized batched trigger keys, updates replay validation, and adds cross-turn regression coverage.
Summary by CodeRabbit
Bug Fixes
Tests