64 lines
5.1 KiB
Markdown
64 lines
5.1 KiB
Markdown
# 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<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`.
|