NEO-90: Add code review for npc-behavior-definitions GET branch.

docs/reviews/2026-05-25-NEO-90.md

@@ -0,0 +1,60 @@
+# Code review — NEO-90 GET `/game/world/npc-behavior-definitions`
+
+**Date:** 2026-05-25  
+**Scope:** Branch `NEO-90-npc-behavior-definitions-get` · commits `c757b7a`–`d3e765f` vs `origin/main`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-90 delivers **E5M2-04**: a read-only **`GET /game/world/npc-behavior-definitions`** projection backed solely by **`INpcBehaviorDefinitionRegistry`**, mirroring the NEO-78 ability-definitions pattern. Implementation adds `NpcBehaviorDefinitionsWorldApi`, versioned DTOs (`schemaVersion` **1**, **`npcBehaviors`**), Bruno smokes, integration test, and reconciled plan/backlog/module/alignment docs. The HTTP layer maps all nine schema fields from `GetDefinitionsInIdOrder()` without duplicating catalog truth. One new AAA integration test passes; **29** Npc-filter tests green. No runtime, instance registry, or client surface — appropriately scoped for a server-only HTTP read model ahead of NEO-97/NEO-98. Risk is low.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| [`docs/plans/NEO-90-implementation-plan.md`](../plans/NEO-90-implementation-plan.md) | **Matches** — kickoff decisions (`npcBehaviors` key, no manual QA), scope, acceptance checklist, reconciliation section. |
+| [`docs/plans/E5M2-prototype-backlog.md`](../plans/E5M2-prototype-backlog.md) · **E5M2-04** | **Matches** — AC checkboxes + landed note. |
+| [`docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md) | **Partially matches** — Status, Related slices, and NEO-90 implementation blurb updated; **Linear** summary row (line 11) still jumps NEO-89 → NEO-98 without E5M2-04/NEO-90 (see Suggestion 2). |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) · **E5.M2** | **Matches** — NEO-90 landed note with README + Bruno links. |
+| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) · **E5.M2 note** | **Partially matches** — note ends at NEO-89; missing NEO-90 HTTP projection (see Suggestion 1). |
+| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — read-only world definition GET; no runtime state or client mutation. |
+| [`server/README.md`](../../server/README.md) | **Matches** — NPC behavior definitions section with curl example and cross-links. |
+| [`server/NeonSprawl.Server/Game/Combat/AbilityDefinitionsWorldApi.cs`](../../server/NeonSprawl.Server/Game/Combat/AbilityDefinitionsWorldApi.cs) (pattern reference) | **Matches** — structurally identical registry → DTO → `Results.Json` mapping. |
+| Full-stack epic decomposition | **Matches** — server-only; kickoff defers Godot labels to NEO-97/NEO-98; no false prototype-complete claim. |
+| Manual QA | **N/A** — plan documents skip: Bruno + integration tests only. |
+
+Register/tracking: alignment table E5.M2 **In Progress** with NEO-90 note is correct; module register **E5.M2 note** should be extended (Suggestion 1).
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Update `module_dependency_register.md` E5.M2 note** — Append **NEO-90 landed:** **`GET /game/world/npc-behavior-definitions`** — `NpcBehaviorDefinitionsWorldApi` + DTOs in `Game/Npc/` with link to [NEO-90 plan](../plans/NEO-90-implementation-plan.md), [server README — NPC behavior definitions (NEO-90)](../../server/README.md#npc-behavior-definitions-neo-90), and Bruno `bruno/neon-sprawl-server/npc-behavior-definitions/` (same pattern as E5.M1 note after NEO-78).
+
+2. **E5.M2 Linear summary row** — In [`E5_M2_NpcAiAndBehaviorProfiles.md`](../decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md), extend the **Linear** field (Summary table line 11) to cite **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) so it matches the updated **Status** row and the decomposed backlog table below.
+
+## Nits
+
+- Nit: Neither C# nor Bruno tests assert **`prototype_ranged_control`** row metadata — acceptable per plan (melee + elite spot-checks only); optional ranged row assertion if NEO-97 client labels want a third anchor.
+
+- Nit: **`FrozenThreeInIdOrder`** in tests uses **`PrototypeNpcBehaviorRegistry`** constants (good); Bruno hard-codes string literals — same established split as ability-definitions Bruno vs C# tests.
+
+- Nit: Freeze **Rules** paragraph cites registry injection (NEO-89) but not the NEO-90 GET endpoint by name — the new NEO-90 blurb below covers it; optional one-line cross-link in Rules for skimmers.
+
+## Verification
+
+```bash
+cd /home/don/neon-sprawl/server
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~NpcBehaviorDefinitions"
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~NpcBehavior"
+```
+
+Confirmed during review: **1** NpcBehaviorDefinitions test passed; **29** Npc-filter tests passed.
+
+Optional: run Bruno `Get npc behavior definitions.bru` against a dev server (`curl` example in `server/README.md`).