# Code review — NEO-94 (E5M2-08)

**Date:** 2026-05-28  
**Scope:** Branch `NEO-94-npc-runtime-snapshot-get` vs `origin/main` — full rewrite in `a68d8aa` (+ plan `5442910`)  
**Base:** `origin/main`

**Follow-up:** Suggestions addressed in `798c601`; re-verified 2026-05-28 (NEO-94 tests 4/4 pass).

## Verdict

**Approve**

## Summary

The rewritten branch delivers E5M2-08 cleanly: **`GET /game/world/npc-runtime-snapshot`** invokes **`NpcRuntimeOperations.AdvanceAll`** on poll, then projects all three **`PrototypeNpcRegistry`** rows with **`behaviorDefId`**, **`state`**, **`aggroHolderPlayerId`**, and optional nested **`activeTelegraph`** (including server-computed **`windupRemainingSeconds`**). The implementation mirrors **`CooldownSnapshotApi`** / **`CombatTargetsWorldApi`** conventions — `internal static BuildSnapshot`, fail-fast registry lookups, versioned envelope with **`serverTimeUtc`**, route registered in **`Program.cs`**. Four AAA integration tests cover idle baseline, cast→aggro, advance→telegraph, and decreasing windup; Bruno adds happy-path + telegraph smoke with documented **`sleep(3100)`**. Build succeeds; NEO-94-filtered tests pass (**4/4**). **`NpcRuntimeStateSnapshot`** is unchanged — projection reads from registry + threat store at HTTP boundary, as planned.

## Documentation checked

| Path | Result |
|------|--------|
| `docs/plans/NEO-94-implementation-plan.md` | **Matches** — AC checkboxes marked done; handler flow, wire shape, lazy tick, and file list align with the diff. |
| `docs/plans/E5M2-prototype-backlog.md` (E5M2-08) | **Matches** — AC checked, landed note present. |
| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Matches** — E5M2-08 landed in status table; NEO-94 HTTP section + README/Bruno cross-links. |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M2 row includes **NEO-94 landed** (snapshot GET, Bruno folder, plan/README links). |
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — server-owned runtime + telegraph timing; client poll-only (NEO-97 deferred). |
| `server/README.md` | **Matches** — threat/runtime HTTP read rows updated; dedicated NEO-94 section with field table and curl. |

## Blocking issues

None.

## Suggestions

1. ~~**Update `documentation_and_implementation_alignment.md`** — Append **NEO-94 landed** to the E5.M2 tracking row (snapshot GET, Bruno folder, plan/README links), matching NEO-90–NEO-93 entries. Listed in the plan “Files to modify” but not yet in the diff.~~ **Done.** (`798c601`)

2. ~~**AAA layout in two integration tests** — `GetNpcRuntimeSnapshot_ShouldShowAggro_OnMeleeAfterDamagingCast` and `GetNpcRuntimeSnapshot_ShouldShowTelegraph_AfterAttackCooldownElapses` call **`ReadFromJsonAsync`** in **Act**. Move status/body reads to **Assert** to match **`CombatTargetsWorldApiTests`** and [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert):

   ```csharp
   // Act
   var response = await client.GetAsync("/game/world/npc-runtime-snapshot");
   // Assert
   Assert.Equal(HttpStatusCode.OK, response.StatusCode);
   var body = await response.Content.ReadFromJsonAsync<NpcRuntimeSnapshotResponse>();
   ```~~ **Done.** Aggro + telegraph tests fixed; decreasing-windup test also moves all **`ReadFromJsonAsync`** calls to **Assert** (`798c601`).

## Nits

- Nit: **`ActiveTelegraph`** / **`AggroHolderPlayerId`** could use **`[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]`** for leaner JSON (optional; explicit `null` is fine for client parsers).
- ~~Nit: **`GetNpcRuntimeSnapshot_ShouldDecreaseWindupRemaining_AcrossPolls`** captures **`firstBody`** during **Arrange** (baseline for comparison). Acceptable for a two-poll delta test; alternatively split into two facts for stricter AAA.~~ **Done.** **`ReadFromJsonAsync`** for both polls now in **Assert**; first poll GET remains in **Arrange** as setup (`798c601`).

## Verification

```bash
# Build + NEO-94 tests (passed 2026-05-28)
dotnet build NeonSprawl.sln
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
  --filter "FullyQualifiedName~NpcRuntimeSnapshot"

# Full server suite (Postgres persistence tests require local DB; 12 env failures unrelated to NEO-94)
dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

# Bruno (CI or local dev server)
# bruno/neon-sprawl-server/npc-runtime-snapshot/
```

Manual: run **`Get snapshot after cast telegraph.bru`** against dev server — melee should reach **`telegraph_windup`** with **`activeTelegraph.windupRemainingSeconds > 0`** after the 3.1 s wait.