jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(simulation): Sprint 19 — save/load, tier eviction, test infra #68

Closed
jpmschweitzer wants to merge 0 commits from server into main
pull from: server
merge into: :main
Conversation 1 Commits - Files Changed -
+2714 -40
jpmschweitzer commented 2026-02-25 12:14:27 +01:00
Owner
Copy Link
Copy Source

Summary

Sprint 19 server delivery — 7 tickets, 1063 tests passing.

  • #555 Protocol version handshake — HandshakeMessage as first IPC frame, HandshakeState resource, forward-compatible input handling
  • #96 State serialization — serialize_npc_to_frozen/deserialize_npc_from_frozen with full D-024 10-axis coverage
  • #98 Scope tag system — ScopeTagKind enum, ScopePinned marker, auto-assignment from KG and RelationshipGraph
  • #97 Timestamp-based eviction — LastInteractionTick LRU, SimSpacePressure, BinaryHeap eviction respecting scope pins (cap: 80)
  • #553 Save/load ECS extraction — save_to_file/load_from_file via MessagePack, SaveGame/LoadGame IPC commands
  • #200 Test module organization — Layer 3 integration test entry point, three-layer docs per D-030
  • #272 Information boundary negative tests — 4 tests proving no KG leakage, LOS fog, tier boundary, save isolation

Key decisions

D-010 (determinism), D-020 (IPC/MessagePack), D-026 (simulation tiers), D-030 (test architecture), D-041 (knowledge graph), D-085 (per-game save dirs)

Files changed

22 files, +2561/-35 lines

Test plan

  • cargo test in server/ — 1063 tests, 0 failures
  • New integration test binaries: information_boundaries, integration_layer3
  • Tier eviction tests: capacity enforcement, scope pin exclusion, LRU ordering
  • Save/load roundtrip: serialize ECS → file → deserialize → verify component equality

🤖 Generated with Claude Code

## Summary Sprint 19 server delivery — 7 tickets, 1063 tests passing. - **#555** Protocol version handshake — `HandshakeMessage` as first IPC frame, `HandshakeState` resource, forward-compatible input handling - **#96** State serialization — `serialize_npc_to_frozen`/`deserialize_npc_from_frozen` with full D-024 10-axis coverage - **#98** Scope tag system — `ScopeTagKind` enum, `ScopePinned` marker, auto-assignment from KG and RelationshipGraph - **#97** Timestamp-based eviction — `LastInteractionTick` LRU, `SimSpacePressure`, BinaryHeap eviction respecting scope pins (cap: 80) - **#553** Save/load ECS extraction — `save_to_file`/`load_from_file` via MessagePack, `SaveGame`/`LoadGame` IPC commands - **#200** Test module organization — Layer 3 integration test entry point, three-layer docs per D-030 - **#272** Information boundary negative tests — 4 tests proving no KG leakage, LOS fog, tier boundary, save isolation ## Key decisions D-010 (determinism), D-020 (IPC/MessagePack), D-026 (simulation tiers), D-030 (test architecture), D-041 (knowledge graph), D-085 (per-game save dirs) ## Files changed 22 files, +2561/-35 lines ## Test plan - `cargo test` in `server/` — 1063 tests, 0 failures - New integration test binaries: `information_boundaries`, `integration_layer3` - Tier eviction tests: capacity enforcement, scope pin exclusion, LRU ordering - Save/load roundtrip: serialize ECS → file → deserialize → verify component equality 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 3 commits 2026-02-25 12:14:27 +01:00
feat(simulation): Sprint 19 — 7 server systems 6ed8d11502 
Protocol handshake (#555): HandshakeMessage as first IPC frame,
HandshakeState resource, forward-compatible input handling.

State serialization (#96): serialize_npc_to_frozen/deserialize with
full D-024 axis coverage (10 new optional fields on NpcSaveState).

Scope tags (#98): ScopeTagKind enum, ScopePinned marker, automatic
assignment from KnowledgeGraph and RelationshipGraph.

Timestamp eviction (#97): LastInteractionTick, SimSpacePressure,
BinaryHeap LRU eviction respecting ScopePinned entities.

Save/load (#553): save_to_file/load_from_file via MessagePack,
SaveGame/LoadGame IPC commands, SaveLoadResultWire on snapshot.

Test infrastructure (#200): Layer 3 integration test entry point,
three-layer architecture documented per D-030.

Information boundary tests (#272): 4 negative tests proving no
passive KG leakage, LOS fog holds, tier boundary holds, save
isolation per NPC.

1063 tests passing, 0 failures.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog 740e97b75d 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merge remote-tracking branch 'origin/main' into server 17a9197b65 
# Conflicts:
#	CHANGELOG.md
jpmschweitzer commented 2026-02-25 12:25:50 +01:00
Author
Owner
Copy Link
Copy Source

Review: server → main (type: code)

Hoshe (Code Quality): REQUEST_CHANGES

Good code quality overall — thorough doc comments, clean system ordering in SimulationPlugin, solid test coverage at the unit level. One concrete bug and four suggestions.

# File Severity Issue
1 bridge/types.rs:20 warning PROTOCOL_VERSION not bumped to 15 after adding save_result. Changelog docstring says v15 adds save_result but the constant is still 14. One-line fix.
2 bridge/tcp.rs:48 suggestion Misleading comment — says read_framed handles WouldBlock by returning Ok(None), but Ok(None) means EOF. WouldBlock propagates as Err and is handled at line 202. Comment should say so.
3 simulation/save_io.rs:59 suggestion Silent overwrite of pending save/load command — no tracing::warn! when a second command arrives before the first executes. Double-tap F5 edge case.
4 simulation/tier.rs:358 suggestion SimSpacePressure.active_count reflects pre-eviction count, not post-eviction. Doc note on the field would prevent future systems from reading stale data.
5 simulation/save_io.rs:85 suggestion KnowledgeGraph::new() fallback silently drops player knowledge if player entity missing at save time. A tracing::warn! on the fallback branch would surface this.

Tyre (Architecture): REQUEST_CHANGES

Architecturally sound — D-010 determinism respected, D-020 MessagePack used correctly, information boundaries honoured in serialization model. Same protocol version bug independently flagged.

# File Severity Issue
1 bridge/types.rs:20 warning PROTOCOL_VERSION not bumped to 15. Same as Hoshe — the constant and ObserverSnapshot.version doc comment must both say 15. Same mismatch pattern fixed in PR #66.
2 simulation/save_io.rs:56 warning SaveLoadPending silently drops commands when a second arrives before the first executes. No test for the overwrite case, no tracing log, no client feedback. At minimum a tracing::warn! on overwrite.
3 simulation/tier.rs:339 suggestion No test asserting ScopePinned NPCs survive evict_excess_active. The Without<ScopePinned> query filter is the core invariant of scope tags — needs regression coverage.
4 simulation/tier.rs:370 suggestion Entity-based tie-breaking in eviction heap is non-deterministic across save/load cycles. Acceptable because tier assignment is re-derived on load, but should be documented inline.
5 tests/information_boundaries.rs suggestion File was inaccessible during review — could not verify D-012 test coverage. Needs human eyes before merge.

Verdict: CHANGES REQUESTED

Required fixes before merge:

  1. Bump PROTOCOL_VERSION to 15 and update doc comment on ObserverSnapshot.version
  2. Add tracing::warn! when SaveLoadPending overwrites a pending command

🤖 Generated with Claude Code

## Review: server → main (type: code) ### Hoshe (Code Quality): REQUEST_CHANGES Good code quality overall — thorough doc comments, clean system ordering in SimulationPlugin, solid test coverage at the unit level. One concrete bug and four suggestions. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `bridge/types.rs:20` | warning | `PROTOCOL_VERSION` not bumped to 15 after adding `save_result`. Changelog docstring says v15 adds `save_result` but the constant is still 14. One-line fix. | | 2 | `bridge/tcp.rs:48` | suggestion | Misleading comment — says `read_framed` handles `WouldBlock` by returning `Ok(None)`, but `Ok(None)` means EOF. `WouldBlock` propagates as `Err` and is handled at line 202. Comment should say so. | | 3 | `simulation/save_io.rs:59` | suggestion | Silent overwrite of pending save/load command — no `tracing::warn!` when a second command arrives before the first executes. Double-tap F5 edge case. | | 4 | `simulation/tier.rs:358` | suggestion | `SimSpacePressure.active_count` reflects pre-eviction count, not post-eviction. Doc note on the field would prevent future systems from reading stale data. | | 5 | `simulation/save_io.rs:85` | suggestion | `KnowledgeGraph::new()` fallback silently drops player knowledge if player entity missing at save time. A `tracing::warn!` on the fallback branch would surface this. | ### Tyre (Architecture): REQUEST_CHANGES Architecturally sound — D-010 determinism respected, D-020 MessagePack used correctly, information boundaries honoured in serialization model. Same protocol version bug independently flagged. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `bridge/types.rs:20` | warning | `PROTOCOL_VERSION` not bumped to 15. Same as Hoshe — the constant and `ObserverSnapshot.version` doc comment must both say 15. Same mismatch pattern fixed in PR #66. | | 2 | `simulation/save_io.rs:56` | warning | `SaveLoadPending` silently drops commands when a second arrives before the first executes. No test for the overwrite case, no tracing log, no client feedback. At minimum a `tracing::warn!` on overwrite. | | 3 | `simulation/tier.rs:339` | suggestion | No test asserting `ScopePinned` NPCs survive `evict_excess_active`. The `Without<ScopePinned>` query filter is the core invariant of scope tags — needs regression coverage. | | 4 | `simulation/tier.rs:370` | suggestion | Entity-based tie-breaking in eviction heap is non-deterministic across save/load cycles. Acceptable because tier assignment is re-derived on load, but should be documented inline. | | 5 | `tests/information_boundaries.rs` | suggestion | File was inaccessible during review — could not verify D-012 test coverage. Needs human eyes before merge. | ### Verdict: CHANGES REQUESTED **Required fixes before merge:** 1. Bump `PROTOCOL_VERSION` to 15 and update doc comment on `ObserverSnapshot.version` 2. Add `tracing::warn!` when `SaveLoadPending` overwrites a pending command 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jpmschweitzer added 1 commit 2026-02-25 12:32:40 +01:00
fix(simulation): PR #68 review — version bump, tracing warns, test coverage 23fbfdbfc5 
- Bump PROTOCOL_VERSION 14 → 15 for save_result field addition
- Add tracing::warn on SaveLoadPending command overwrite (double-tap F5)
- Add tracing::warn on KnowledgeGraph::new() fallback during save
- Fix misleading WouldBlock comment in tcp.rs
- Document SimSpacePressure.active_count pre-eviction timing
- Document entity-based eviction tie-breaking non-determinism
- Add ScopePinned eviction survival regression test
- Regenerate msgpack fixtures for protocol v15

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer closed this pull request 2026-02-25 12:45:46 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#68