59 lines
4.8 KiB
Markdown
59 lines
4.8 KiB
Markdown
# 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`](../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`](../plans/E5M2-prototype-backlog.md) · **E5M2-03** | **Matches** — AC checkboxes + landed note. |
|
||
| [`docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md`](../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`](../decomposition/modules/documentation_and_implementation_alignment.md) · **E5.M2** | **Matches** — NEO-89 landed note added. |
|
||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E5.M2 note cites NEO-89 registry + DI. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — server-only read registry; no runtime or client mutation. |
|
||
| [`server/README.md`](../../server/README.md) | **Matches** — registry methods documented; catalog reserved for fail-fast boot; `PrototypeNpcBehaviorRegistry` noted. |
|
||
| [`server/NeonSprawl.Server/Game/Combat/AbilityDefinitionRegistry.cs`](../../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`](../decomposition/modules/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
|
||
|
||
```bash
|
||
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.
|