From fedb735e535ecd0ad441e143d3cc0838234f0d77 Mon Sep 17 00:00:00 2001
From: VinPropane <don@donmail.org>
Date: Fri, 29 May 2026 20:49:41 -0400
Subject: [PATCH] NEO-96: add code review for E5M2-10 telemetry hook sites

---
 docs/reviews/2026-05-29-NEO-96.md | 54 +++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 docs/reviews/2026-05-29-NEO-96.md

diff --git a/docs/reviews/2026-05-29-NEO-96.md b/docs/reviews/2026-05-29-NEO-96.md
new file mode 100644
index 0000000..59d3b0f
--- /dev/null
+++ b/docs/reviews/2026-05-29-NEO-96.md
@@ -0,0 +1,54 @@
+# Code review — NEO-96 (E5M2-10)
+
+**Date:** 2026-05-29  
+**Scope:** Branch `NEO-96-e5m2-10-npc-telemetry-hooks` vs `origin/main` — commits `dedd9a9` … `5e1e374`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-96 replaces NEO-93 telemetry stubs in **`NpcRuntimeOperations.AdvanceAll`** with full NEO-84-style comment blocks for Epic 5 Slice 2 catalog events **`telegraph_fired`** (at each **`telegraph_windup`** entry) and **`npc_state_transition`** (on every committed state change, including holder clear → **`idle`**). Class summary, **`server/README.md`**, E5.M2 module doc, E5M2 backlog E5M2-10, alignment register, and implementation plan are updated. No production ingest, no new tests — appropriate for comment-only scope. **`NpcRuntimeOperations`** tests pass (**21/21**). One mid-cycle holder-clear path adds a defensive **`TryGet`** guard beyond pure comments; dev fixture **`ResetAllPrototypeRows`** bypasses hook anchors (pre-existing gap, comment wording slightly overstates coverage).
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-96-implementation-plan.md` | **Matches** — AC checked; kickoff decisions (comments-only, engine-only, telegraph at windup start, every state commit, no manual QA) reflected in code and docs; reconciliation section accurate. |
+| `docs/plans/E5M2-prototype-backlog.md` (E5M2-10) | **Matches** — AC checked, landed note present; no client counterpart (NEO-97 deferred). |
+| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Partially matches** — Related implementation slices landed line present; Summary **Status** row still jumps **E5M2-09** → **E5M2-12** without **E5M2-10 / NEO-96 landed** (same gap NEO-95 review closed for E5M2-09). |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M2 row includes **NEO-96 landed** with README/plan links. |
+| `docs/decomposition/epics/epic_05_pve_combat.md` (Slice 2 telemetry) | **Matches** — hook vocabulary `telegraph_fired`, `npc_state_transition`. |
+| `docs/decomposition/modules/client_server_authority.md` | **N/A** — no authority change; server-only comment anchors. |
+| `server/README.md` | **Matches** — NPC runtime telemetry hooks subsection with event timing and E9.M1 deferral. |
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **E5.M2 Summary status table** — Append **E5M2-10 / NEO-96 telemetry hooks landed** to the module Summary **Status** row (between E5M2-09 and E5M2-12), matching the pattern used when NEO-95 landed.
+
+2. **Fixture reset vs hook comments** — Plan transition table and the holder-clear hook comment cite “fixture reset,” but **`ResetAllPrototypeRows`** writes idle directly in the store (dev fixture API) without passing through **`AdvanceOne`** hook sites. Either add a comment-only anchor on the reset path when E9.M1 lands, or narrow comment/plan wording to “holder cleared during **`AdvanceAll`**” so implementers do not assume fixture reset is already instrumented.
+
+## Nits
+
+- Nit: Mid-cycle holder-clear path (inside the **`while`** loop) adds **`TryGet`** before **`WriteIdle`** — slightly exceeds “comments only” AC; **`snapshot`** from the top of the same iteration should already be valid, so the guard is redundant unless retained for future emit code that reads **`fromState`** from **`snapshot`**.
+
+- Nit: **`telegraph_fired`** payload comments list **`archetypeKind`** but engine scope has **`behaviorDefId`** only — fine for planned fields; E9.M1 implementer will map via behavior catalog (same as snapshot DTO projection).
+
+## Verification
+
+```bash
+# NEO-96-adjacent runtime tests (passed 2026-05-29)
+dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~NpcRuntimeOperations"
+
+# Full server suite (Postgres persistence tests require local DB)
+dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
+```
+
+Manual: none required (server-only comment hooks; kickoff decision).