feat(server): Sprint 23 — tile system, storyteller activation, debug console, LOS fix #81

2026-03-03T19:13:38+01:00

jpmschweitzer commented

2026-03-03 19:13:38 +01:00

Summary

Sprint 23 server deliverables — 9 tickets across 4 parallel tracks:

Spatial track (#576, #577, #578)

TileKind enum (Floor/Wall/Void/Restricted) on WalkabilityMap
Location YAML tile format with content loader — stamps tile data on production startup
Chunk streaming system — cadence-gated load/unload around player position

Storyteller track (#570, #571, #572, #579)

EngagementRecord component — per-NPC observation/conversation/monologue metrics
MovementHistoryBuffer resource — 3000-tick ring buffer with proximity query
Lifecycle rules — single activation, no concurrency, terminal resolution
activation_pass() system — full 6-step spec from #162

Debug track (#580)

10 DebugCommandKind variants with handler system
DebugResponsePayload on ObserverSnapshot
PROTOCOL_VERSION 17 → 18

LOS fix (#584)

BoundaryWall visibility sector — 1-tile wall margin beyond LOS cone
Walls at fog edge now render correctly

Test results

1133 lib tests + 51 serialization + 7 integration + 1 golden = all green
45 new tests added across all features

Cross-team notes

Client (#581): Must bump PROTOCOL_VERSION to 18 and handle debug_response field
Visual (#582, #583): Tile format published in types.rs — Araminta can start authoring
Client (#585): BoundaryWall sector ready for client-side rendering

## Summary Sprint 23 server deliverables — 9 tickets across 4 parallel tracks: ### Spatial track (#576, #577, #578) - TileKind enum (Floor/Wall/Void/Restricted) on WalkabilityMap - Location YAML tile format with content loader — stamps tile data on production startup - Chunk streaming system — cadence-gated load/unload around player position ### Storyteller track (#570, #571, #572, #579) - EngagementRecord component — per-NPC observation/conversation/monologue metrics - MovementHistoryBuffer resource — 3000-tick ring buffer with proximity query - Lifecycle rules — single activation, no concurrency, terminal resolution - activation_pass() system — full 6-step spec from #162 ### Debug track (#580) - 10 DebugCommandKind variants with handler system - DebugResponsePayload on ObserverSnapshot - PROTOCOL_VERSION 17 → 18 ### LOS fix (#584) - BoundaryWall visibility sector — 1-tile wall margin beyond LOS cone - Walls at fog edge now render correctly ## Test results - 1133 lib tests + 51 serialization + 7 integration + 1 golden = all green - 45 new tests added across all features ## Cross-team notes - **Client (#581):** Must bump PROTOCOL_VERSION to 18 and handle `debug_response` field - **Visual (#582, #583):** Tile format published in types.rs — Araminta can start authoring - **Client (#585):** BoundaryWall sector ready for client-side rendering

jpmschweitzer added 5 commits 2026-03-03 19:13:38 +01:00

feat(simulation): add tile-type layer, content loader, and chunk streaming 7fbfc5ae65

Spatial chain for Sprint 23 (#576, #577, #578):

- TileKind enum (Floor/Wall/Void/Restricted) on WalkabilityMap with
  set_tile_kind/tile_kind API, backward-compatible with existing
  is_walkable/set_walkable
- Location YAML tile format: tiles as string arrays (F/W/V/R chars),
  load_location_tiles() stamps tile data onto WalkabilityMap from
  ContentStore on production startup
- Chunk streaming system: ChunkLoadRadius + ChunkStreamingCadence
  resources, loads/unloads chunks around player position on cadence.
  v0.1 radius covers full district (no streaming stutter)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

feat(simulation): add storyteller engagement tracking and activation pass 50f5d6c22e

Storyteller chain for Sprint 23 (#570, #571, #572, #579):

- EngagementRecord component: per-NPC observation_time_ticks,
  conversation_count, monologue_trigger_count — incremented by
  perception, dialogue, and monologue systems
- MovementHistoryBuffer resource: ring buffer of player positions
  over last 3000 ticks with npcs_copresent_in_window() query
- Lifecycle rules: single activation per session, no concurrency,
  no cooldown, terminal resolution constants
- activation_pass() system: gate check, proximity query, engagement
  scoring, unentangled-NPC routing, module selection, emits
  TriangleActivatedEvent on 10-tick cadence after contamination

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

feat(simulation): add debug console server and LOS boundary wall margin c30197db0e

Debug and LOS tracks for Sprint 23 (#580, #584):

- DebugCommandKind enum with 10 variants (AdvanceTicks,
  SkipToContamination, TeleportToPosition, ForceContaminationActivate,
  InspectNpc, ListTriangles, ListPopulation, GetContaminationStatus,
  TeleportToLocation, ForceTriangleActivation)
- DebugResponsePayload on ObserverSnapshot, handle_debug_commands
  system gated by DebugEnabled resource
- PROTOCOL_VERSION bumped 17 → 18
- VisibilitySector::BoundaryWall variant — 1-tile wall margin beyond
  LOS boundary included in visible_tiles (not exploration/memory)
- compute_boundary_walls() pass in NaturalVision after FOV+cone

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chore(simulation): update tests and fixtures for Sprint 23 c797a72fc2

- Add debug_response: None to all ObserverSnapshot constructors in
  integration tests
- Bump PROTOCOL_VERSION assertion 17 → 18 in serialization tests
- Regenerate golden proof_room_tick_10.json (BoundaryWall tiles)
- Regenerate client msgpack fixtures for new snapshot fields
- Fix debug.rs resource optionality (Option<ResMut> for
  ContaminationActive/EventQueue)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chore(meta): update changelog eb64f23b77

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpmschweitzer commented

2026-03-03 21:40:31 +01:00

Review: `server` → `main` (type: code)

Hoshe (Code Quality): REQUEST_CHANGES

Summary: Well-structured sprint delivering five features with good test coverage. However, the deep pass on core systems uncovered critical issues: inverted tile_bounds cause silent map corruption via integer overflow, debug commands can corrupt game state (time rewind, player stuck in walls), and the storyteller has edge cases around duplicate NPC candidates and precision loss in long sessions.

Pass 1

#	File:line	Severity	Issue
1	`storyteller/mod.rs:561`	critical	`.expect()` in production `activation_pass` — panics if triangle entity is despawned between snapshot and lookup. Guard with `unwrap_or_else` + log.
2	`bridge/debug.rs:78-301`	warning	Last response wins silently drops earlier debug responses with no log.
3	`bridge/types.rs:558-560`	warning	`DebugEnabled` defaults to `true` in all builds — no `#[cfg(debug_assertions)]` guard.
4	`bridge/debug.rs:116-128`	warning	`SkipToContamination` sets tick exactly to delay threshold — off-by-one confusion in response message.
5	`content/loader.rs:380-403`	warning	Row/width mismatches only warned, not errored — content typos produce invisible walkability holes.

Pass 2 (Deep — Core Systems)

#	File:line	Severity	Issue
A	`storyteller/mod.rs:451`	warning	`observation_time_ticks as f32` loses precision beyond ~16.8M ticks; score differentiation breaks in long sessions.
B	`storyteller/mod.rs:512-531`	warning	`copresent` can contain duplicate entities — same NPC added to candidates twice, doubling its RNG weight. Deduplicate before scoring.
C	`content/loader.rs:380-381`	critical	Inverted `tile_bounds` (`x_max < x_min`) causes `i32 as usize` wrap to ~18 quintillion, silently writing tiles at wrong positions. No validation.
D	`content/types.rs:45`	warning	`DistrictMeta.description` is required (no `serde(default)`) — missing field silently drops entire district with all NPCs/triangles.
E	`bridge/debug.rs:130-150`	warning	`TeleportToPosition` does not validate target — player can be stuck in wall/void permanently.
F	`bridge/debug.rs:116-128`	warning	`SkipToContamination` when tick already past delay silently rewinds time, causing all cooldowns to fire immediately.
G	`storyteller/mod.rs:488`	warning	`activation_pass` fires at tick 0 if contamination pre-set (save/load) — all NPCs score 0.0, selection is random.

Tyre (Architecture): REQUEST_CHANGES

Summary: Architecturally sound sprint. The tile system, chunk streaming, and storyteller activation follow established patterns. However, three critical architectural issues emerged: ActivationState is not serialized on save (breaks one-shot activation guarantee after save/load), MovementHistoryBuffer is not reset on load (stale engagement data), and activation_pass has no explicit system ordering relative to advance_tick (latent determinism risk).

Pass 1

#	File:line	Severity	Issue
1	`bridge/types.rs:78`	warning	`ObserverSnapshot.version` doc says Current: 17 — should be 18.
2	`bridge/types.rs:73-75`	warning	Version history missing `v18 adds:` entry for debug_response.
3	`storyteller/mod.rs:463-465`	warning	Doc claims D-025 fallback that is not implemented — misleading for reviewers.

Pass 2 (Deep — Core Systems)

#	File:line	Severity	Issue
7	`save_io.rs:149-151,268-274`	critical	`ActivationState` not serialized — save/load after activation resets the guard, causing double-activation (breaks one-shot guarantee).
8	`save_io.rs:271-274`	warning	`TriangleActivatedQueue` and `MovementHistoryBuffer` not reset on load — stale events/history carry over into new session.
9	`storyteller/mod.rs:375,383-385`	warning	`activation_pass` has no explicit ordering vs `advance_tick` — scheduler-dependent; could skip cadence tick on Bevy version change. Fix with `.before(time::advance_tick)`.
10	`save_state.rs:347-397`	warning	`deserialize_npc_from_frozen` does not insert `EngagementRecord` — loaded NPCs silently score 0.0, engagement history lost after load.

Verdict: CHANGES REQUESTED

Critical issues (must fix):

ActivationState not saved (Tyre #7) — guaranteed double-activation on save/load. Same pattern as contamination_active persistence, just missing.
Inverted tile_bounds (Hoshe #C) — integer overflow silently corrupts walkability map. Add x_min <= x_max / y_min <= y_max validation.
Storyteller .expect() panic (Hoshe #1) — production panic path in core gameplay loop.

High-priority warnings (strongly recommend fixing):

Save/load: reset TriangleActivatedQueue + MovementHistoryBuffer on load; insert EngagementRecord on NPC deserialize
System ordering: explicit .before(advance_tick) on activation_pass
Debug console: validate teleport target, guard time rewind, cfg(debug_assertions) on DebugEnabled
Content loader: error (not warn) on tile dimension mismatches, make DistrictMeta.description optional
Storyteller: deduplicate copresent list, guard tick 0 activation

## Review: `server` → `main` (type: code) ### Hoshe (Code Quality): REQUEST_CHANGES **Summary:** Well-structured sprint delivering five features with good test coverage. However, the deep pass on core systems uncovered critical issues: inverted tile_bounds cause silent map corruption via integer overflow, debug commands can corrupt game state (time rewind, player stuck in walls), and the storyteller has edge cases around duplicate NPC candidates and precision loss in long sessions. #### Pass 1 | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 1 | `storyteller/mod.rs:561` | critical | `.expect()` in production `activation_pass` — panics if triangle entity is despawned between snapshot and lookup. Guard with `unwrap_or_else` + log. | | 2 | `bridge/debug.rs:78-301` | warning | Last response wins silently drops earlier debug responses with no log. | | 3 | `bridge/types.rs:558-560` | warning | `DebugEnabled` defaults to `true` in all builds — no `#[cfg(debug_assertions)]` guard. | | 4 | `bridge/debug.rs:116-128` | warning | `SkipToContamination` sets tick exactly to delay threshold — off-by-one confusion in response message. | | 5 | `content/loader.rs:380-403` | warning | Row/width mismatches only warned, not errored — content typos produce invisible walkability holes. | #### Pass 2 (Deep — Core Systems) | # | File:line | Severity | Issue | |---|-----------|----------|-------| | A | `storyteller/mod.rs:451` | warning | `observation_time_ticks as f32` loses precision beyond ~16.8M ticks; score differentiation breaks in long sessions. | | B | `storyteller/mod.rs:512-531` | warning | `copresent` can contain duplicate entities — same NPC added to candidates twice, doubling its RNG weight. Deduplicate before scoring. | | C | `content/loader.rs:380-381` | **critical** | Inverted `tile_bounds` (`x_max < x_min`) causes `i32 as usize` wrap to ~18 quintillion, silently writing tiles at wrong positions. No validation. | | D | `content/types.rs:45` | warning | `DistrictMeta.description` is required (no `serde(default)`) — missing field silently drops entire district with all NPCs/triangles. | | E | `bridge/debug.rs:130-150` | warning | `TeleportToPosition` does not validate target — player can be stuck in wall/void permanently. | | F | `bridge/debug.rs:116-128` | warning | `SkipToContamination` when tick already past delay silently **rewinds time**, causing all cooldowns to fire immediately. | | G | `storyteller/mod.rs:488` | warning | `activation_pass` fires at tick 0 if contamination pre-set (save/load) — all NPCs score 0.0, selection is random. | --- ### Tyre (Architecture): REQUEST_CHANGES **Summary:** Architecturally sound sprint. The tile system, chunk streaming, and storyteller activation follow established patterns. However, three critical architectural issues emerged: `ActivationState` is not serialized on save (breaks one-shot activation guarantee after save/load), `MovementHistoryBuffer` is not reset on load (stale engagement data), and `activation_pass` has no explicit system ordering relative to `advance_tick` (latent determinism risk). #### Pass 1 | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 1 | `bridge/types.rs:78` | warning | `ObserverSnapshot.version` doc says Current: 17 — should be 18. | | 2 | `bridge/types.rs:73-75` | warning | Version history missing `v18 adds:` entry for debug_response. | | 3 | `storyteller/mod.rs:463-465` | warning | Doc claims D-025 fallback that is not implemented — misleading for reviewers. | #### Pass 2 (Deep — Core Systems) | # | File:line | Severity | Issue | |---|-----------|----------|-------| | 7 | `save_io.rs:149-151,268-274` | **critical** | `ActivationState` not serialized — save/load after activation **resets the guard**, causing double-activation (breaks one-shot guarantee). | | 8 | `save_io.rs:271-274` | warning | `TriangleActivatedQueue` and `MovementHistoryBuffer` not reset on load — stale events/history carry over into new session. | | 9 | `storyteller/mod.rs:375,383-385` | warning | `activation_pass` has no explicit ordering vs `advance_tick` — scheduler-dependent; could skip cadence tick on Bevy version change. Fix with `.before(time::advance_tick)`. | | 10 | `save_state.rs:347-397` | warning | `deserialize_npc_from_frozen` does not insert `EngagementRecord` — loaded NPCs silently score 0.0, engagement history lost after load. | --- ### Verdict: CHANGES REQUESTED **Critical issues (must fix):** 1. **ActivationState not saved** (Tyre #7) — guaranteed double-activation on save/load. Same pattern as `contamination_active` persistence, just missing. 2. **Inverted tile_bounds** (Hoshe #C) — integer overflow silently corrupts walkability map. Add `x_min <= x_max` / `y_min <= y_max` validation. 3. **Storyteller `.expect()` panic** (Hoshe #1) — production panic path in core gameplay loop. **High-priority warnings (strongly recommend fixing):** - Save/load: reset `TriangleActivatedQueue` + `MovementHistoryBuffer` on load; insert `EngagementRecord` on NPC deserialize - System ordering: explicit `.before(advance_tick)` on `activation_pass` - Debug console: validate teleport target, guard time rewind, `cfg(debug_assertions)` on `DebugEnabled` - Content loader: error (not warn) on tile dimension mismatches, make `DistrictMeta.description` optional - Storyteller: deduplicate `copresent` list, guard tick 0 activation

jpmschweitzer referenced this issue from a commit

2026-03-03 23:41:07 +01:00

fix(simulation): address PR #81 review — critical and high-priority issues

jpmschweitzer added 1 commit 2026-03-03 23:41:07 +01:00

fix(simulation): address PR #81 review — critical and high-priority issues ac68ec6eef

Critical fixes:
- storyteller: replace .expect() with guard + log in activation_pass (Hoshe #1)
- content/loader: validate inverted tile_bounds before iteration (Hoshe #C)
- save_io: persist ActivationState on save/load (Tyre #7)

High-priority fixes:
- storyteller: f64 intermediate for observation_time_ticks scoring (Hoshe #A)
- storyteller: deduplicate copresent entities before scoring (Hoshe #B)
- storyteller: skip activation_pass at tick 0 (Hoshe #G)
- storyteller: explicit .before(advance_tick) ordering (Tyre #9)
- save_state: insert EngagementRecord on NPC deserialize (Tyre #10)
- save_io: reset TriangleActivatedQueue + MovementHistoryBuffer on load (Tyre #8)
- debug: validate teleport target walkability (Hoshe #E)
- debug: reject SkipToContamination when tick past delay (Hoshe #F)
- debug: DebugEnabled defaults to cfg!(debug_assertions) (Hoshe #3)
- debug: log when response overwritten (Hoshe #2)
- content/loader: error on tile dimension mismatch (Hoshe #5)
- content/types: DistrictMeta.description optional (Hoshe #D)
- types: version docs updated to v18 (Tyre #1, #2, #3)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

jpmschweitzer commented

2026-03-03 23:41:53 +01:00

Review Fixes Applied (`ac68ec6`)

All 3 critical and 14 high-priority issues from the review have been addressed:

Critical (3/3 fixed)

#	Issue	Fix
Hoshe #1	`.expect()` panic in activation_pass	Replaced with `let-else` guard + error log
Hoshe #C	Inverted tile_bounds integer overflow	Validation before iteration; skips with error
Tyre #7	ActivationState not serialized	Save/load roundtrip with `serde(default)`; 2 new tests

High-priority (14/14 fixed)

#	Issue	Fix
Hoshe #A	f32 precision loss in observation_time_ticks	f64 intermediate in scoring
Hoshe #B	Duplicate copresent entities	`sort + dedup` before scoring
Hoshe #G	activation_pass fires at tick 0	Guard: skip tick 0
Hoshe #2	Last-response-wins silent drop	trace log on overwrite
Hoshe #3	DebugEnabled defaults true in all builds	`cfg!(debug_assertions)` + test
Hoshe #5	Tile dimension warn-not-error	error + skip to prevent walkability holes
Hoshe #D	DistrictMeta.description required	`serde(default)`
Hoshe #E	Teleport to wall/void	WalkabilityMap validation + test
Hoshe #F	SkipToContamination time rewind	Reject when tick past delay + test
Tyre #1/#2/#3	Version docs stale	Updated to v18 with history entry
Tyre #8	Stale queues/buffers on load	Reset TriangleActivatedQueue + MovementHistoryBuffer
Tyre #9	activation_pass ordering	`.before(advance_tick)`
Tyre #10	EngagementRecord missing on NPC load	Insert in `deserialize_npc_from_frozen`

Test results

1319 passed, 0 failed (1138 lib + 51 serialization + integration/golden). 5 new tests added for review fixes.

## Review Fixes Applied (ac68ec6) All 3 critical and 14 high-priority issues from the review have been addressed: ### Critical (3/3 fixed) | # | Issue | Fix | |---|-------|-----| | Hoshe #1 | `.expect()` panic in activation_pass | Replaced with `let-else` guard + error log | | Hoshe #C | Inverted tile_bounds integer overflow | Validation before iteration; skips with error | | Tyre #7 | ActivationState not serialized | Save/load roundtrip with `serde(default)`; 2 new tests | ### High-priority (14/14 fixed) | # | Issue | Fix | |---|-------|-----| | Hoshe #A | f32 precision loss in observation_time_ticks | f64 intermediate in scoring | | Hoshe #B | Duplicate copresent entities | `sort + dedup` before scoring | | Hoshe #G | activation_pass fires at tick 0 | Guard: skip tick 0 | | Hoshe #2 | Last-response-wins silent drop | trace log on overwrite | | Hoshe #3 | DebugEnabled defaults true in all builds | `cfg!(debug_assertions)` + test | | Hoshe #5 | Tile dimension warn-not-error | error + skip to prevent walkability holes | | Hoshe #D | DistrictMeta.description required | `serde(default)` | | Hoshe #E | Teleport to wall/void | WalkabilityMap validation + test | | Hoshe #F | SkipToContamination time rewind | Reject when tick past delay + test | | Tyre #1/#2/#3 | Version docs stale | Updated to v18 with history entry | | Tyre #8 | Stale queues/buffers on load | Reset TriangleActivatedQueue + MovementHistoryBuffer | | Tyre #9 | activation_pass ordering | `.before(advance_tick)` | | Tyre #10 | EngagementRecord missing on NPC load | Insert in `deserialize_npc_from_frozen` | ### Test results 1319 passed, 0 failed (1138 lib + 51 serialization + integration/golden). 5 new tests added for review fixes.

jpmschweitzer commented

2026-03-03 23:45:52 +01:00

Re-review: `server` → `main` — after fix commit `ac68ec6`

Hoshe (Code Quality): APPROVE

All 3 criticals and 14 warnings from the first review verified fixed. 3 minor new observations (all low severity, not blockers):

.unwrap() calls in ForceContaminationActivate arm — provably safe but stylistically inconsistent
SkipToContamination success message when already active shows current tick twice
minimal_world() test helper doesn't initialize TriangleCrisisEventQueue

Tyre (Architecture): APPROVE

All 7 architectural issues verified correct. #[serde(default)] on new SaveStateV1 fields ensures backwards compat with old saves. System ordering chain confirmed: validate_movement → append_player_history → activation_pass → advance_tick. One informational note about DebugEnabled being build-mode conditional — documented, intentional, fine for v0.1.

Verdict: APPROVED

## Re-review: `server` → `main` — after fix commit ac68ec6 ### Hoshe (Code Quality): APPROVE All 3 criticals and 14 warnings from the first review verified fixed. 3 minor new observations (all low severity, not blockers): - `.unwrap()` calls in `ForceContaminationActivate` arm — provably safe but stylistically inconsistent - `SkipToContamination` success message when already active shows current tick twice - `minimal_world()` test helper doesn't initialize `TriangleCrisisEventQueue` ### Tyre (Architecture): APPROVE All 7 architectural issues verified correct. `#[serde(default)]` on new `SaveStateV1` fields ensures backwards compat with old saves. System ordering chain confirmed: `validate_movement` → `append_player_history` → `activation_pass` → `advance_tick`. One informational note about `DebugEnabled` being build-mode conditional — documented, intentional, fine for v0.1. ### Verdict: APPROVED

jpmschweitzer closed this pull request

2026-03-03 23:45:59 +01:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#81

feat(server): Sprint 23 — tile system, storyteller activation, debug console, LOS fix #81

Summary

Spatial track (#576, #577, #578)

Storyteller track (#570, #571, #572, #579)

Debug track (#580)

LOS fix (#584)

Test results

Cross-team notes

Review: server → main (type: code)

Hoshe (Code Quality): REQUEST_CHANGES

Pass 1

Pass 2 (Deep — Core Systems)

Tyre (Architecture): REQUEST_CHANGES

Pass 1

Pass 2 (Deep — Core Systems)

Verdict: CHANGES REQUESTED

Review Fixes Applied (ac68ec6)

Critical (3/3 fixed)

High-priority (14/14 fixed)

Test results

Re-review: server → main — after fix commit ac68ec6

Hoshe (Code Quality): APPROVE

Tyre (Architecture): APPROVE

Verdict: APPROVED

Pull request closed

Review: `server` → `main` (type: code)

Review Fixes Applied (`ac68ec6`)

Re-review: `server` → `main` — after fix commit `ac68ec6`