cr733(p2): journal resolved library shuffles + RNG entropy receipt#6522
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughLibrary shuffles now resolve into validated journal receipts containing card orders and RNG positions. The engine applies those receipts deterministically, enforces RNG and library preconditions, records shuffle commands, and supports them in semantic replay. ChangesLibrary shuffle replay
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ShuffleEffect
participant LibraryModule
participant GameState
participant ResolvedRulesJournal
ShuffleEffect->>LibraryModule: resolve_and_apply_library_shuffle
LibraryModule->>GameState: capture and advance RNG word positions
LibraryModule->>LibraryModule: apply_resolved_library_shuffle
LibraryModule->>ResolvedRulesJournal: record_library_shuffle
LibraryModule-->>ShuffleEffect: emit ShuffledLibrary action
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/game/library.rs (1)
73-80: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid materializing the whole library into a
Vecjust to compare it.
current_orderclones the entireim::Vectorinto a freshVecsolely to run!=againstcommand.precondition_order. An iterator-based.eq()comparison avoids the allocation/copy and keeps the im::Vector zone comparison idiomatic.♻️ Proposed refactor
- let current_order: Vec<_> = state.players[player_index] - .library - .iter() - .copied() - .collect(); - if current_order != command.precondition_order { + if !state.players[player_index] + .library + .iter() + .eq(command.precondition_order.iter()) + { return Err(ResolvedLibraryShuffleReplayInvariantError::LibraryOrderPreconditionMismatch); }As per path instructions, "Hot
im::Vectorzones must useimmethods (push_back/pop_back/iter_mut), not materialize a stdVec."🤖 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/library.rs` around lines 73 - 80, Replace the `current_order` materialization in the library shuffle replay validation with an iterator-based equality comparison between `state.players[player_index].library` and `command.precondition_order`, preserving the existing mismatch error and control flow. Avoid collecting the `im::Vector` into a `Vec`.Source: Path instructions
🤖 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.
Nitpick comments:
In `@crates/engine/src/game/library.rs`:
- Around line 73-80: Replace the `current_order` materialization in the library
shuffle replay validation with an iterator-based equality comparison between
`state.players[player_index].library` and `command.precondition_order`,
preserving the existing mismatch error and control flow. Avoid collecting the
`im::Vector` into a `Vec`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: feca0cf5-e15b-4933-98da-e9a31bd4907d
📒 Files selected for processing (8)
crates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/shuffle.rscrates/engine/src/game/library.rscrates/engine/src/game/mod.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_commands_p2.rs
f911b32 to
1c27eae
Compare
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/src/types/resolved_commands.rs`:
- Around line 1509-1532: Update validate_library_shuffle_receipt to reject
pre_word_pos == post_word_pos when the validated library order contains more
than one card, while preserving zero-length spans for zero- or one-card
libraries and existing regression checks. Add a malformed-receipt test covering
a multi-card permutation with an unchanged entropy span.
🪄 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: ccb03fc3-0df7-4a52-bd6b-757ca68922f5
📒 Files selected for processing (8)
crates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/shuffle.rscrates/engine/src/game/library.rscrates/engine/src/game/mod.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_commands_p2.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/engine/src/types/mod.rs
- crates/engine/src/game/mod.rs
- crates/engine/src/types/game_state.rs
- crates/engine/tests/integration/cr733_resolved_commands_p2.rs
- crates/engine/src/game/effects/change_zone.rs
- crates/engine/src/game/library.rs
- crates/engine/src/game/effects/shuffle.rs
1c27eae to
8e80b4e
Compare
Parse changes introduced by this PR✓ No card-parse changes detected. |
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/src/game/effects/shuffle.rs`:
- Around line 69-75: Move the player-existence validation in the shuffle effect
to immediately after resolving target_player, before evaluating CantShuffle
suppression; ensure unknown players always return EffectError::PlayerNotFound
even when suppressed. Update the regression coverage for the broad CantShuffle
static to assert this behavior.
🪄 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: f61b09b1-da32-4244-83f3-5c954ef50529
📒 Files selected for processing (8)
crates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/shuffle.rscrates/engine/src/game/library.rscrates/engine/src/game/mod.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_commands_p2.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/engine/src/types/mod.rs
- crates/engine/tests/integration/cr733_resolved_commands_p2.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/mod.rs
- crates/engine/src/game/effects/change_zone.rs
- crates/engine/src/game/library.rs
- crates/engine/src/types/resolved_commands.rs
Adds the first Tier-A leaf command family in the re-sequenced P2 phase
(leaves-first, hub-last): a resolved library shuffle command with an exact
predecessor/resulting order fingerprint and a ChaCha20 entropy receipt
(pre/post word position). Replay installs the recorded order and advances the
RNG via set_word_pos — it never calls shuffle_vector, so retained-prefix replay
cannot consume entropy or pick a different permutation (CR 701.24a).
Introduces the reusable GameState::advance_rng_high_water primitive (monotonic
on both the persisted high-water and the live stream cursor); capture_rng_word_pos
now routes through it as the single RNG-position write path. Both
change_zone::shuffle_library and the library branch of Effect::Shuffle delegate
to game::library::{resolve_and_apply_library_shuffle, apply_resolved_library_shuffle}.
Writes only already-classified census fields (rng/rng_word_pos/players), so the
always-on P0 field-set ratchet holds against the current fixtures; the opt-in
CR733_CENSUS_STRICT line-level pins fold in at the next full fixture regen.
Authored via codex Run 9; ported code-only onto current main (campaign branch
had drifted; uncompressed .json census fixtures superseded by main's gzipped set).
8e80b4e to
226eeac
Compare
1 participant
Adds the first Tier-A leaf command family in the re-sequenced P2 phase
(leaves-first, hub-last): a resolved library shuffle command with an exact
predecessor/resulting order fingerprint and a ChaCha20 entropy receipt
(pre/post word position). Replay installs the recorded order and advances the
RNG via set_word_pos — it never calls shuffle_vector, so retained-prefix replay
cannot consume entropy or pick a different permutation (CR 701.24a).
Introduces the reusable GameState::advance_rng_high_water primitive (monotonic
on both the persisted high-water and the live stream cursor); capture_rng_word_pos
now routes through it as the single RNG-position write path. Both
change_zone::shuffle_library and the library branch of Effect::Shuffle delegate
to game::library::{resolve_and_apply_library_shuffle, apply_resolved_library_shuffle}.
Writes only already-classified census fields (rng/rng_word_pos/players), so the
always-on P0 field-set ratchet holds against the current fixtures; the opt-in
CR733_CENSUS_STRICT line-level pins fold in at the next full fixture regen.
Authored via codex Run 9; ported code-only onto current main (campaign branch
had drifted; uncompressed .json census fixtures superseded by main's gzipped set).
Summary by CodeRabbit