diff --git a/docs/reviews/2026-05-24-NEO-79.md b/docs/reviews/2026-05-24-NEO-79.md new file mode 100644 index 0000000..fd5d7e6 --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-79.md @@ -0,0 +1,63 @@ +# Code review — NEO-79 `IAbilityDefinitionRegistry` + DI + +**Date:** 2026-05-24 +**Scope:** Branch `NEO-79-iabilitydefinitionregistry-di` · commits `8bafec9`–`b9644a0` vs `origin/main` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-79 delivers **E5M1-03**: a thin `IAbilityDefinitionRegistry` adapter over the NEO-77 `AbilityDefinitionCatalog`, registered in DI via `AddAbilityDefinitionCatalog`, with `TryGetDefinition`, `TryNormalizeKnown`, and `GetDefinitionsInIdOrder`. Hotbar loadout and ability cast routes now inject the registry instead of the static `PrototypeAbilityRegistry` allowlist; `PrototypeAbilityRegistry` is reduced to id constants for tests. Ten new AAA unit/host tests cover lookup, normalization, enumeration, loader fixture parity, and DI resolution. Existing hotbar/cast API regression tests remain green. Bruno adds two HTTP deny smoke requests for `unknown_ability`. Implementation matches the kickoff plan and mirrors the NEO-67 recipe registry pattern. Residual risk is low — behavior-preserving internal migration; module tracking docs should catch up on merge. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-79-implementation-plan.md`](../plans/NEO-79-implementation-plan.md) | **Matches** — kickoff decisions, scope, acceptance checklist, reconciliation; all AC checked. | +| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-03** | **Matches** — AC checkboxes + landed note with plan link. | +| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Partially matches** — freeze box and NEO-79 reference are correct; **Status** line still reads “E5M1-03+ pending” (should note NEO-79 landed). | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E5.M1 **In Progress** is correct; **E5.M1 note** cites NEO-76/77 only — add NEO-79 registry landed. | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Partially matches** — E5.M1 row lists NEO-76/77; should add NEO-79 registry + hotbar/cast migration. | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — server-only DI; no client or combat resolution changes. | +| [`server/README.md`](../../server/README.md) | **Matches** — ability catalog section updated for registry-first lookup and hotbar/cast validation. | +| Full-stack epic decomposition | **Matches** — plan documents no client counterpart; server-only story. | +| Manual QA | **N/A** — plan skip rationale holds; Bruno deny smokes are a useful bonus. | + +Register/tracking: update E5.M1 module page, dependency register note, and alignment table after merge. + +## Blocking issues + +None. + +## Suggestions + +1. **Module doc status line** — In `docs/decomposition/modules/E5_M1_CombatRulesEngine.md`, change the **Status** row from “E5M1-03+ pending” to cite **E5M1-03 NEO-79 landed** (registry + hotbar/cast migration), matching the E5M1 backlog landed note. + +2. **Dependency register + alignment table** — Extend the **E5.M1 note** in `module_dependency_register.md` and the E5.M1 row in `documentation_and_implementation_alignment.md` with **NEO-79 landed:** `IAbilityDefinitionRegistry` + DI; hotbar/cast use `TryNormalizeKnown` (same pattern as NEO-77 reconciliation). + +3. **Plan reconciliation vs Bruno** — `NEO-79-implementation-plan.md` says “No Bruno or manual QA”; the branch adds two Bruno deny requests under `ability-cast/` and `hotbar-loadout/`. Either add a one-line reconciliation note (“optional Bruno deny smokes added”) or leave as-is — the requests are valuable HTTP regression coverage. + +## Nits + +- Nit: `TryGetDefinition` has no explicit test for case-variant ids returning false (normalization is covered on `TryNormalizeKnown` only). Acceptable given the plan’s split between exact lookup and normalize-for-validation. + +- Nit: `TryNormalizeKnown` would throw on `null` input (same as the removed static allowlist); call sites guard with `string.IsNullOrWhiteSpace` first — document in XML if future callers might skip that guard. + +- Nit: `BuildDefinitionsInIdOrder` materializes a `List` at construction — same pattern as `RecipeDefinitionRegistry`; fine for four prototype rows. + +## Verification + +```bash +cd /home/don/neon-sprawl/server +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \ + --filter "FullyQualifiedName~AbilityDefinitionRegistryTests|FullyQualifiedName~HotbarLoadoutApiTests|FullyQualifiedName~AbilityCastApiTests" +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj +``` + +Manual (optional): + +- Bruno: `bruno/neon-sprawl-server/hotbar-loadout/Post loadout unknown ability deny.bru` → 200, `updated: false`, `reasonCode: unknown_ability`. +- Bruno: `bruno/neon-sprawl-server/ability-cast/Post cast unknown ability deny.bru` → 200, `accepted: false`, `reasonCode: unknown_ability`.