fix(engine): serialize game-state hash collections deterministically - #6989
fix(engine): serialize game-state hash collections deterministically#6989nishu-builder wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughThe engine adds deterministic Serde support for unordered collections, validates numeric map keys, applies stable serializers across game state and event types, and adds integration tests for ordering, round trips, persistence forms, and array comparison behavior. ChangesDeterministic serialization
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GameState
participant deterministic_serde
participant Persistence
participant IntegrationTests
GameState->>deterministic_serde: serialize maps and sets
deterministic_serde->>Persistence: emit sorted serialized state
Persistence->>deterministic_serde: deserialize numeric-key maps
deterministic_serde->>GameState: restore validated state
IntegrationTests->>GameState: compare insertion-order variants
GameState->>IntegrationTests: return stable serialized bytes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 1
🧹 Nitpick comments (3)
crates/engine/src/types/deterministic_serde.rs (2)
685-708: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a key that overflows the target type, not only the
u64domain.The malformed list exercises syntax rejection and
u64overflow. It never reaches the narrowing branch ofNumericMapKey for PlayerIdat Lines 29-33, which is the only guard that rejects a player key above 255. Add aPlayerId-keyed fixture with key"256"and assert the error. Without it, replacingu8::try_fromwith a truncating cast breaks no test.🤖 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/deterministic_serde.rs` around lines 685 - 708, The malformed-key coverage in the deterministic serde tests does not exercise the narrowing guard in NumericMapKey for PlayerId. Add a PlayerId-keyed fixture using the string key "256" and assert that deserializing it returns an error, while preserving the existing malformed-key assertions.Source: Path instructions
326-344: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDelegate
im_hash_mapto the existingSortedImHashMapwrapper.The body of
im_hash_mapis identical toSortedImHashMap::serializeat Lines 414-426. Reuse the wrapper so one implementation owns the sort-then-emit sequence. The same duplication exists inhash_map_of_hash_setandhash_map_of_hash_map, which repeat theSortedHashMap::serializebody with a different value wrapper; a value-adapter generic over the wrapper would collapse those too.♻️ Proposed delegation
where K: Clone + Eq + Hash + Ord + Serialize, V: Clone + Serialize, H: BuildHasher, S: Serializer, { - let mut entries: Vec<_> = values.iter().collect(); - entries.sort_unstable_by_key(|(key, _)| *key); - - let mut map = serializer.serialize_map(Some(entries.len()))?; - for (key, value) in entries { - map.serialize_entry(key, value)?; - } - map.end() + SortedImHashMap(values).serialize(serializer) }🤖 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/deterministic_serde.rs` around lines 326 - 344, Update im_hash_map to delegate serialization to the existing SortedImHashMap wrapper instead of duplicating its sort-and-emit logic. Also consolidate hash_map_of_hash_set and hash_map_of_hash_map around SortedHashMap::serialize using a value adapter generic over the wrapper, while preserving their existing value serialization behavior.Source: Coding guidelines
crates/engine/tests/integration/deterministic_game_state_serde.rs (1)
2105-2128: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider tracking the pinned tuple-key serialization failure.
This test pins existing behavior: a nonempty
protection_start_exempt_attachmentsmakes the wholeGameStatefail to serialize with"key must be a string". Pinning it is the right scope decision for this PR, and the census at line 1116 stops the field from adopting a generic adapter.The underlying defect remains: any game state that populates this map cannot be saved.
GameObject.protection_start_exempt_attachmentsuses a(u64, u64, ObjectId)key, which JSON cannot represent as an object key. A dedicated string-key adapter, in the style oftuple_key_mapincrates/engine/src/types/game_state.rs, would resolve it.Do you want me to open an issue to track this separately?
🤖 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/tests/integration/deterministic_game_state_serde.rs` around lines 2105 - 2128, The test currently pins the known serialization failure for populated GameObject.protection_start_exempt_attachments; preserve this regression coverage and its exact "key must be a string" assertion. Do not add a serialization fix or generic adapter in this change; track the dedicated string-key adapter for the tuple-key map separately, following tuple_key_map as the eventual implementation reference.
🤖 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/deterministic_serde.rs`:
- Around line 151-160: In the map deserializer’s visit_map method, clamp the
untrusted access.size_hint() to a safe maximum before passing it to
HashMap::with_capacity. Preserve the existing zero fallback when no hint is
available and keep the subsequent entry-reading logic unchanged.
---
Nitpick comments:
In `@crates/engine/src/types/deterministic_serde.rs`:
- Around line 685-708: The malformed-key coverage in the deterministic serde
tests does not exercise the narrowing guard in NumericMapKey for PlayerId. Add a
PlayerId-keyed fixture using the string key "256" and assert that deserializing
it returns an error, while preserving the existing malformed-key assertions.
- Around line 326-344: Update im_hash_map to delegate serialization to the
existing SortedImHashMap wrapper instead of duplicating its sort-and-emit logic.
Also consolidate hash_map_of_hash_set and hash_map_of_hash_map around
SortedHashMap::serialize using a value adapter generic over the wrapper, while
preserving their existing value serialization behavior.
In `@crates/engine/tests/integration/deterministic_game_state_serde.rs`:
- Around line 2105-2128: The test currently pins the known serialization failure
for populated GameObject.protection_start_exempt_attachments; preserve this
regression coverage and its exact "key must be a string" assertion. Do not add a
serialization fix or generic adapter in this change; track the dedicated
string-key adapter for the tuple-key map separately, following tuple_key_map as
the eventual implementation reference.
🪄 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: b90ed091-1479-432c-ad4c-fe8ea7d43f4d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
crates/engine/Cargo.tomlcrates/engine/src/game/combat.rscrates/engine/src/game/game_object.rscrates/engine/src/types/ability.rscrates/engine/src/types/counter.rscrates/engine/src/types/deterministic_serde.rscrates/engine/src/types/events.rscrates/engine/src/types/game_state.rscrates/engine/src/types/identifiers.rscrates/engine/src/types/mod.rscrates/engine/src/types/player.rscrates/engine/src/types/proposed_event.rscrates/engine/src/types/resolution.rscrates/engine/tests/integration/deterministic_game_state_serde.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Two merge-blocking issues remain on this head:
-
crates/engine/src/serialization/deterministic_serde.rs:151-160passes the untrustedMapAccess::size_hint()directly toHashMap::with_capacity. A malicious or custom deserializer can advertise an enormous hint and force unreasonable preallocation; the JSON path happens to reportNonetoday, but that does not make the genericDeserializeimplementation safe. Avoid or cap the preallocation, and add an adversarialMapAccesssize-hint regression. -
The strict numeric-key tests at
crates/engine/src/serialization/deterministic_serde.rs:676-708cover onlyObjectId, whilePlayerIdconverts throughu8::try_fromat:29-33. Add aHashMap<PlayerId, _>fixture that rejects the key"256", so the actual narrow-ID boundary is covered.
|
Both findings addressed in 7a01cbb.
Verification on head
Model: gpt-5.6-sol |
|
This PR is closed as out of policy. It was created on 2026-08-04, after the 2026-07-24 declaration cutoff. The policy revision in force requires the canonical |
|
Re-opening this @nishu-builder - this shouldn't have been closed. Mistake by the review bot. |
|
Closing as redundant: the current PR head has no delta from its base (GitHub reports no changed files), and the equivalent deterministic serialization change has already landed in #7008 ( |
2 participants
Summary
Makes serialized
GameStatedeterministic: hash-backed collections (HashSet/HashMapandimequivalents) reachable from the serialized state graph now emit a canonical sorted order at their owning field/type boundary, so identical states serialize byte-identically across processes. Found by an external deterministic-replay harness: replaying an identical seeded action sequence in a fresh process diverged at one transition withplayers_who_created_token_this_turnserializing[0,1]in one process and[1,0]in the other. Ordering-semantic collections (libraries, the stack, trigger order) are untouched; numeric-keyed maps keep their exact default wire representation and gain narrow field-owned deserializers because Serde's internalContentbuffering otherwise breaks their numeric-key round-trip.Files changed
synfor the exhaustive census guard test)CR references
None — serialization determinism only; no rules behavior changes.
Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: gpt-5.6-sol
Thinking: high
Tier: Standard
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
tilt get uiresource clippy— Tilt unavailable in this worktree; used the documented direct fallback.cargo fmt --all— passed.cargo fmt --all -- --check— passed.git diff --check— passed.cargo clippy -p phase-engine --all-targets -- -D warnings— passed on headdf220c4ebae49b87aaf7b003b76fe0f2862b14f8.cargo test -p phase-engine— passed on headdf220c4ebae49b87aaf7b003b76fe0f2862b14f8(full unit + integration suites).Plan verification matrix (16 targeted test invocations incl. populated
WaitingFor::DeclareAttackers.attacker_constraintsround-trip and adverse-order byte-identity through the realGameStatefields) — all passed on headdf220c4ebae49b87aaf7b003b76fe0f2862b14f8.Revert probe — with the owning-field serializer reverted,
deterministic_game_state_serdeproduction-boundary regression failed 5/5 fresh runs; restored, passed../scripts/gen-card-data.sh— passed on headdf220c4ebae49b87aaf7b003b76fe0f2862b14f8: generated card data for ~35627 cards.cargo coverage— passed on headdf220c4ebae49b87aaf7b003b76fe0f2862b14f8: timeless legal 15123/16179 fully supported (93.5%); vintage legal 29840/32267 fully supported (92.5%).cargo semantic-audit— passed on headdf220c4ebae49b87aaf7b003b76fe0f2862b14f8: 32699 cards audited, 295 existing findings.Gate A
Gate A PASS head=df220c4ebae49b87aaf7b003b76fe0f2862b14f8 base=6d7821dced9623609edea342b47dd9c704ff0b36
Anchored on
ordered_valid_blocker_idsestablishes the deterministic-ordering-at-the-producing-seam precedent (PR fix(engine): stabilize blocker prompt ordering #5771 heritage) that this change extends to serialized state.counter_map_serdemodule owns a specialized field-boundary wire representation; the newdeterministic_serdeadapters follow the same field-owned serde-module pattern.Final review-impl
Final review-impl PASS head=df220c4ebae49b87aaf7b003b76fe0f2862b14f8
Claimed parse impact
None.
Validation Failures
Resolved local environment issue:
cargo coverageinitially failed to compile due toNo space left on device; cleared a generated target cache and reran all affected checks cleanly. Plan-review ran 4 rounds to clean (6 → 1 → 1 → 0 findings, including one executor hard-stop for a numeric-key round-trip contradiction that round 4 resolved); implementation review ran 2 rounds to clean (2 → 0 findings). Contributor-environment note per the engine-implementer skill: pipeline steps ran as isolated fresh contexts (Codex CLI sessions) with artifact-only handoffs rather than spawned Claude subagents.CI Failures
None.
Summary by CodeRabbit
Improvements
Testing