NEO-78: Add code review for ability-definitions GET story.

docs/reviews/2026-05-24-NEO-78.md

@@ -0,0 +1,61 @@
+# Code review — NEO-78 GET world ability-definitions
+
+**Date:** 2026-05-24  
+**Scope:** Branch `NEO-78-get-world-ability-definitions` · commits `374b4b5`–`44e0056` vs `origin/main`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-78 delivers **E5M1-04**: a read-only **`GET /game/world/ability-definitions`** endpoint backed by **`IAbilityDefinitionRegistry.GetDefinitionsInIdOrder()`**, with versioned DTOs (`schemaVersion` **1**, **`abilities`** array), row projection (`id`, `displayName`, `baseDamage`, `cooldownSeconds`, optional **`abilityKind`** omitted when null), Bruno coverage, and an AAA integration test asserting the frozen-four id order plus **`prototype_pulse`** / **`prototype_burst`** spot-checks. Implementation closely mirrors the landed NEO-68 recipe-definitions and NEO-53 item-definitions patterns; no auth, no second catalog source, no client wiring (deferred to NEO-85). Residual risk is low — additive HTTP read model on existing registry truth.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| [`docs/plans/NEO-78-implementation-plan.md`](../plans/NEO-78-implementation-plan.md) | **Matches** — kickoff decisions (no manual QA, omit null `abilityKind`, registry-only injection); acceptance checklist complete; reconciliation section reflects landed files. |
+| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-04** | **Matches** — AC checkboxes checked; landed note with plan + Bruno links. In-scope line still mentions “manual QA stub if needed” (kickoff override documented in plan only). |
+| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Matches** — Status cites NEO-78 landed; **Related implementation slices** HTTP bullet added; freeze table ids align with test assertions. |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E5.M1 row includes NEO-78 HTTP projection + Bruno folder. |
+| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E5.M1 note still ends at **NEO-79 landed**; does not yet cite NEO-78 GET (see suggestion). |
+| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — read-only world catalog; server source of truth; no client mutation in this story. |
+| NEO-68 / NEO-53 reference implementations | **Matches** — `AbilityDefinitionsWorldApi` + separate DTO file mirror `RecipeDefinitionsWorldApi` / `ItemDefinitionsWorldApi`; integration test shape matches `RecipeDefinitionsWorldApiTests`. |
+| Full-stack epic decomposition | **Matches** — plan documents no client counterpart; NEO-85 optional enrichment; does not claim prototype slice complete without Godot. |
+| [`server/README.md`](../../server/README.md) | **Matches** — new **Ability definitions (NEO-78)** section with curl example and plan/Bruno links. |
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Dependency register E5.M1 note** — Extend the **E5.M1 note** in `docs/decomposition/modules/module_dependency_register.md` with **NEO-78 landed:** `GET /game/world/ability-definitions` — `AbilityDefinitionsWorldApi` + DTOs; Bruno `ability-definitions/` (same pattern as NEO-79 review follow-up for registry DI).
+
+2. **E5M1 backlog in-scope line** — In **E5M1-04** “In scope”, consider replacing “manual QA stub if needed” with “no manual QA (server-only per NEO-78 kickoff)” so the backlog does not contradict the plan decision.
+
+## Nits
+
+- Nit: Bruno test **“frozen prototype four is present”** is redundant given **“frozen prototype four matches registry id order”** already asserts the exact four ids.
+
+- Nit: C# integration test infers count **4** via `FrozenFourInIdOrder` length only; an explicit `Assert.Equal(4, body.Abilities.Count)` would match Bruno’s explicit length check (optional — recipe test uses the same frozen-array pattern).
+
+- Nit: Spot-checks cover **`prototype_pulse`** and **`prototype_burst`** only; **`prototype_guard`** (`utility`) and **`prototype_dash`** (`movement`) kinds are untested in automated/Bruno paths — acceptable per plan scope but easy to add later if desired.
+
+## Verification
+
+```bash
+cd /home/don/neon-sprawl/server
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~AbilityDefinitionsWorldApiTests|FullyQualifiedName~AbilityDefinitionRegistryTests"
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~Combat"
+```
+
+Manual (optional):
+
+- Bruno: `bruno/neon-sprawl-server/ability-definitions/Get ability definitions.bru` against a running dev server (`http://localhost:5253`).
+
+**Reviewer note:** Full `dotnet test` reported **32** failures in `GatherOperationsTests` (likely environment/DB-dependent, unrelated to this diff). All **27** Combat-filtered tests and the new ability-definitions integration test passed.