diff --git a/docs/reviews/2026-05-24-NEO-78.md b/docs/reviews/2026-05-24-NEO-78.md new file mode 100644 index 0000000..aa18f6b --- /dev/null +++ b/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.