NEO-96: address code review — fixture wording, E5.M2 status

Narrow holder-clear hook comments to AdvanceAll scope; document
ResetAllPrototypeRows bypass with optional E9.M1 anchor; remove
redundant TryGet; update E5.M2 Summary status and review follow-up.

docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md

@@ -7,7 +7,7 @@
 | **Module ID** | E5.M2 |
 | **Epic** | [Epic 5 — PvE Combat](../epics/epic_05_pve_combat.md) |
 | **Stage target** | Prototype |
-| **Status** | In Progress — Slice 2 backlog [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md): **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) catalog + CI landed · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) server load landed · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) registry + DI landed · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) behavior-definitions GET landed · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) NPC instance registry + combat-target migration landed · **E5M2-06** [NEO-92](https://linear.app/neon-sprawl/issue/NEO-92) aggro rule + threat store landed · **E5M2-07** [NEO-93](https://linear.app/neon-sprawl/issue/NEO-93) NPC runtime state machine landed · **E5M2-08** [NEO-94](https://linear.app/neon-sprawl/issue/NEO-94) runtime snapshot GET landed · **E5M2-09** [NEO-95](https://linear.app/neon-sprawl/issue/NEO-95) NPC attack resolve + player combat HP landed → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) |
+| **Status** | In Progress — Slice 2 backlog [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md): **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) catalog + CI landed · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) server load landed · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) registry + DI landed · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) behavior-definitions GET landed · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) NPC instance registry + combat-target migration landed · **E5M2-06** [NEO-92](https://linear.app/neon-sprawl/issue/NEO-92) aggro rule + threat store landed · **E5M2-07** [NEO-93](https://linear.app/neon-sprawl/issue/NEO-93) NPC runtime state machine landed · **E5M2-08** [NEO-94](https://linear.app/neon-sprawl/issue/NEO-94) runtime snapshot GET landed · **E5M2-09** [NEO-95](https://linear.app/neon-sprawl/issue/NEO-95) NPC attack resolve + player combat HP landed · **E5M2-10** [NEO-96](https://linear.app/neon-sprawl/issue/NEO-96) NPC runtime telemetry hooks landed → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) |
 | **Linear** | Label **`E5.M2`** · [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md) **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) |
 
 ## Purpose

docs/plans/NEO-96-implementation-plan.md

@@ -52,7 +52,8 @@
 ## Implementation reconciliation (shipped)
 
 - NEO-93 `(NEO-96)` stub comments replaced with full NEO-84-style hook blocks in **`NpcRuntimeOperations.AdvanceAll`**.
-- **`telegraph_fired`** at aggro→telegraph and recover→telegraph entries; **`npc_state_transition`** on every state commit including holder clear → idle.
+- **`telegraph_fired`** at aggro→telegraph and recover→telegraph entries; **`npc_state_transition`** on every state commit during **`AdvanceAll`** (holder clear → idle).
+- Dev fixture **`ResetAllPrototypeRows`** documented as bypassing **`AdvanceOne`** hooks; optional E9.M1 anchor on reset wrapper.
 - **`server/README.md`**, **E5_M2** module doc, **E5M2** backlog, and alignment register updated.
 - No new tests (comment-only); existing NPC runtime tests unchanged.
 
@@ -84,7 +85,9 @@ Immediately after every **`WriteState`** / **`WriteIdle`** commit:
 | Windup complete | `telegraph_windup` → `recover` (logical attack; no visible `attack_execute` phase) |
 | Defensive `AttackExecute` branch | `attack_execute` → `recover` |
 | Recover cooldown complete | `recover` → `telegraph_windup` |
-| Holder cleared / fixture reset | any → `idle` |
+| Holder cleared during **`AdvanceAll`** | any → `idle` |
+
+**Not instrumented in NEO-96:** dev fixture **`ResetAllPrototypeRows`** writes idle directly (bypasses **`AdvanceOne`**); optional E9.M1 anchor noted on reset wrapper.
 
 Comment block names future E9.M1 event **`npc_state_transition`**. **`TODO(E9.M1): catalog emit`** — once per committed transition.
 

docs/reviews/2026-05-29-NEO-96.md

@@ -4,6 +4,8 @@
 **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**
@@ -18,7 +20,7 @@ NEO-96 replaces NEO-93 telemetry stubs in **`NpcRuntimeOperations.AdvanceAll`**
 |------|--------|
 | `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/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. |
@@ -30,13 +32,13 @@ 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.
+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.
+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`**.
+- ~~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).
 

server/NeonSprawl.Server/Game/Npc/NpcRuntimeOperations.cs

@@ -85,8 +85,12 @@ public static class NpcRuntimeOperations
     }
 
     /// <summary>Resets all prototype NPC runtime rows to idle (dev fixture).</summary>
-    public static void ResetAllPrototypeRows(INpcRuntimeStateStore runtimeStore) =>
+    public static void ResetAllPrototypeRows(INpcRuntimeStateStore runtimeStore)
+    {
+        // NEO-96: dev fixture reset writes idle directly — bypasses AdvanceOne npc_state_transition hooks.
+        // TODO(E9.M1): optional catalog emit anchor here if fixture reset transitions must be tracked.
         runtimeStore.ResetAllPrototypeRows();
+    }
 
     private static void AdvanceOne(
         string npcInstanceId,
@@ -119,7 +123,7 @@ public static class NpcRuntimeOperations
             {
                 WriteIdle(runtimeStore, npcInstanceId);
                 // --- Telemetry hook site (NEO-96): future E9.M1 catalog event `npc_state_transition` ---
-                // TODO(E9.M1): catalog emit — holder cleared or fixture reset (any → idle).
+                // TODO(E9.M1): catalog emit — holder cleared during AdvanceAll (any → idle).
                 // Planned payload fields: npcInstanceId, behaviorDefId, fromState, toState (idle),
                 // phaseStartedUtc, optional aggroHolderPlayerId (previous holder when known).
             }
@@ -150,14 +154,9 @@ public static class NpcRuntimeOperations
         {
             if (!TryHasAggroHolder(npcInstanceId, threatStore, out aggroHolderPlayerId))
             {
-                if (!runtimeStore.TryGet(npcInstanceId, out snapshot))
-                {
-                    return;
-                }
-
                 WriteIdle(runtimeStore, npcInstanceId);
                 // --- Telemetry hook site (NEO-96): future E9.M1 catalog event `npc_state_transition` ---
-                // TODO(E9.M1): catalog emit — holder cleared mid-cycle (any → idle).
+                // TODO(E9.M1): catalog emit — holder cleared mid-cycle during AdvanceAll (any → idle).
                 // Planned payload fields: npcInstanceId, behaviorDefId, fromState, toState (idle),
                 // phaseStartedUtc, optional aggroHolderPlayerId (previous holder).
                 return;

server/README.md

@@ -190,7 +190,7 @@ Plan: [NEO-93 implementation plan](../../docs/plans/NEO-93-implementation-plan.m
 
 ### NPC runtime telemetry hooks (NEO-96)
 
-Comment-only hook sites in **`NpcRuntimeOperations.AdvanceAll`** for future E9.M1 catalog events — **`telegraph_fired`** at each **`telegraph_windup`** entry (aggro→telegraph and recover→telegraph); **`npc_state_transition`** on every committed state change (including holder clear → **`idle`**). **`TODO(E9.M1)`** — no production ingest. Snapshot GET invokes **`AdvanceAll`** before read (no duplicate API-layer hooks). Plan: [NEO-96 implementation plan](../../docs/plans/NEO-96-implementation-plan.md).
+Comment-only hook sites in **`NpcRuntimeOperations.AdvanceAll`** for future E9.M1 catalog events — **`telegraph_fired`** at each **`telegraph_windup`** entry (aggro→telegraph and recover→telegraph); **`npc_state_transition`** on every committed state change during **`AdvanceAll`** (including holder clear → **`idle`**). Dev fixture **`ResetAllPrototypeRows`** writes idle directly and bypasses **`AdvanceOne`** hooks (optional E9.M1 anchor noted on reset path). **`TODO(E9.M1)`** — no production ingest. Snapshot GET invokes **`AdvanceAll`** before read (no duplicate API-layer hooks). Plan: [NEO-96 implementation plan](../../docs/plans/NEO-96-implementation-plan.md).
 
 ## NPC runtime snapshot (NEO-94)