diff --git a/docs/reviews/2026-05-25-NEO-90.md b/docs/reviews/2026-05-25-NEO-90.md new file mode 100644 index 0000000..0419882 --- /dev/null +++ b/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`).