diff --git a/docs/reviews/2026-05-23-NEO-52.md b/docs/reviews/2026-05-23-NEO-52.md new file mode 100644 index 0000000..50eb8e0 --- /dev/null +++ b/docs/reviews/2026-05-23-NEO-52.md @@ -0,0 +1,54 @@ +# Code review — NEO-52 item definition registry + DI + +**Date:** 2026-05-23 +**Scope:** Branch `NEO-52-item-definition-registry-di` · commits `1e9f1e9`–`792b38d` vs `origin/main` +**Base:** `origin/main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-52 adds **`IItemDefinitionRegistry`** and **`ItemDefinitionRegistry`** as a thin adapter over the NEO-51 **`ItemDefinitionCatalog`**, registers both in **`AddItemDefinitionCatalog`**, and ships six AAA unit tests plus a host DI smoke test mirroring **`SkillDefinitionRegistry`** (NEO-35). Kickoff decisions (`TryGetDefinition`, `GetDefinitionsInIdOrder`, no extra eager resolve in `Program.cs`, no re-validation) are implemented as specified. Risk is low: no HTTP, persistence, or gameplay callers yet; enumeration allocates per call at prototype scale (same as skills). Tests pass locally. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-52-implementation-plan.md`](../plans/NEO-52-implementation-plan.md) | **Matches** — interface surface, adapter, DI, README, alignment table, test matrix; acceptance checklist complete. | +| [`docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Partially matches** — frozen roster and NEO-51 server load documented; **no NEO-52 registry bullet** yet (see suggestion). | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M3 row notes NEO-52 landed with plan link. | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M3 **In Progress** is correct; footnote names NEO-50/NEO-51 only (optional NEO-52 mention). | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — read-only definition registry; no client trust boundary change. | +| [`server/README.md`](../../server/README.md) | **Matches** — item catalog section points game code at **`IItemDefinitionRegistry`**. | + +Register/tracking: **E3.M3** row update in alignment doc is appropriate; no module **Status** promotion required until HTTP/inventory slices land. + +## Blocking issues + +None. + +## Suggestions + +1. **E3.M3 module page — NEO-52 registry** — Under **Related implementation slices**, add a short bullet parallel to **Server load (NEO-51)** documenting **`IItemDefinitionRegistry`** in `Game/Items/` and that NEO-53+ / inventory callers should inject the interface (link [NEO-52 plan](../plans/NEO-52-implementation-plan.md)). Readers who open the module doc before the alignment register will otherwise only see catalog load. + +2. **Dependency register footnote (optional)** — Extend the **E3.M3 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to mention **NEO-52** alongside NEO-50/NEO-51 so the register matches the alignment table. + +## Nits + +- Nit: **`Host_ShouldResolveRegistryFromDi_WhenStartupSucceeds`** places **`TryGetDefinition`** in **Act** (metadata assertions in **Assert**). Same pattern as **`SkillDefinitionRegistryTests`**; strictly, only **`GetRequiredService()`** belongs in Act — optional tidy for both registries later, not blocking. + +- Nit: **`GetDefinitionsInIdOrder`** rebuilds a sorted list on every call (keys sort + list materialization). Acceptable for six prototype rows and consistent with **`SkillDefinitionRegistry`**; revisit only if callers poll in hot paths. + +- Nit: **`ItemDefinitionRegistry.TryGetDefinition`** duplicates **`ItemDefinitionCatalog.ById`** lookup rather than delegating to **`TryGetItem`** — intentional per plan (game surface vs catalog helper); **`TryGetItem`** does not guard **`null`** ids while the registry does (good for public API). + +## Verification + +```bash +cd server +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~ItemDefinitionRegistryTests" +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj +``` + +Manual QA: not required for this story (no user-visible API); NEO-53 will exercise enumeration via HTTP.