# 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`

**Follow-up:** Code review suggestions addressed (2026-05-29).

## 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` | **Matches** — Related implementation slices landed line present; Summary **Status** row includes **E5M2-10 / NEO-96 landed**. |
| `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.~~ **Done.** Summary **Status** row updated.

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.~~ **Done.** Wording narrowed to “holder cleared during **`AdvanceAll`**”; reset wrapper documents bypass + optional E9.M1 anchor; plan and README updated.

## 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`**.~~ **Done.** Redundant **`TryGet`** removed.

- 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).