# 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`  
**Follow-up:** Code review suggestions addressed (module/alignment docs, plan Bruno note, `TryNormalizeKnown` null guard + test).

## 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; Bruno deny smokes noted in reconciliation. |
| [`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) | **Matches** — Status cites NEO-79 landed; freeze box updated. |
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E5.M1 note includes NEO-79 registry + hotbar/cast migration. |
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E5.M1 row includes NEO-79 landed + Bruno smokes. |
| [`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. |

## 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.~~ **Done.** Status + freeze box updated.

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).~~ **Done.**

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.~~ **Done.** Tests + reconciliation sections updated.

## 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.~~ **Done.** `string?` parameter, null guard in implementation, XML remark, and unit test.

- Nit: `BuildDefinitionsInIdOrder` materializes a `List<AbilityDefRow>` 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`.