NEO-52: Add code review for item definition registry + DI.

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<IItemDefinitionRegistry>()`** 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.