neon-sprawl/docs/reviews/2026-05-25-NEO-89.md

Code review — NEO-89 injectable INpcBehaviorDefinitionRegistry + DI

Date: 2026-05-25
Scope: Branch NEO-89-inpcbehaviordefinitionregistry-di · commits 7e4e0ca–e25b140 vs origin/main
Base: origin/main

Verdict

Approve with nits

Summary

NEO-89 delivers E5M2-03: a thin INpcBehaviorDefinitionRegistry adapter over the NEO-88 startup-loaded NpcBehaviorDefinitionCatalog, registered in AddNpcBehaviorDefinitionCatalog alongside the catalog singleton. The implementation mirrors AbilityDefinitionRegistry (NEO-79): exact-id TryGetDefinition, trim/lowercase TryNormalizeKnown, precomputed GetDefinitionsInIdOrder, and PrototypeNpcBehaviorRegistry id constants for tests. Eleven new AAA unit/host tests pass; 27 Npc-filter tests green including loader regression. Docs (plan, E5M2 backlog E5M2-03, alignment register, module register, E5.M2 status, server/README.md) are reconciled. No HTTP, runtime, or client surface — appropriately scoped for a DI-only story. Risk is low: read-only adapter with no behavior change to catalog load semantics.

Documentation checked

Document	Result
docs/plans/NEO-89-implementation-plan.md	Matches — kickoff decisions, scope, acceptance checklist, reconciliation (11 tests, DI pattern, no Program.cs eager registry resolve).
docs/plans/E5M2-prototype-backlog.md · E5M2-03	Matches — AC checkboxes + landed note.
docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md	Matches — Status/Linear rows cite NEO-89; freeze Rules + NEO-88/NEO-89 implementation blurbs (review follow-up).
docs/decomposition/modules/documentation_and_implementation_alignment.md · E5.M2	Matches — NEO-89 landed note added.
docs/decomposition/modules/module_dependency_register.md	Matches — E5.M2 note cites NEO-89 registry + DI.
docs/decomposition/modules/client_server_authority.md	N/A — server-only read registry; no runtime or client mutation.
server/README.md	Matches — registry methods documented; catalog reserved for fail-fast boot; PrototypeNpcBehaviorRegistry noted.
server/NeonSprawl.Server/Game/Combat/AbilityDefinitionRegistry.cs (pattern reference)	Matches — NPC registry is structurally identical to NEO-79 precedent.
Full-stack epic decomposition	Matches — E5M2-03 is server-only; kickoff defers player-visible HTTP to NEO-90 and Godot to NEO-98; no false prototype-complete claim.
Manual QA	N/A — plan documents skip: automated unit/host tests only; Bruno proof deferred to NEO-90.

Register/tracking: alignment table and module register updates are correct; E5.M2 remains In Progress toward NEO-90+.

Blocking issues

None.

Suggestions

1. E5.M2 freeze Rules paragraph — In E5_M2_NpcAiAndBehaviorProfiles.md, extend the Rules sentence to cite INpcBehaviorDefinitionRegistry + NEO-89 as the preferred game lookup path (mirror E5.M1 freeze rules citing IAbilityDefinitionRegistry / NEO-79 after NEO-77 server load). Done.

2. Optional E5.M2 implementation blurb — Add a short “NPC behavior definition registry (NEO-89)” paragraph under the module doc (same style as E3.M3’s IItemDefinitionRegistry section) so future NEO-90+ stories have a single anchor for the contract without rereading the plan. Done.

Nits

• Nit: No dedicated unit test that TryGetDefinition rejects case-variant ids (e.g. "Prototype_Melee_Pressure") — fail-closed by design and covered indirectly via TryNormalizeKnown; optional explicit test if NEO-90 HTTP validation wants to document the split. Done.

• Nit: TryGetDefinition for prototype_ranged_control metadata is only asserted in the loader-fixture and host tests, not a standalone unit test like melee/elite — acceptable given loader coverage.

• Nit: Inline MeleeRow/RangedRow/EliteRow test helpers duplicate freeze-box numbers — same established pattern as ability registry tests.

Verification

cd /home/don/neon-sprawl/server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~NpcBehavior"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

Confirmed during review: 27 Npc-filter tests passed.