67 lines
4.6 KiB
Markdown
67 lines
4.6 KiB
Markdown
# 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.
|