neon-sprawl/docs/reviews/2026-05-24-NEO-79.md

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	Matches — kickoff decisions, scope, acceptance checklist, reconciliation; all AC checked; Bruno deny smokes noted in reconciliation.
docs/plans/E5M1-prototype-backlog.md · E5M1-03	Matches — AC checkboxes + landed note with plan link.
docs/decomposition/modules/E5_M1_CombatRulesEngine.md	Matches — Status cites NEO-79 landed; freeze box updated.
docs/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	Matches — E5.M1 row includes NEO-79 landed + Bruno smokes.
docs/decomposition/modules/client_server_authority.md	N/A — server-only DI; no client or combat resolution changes.
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

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.