diff --git a/docs/reviews/2026-05-28-NEO-94.md b/docs/reviews/2026-05-28-NEO-94.md new file mode 100644 index 0000000..b9bd90a --- /dev/null +++ b/docs/reviews/2026-05-28-NEO-94.md @@ -0,0 +1,64 @@ +# 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` + +## Verdict + +**Approve with nits** + +## 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` | **Partially matches** — E5.M2 row still ends at **NEO-93**; should add **NEO-94 landed** note per plan. | +| `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. + +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(); + ``` + +## 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. + +## 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.