# Code review — NEO-51 item catalog fail-fast load

**Date:** 2026-05-17  
**Scope:** Branch `NEO-51-server-item-catalog-load-fail-fast` (`c2366ee` … `b5674c4` vs `main`)  
**Base:** `main` (post NEO-50 merge)

## Verdict

**Approve with nits**

**Follow-up:** Suggestions 1–3 and the temp-dir nit below are **done** (strikethrough + **Done.**).

## Summary

NEO-51 adds server-side fail-fast loading of `content/items/*_items.json` at startup, mirroring the NEO-34 skill-catalog pattern: JsonSchema.Net row validation, duplicate-`id` rejection, `schemaVersion` 1 enforcement, and a C# prototype Slice 1 gate aligned with `scripts/validate_content.py`. `ItemDefinitionCatalog` is registered as a singleton and eagerly resolved in `Program.cs` before the host listens; tests cover loader happy/failure paths and host boot success/failure. The change is localized, well-tested for the main failure classes, and documentation/plan alignment is strong. Residual risk is low: constants drift vs Python (mitigated by comments) and two plan-listed negative tests not yet written.

## Documentation checked

| Document | Result |
|----------|--------|
| [`docs/plans/NEO-51-implementation-plan.md`](../plans/NEO-51-implementation-plan.md) | **Matches** — loader, path resolution, DI, eager boot, README, alignment table, strict NEO-51/NEO-52 split, in-process validation, `ContentPathsOptions` extension. Acceptance checklist marked complete. |
| [`docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Matches** — frozen six-item roster, roles, slot kinds, and stack rules enforced at load; no inventory/HTTP (out of scope). Module **Status** line still generic “In Progress”; server load not called out on that page (see suggestion). |
| [`docs/decomposition/modules/CT_M1_ContentValidationPipeline.md`](../decomposition/modules/CT_M1_ContentValidationPipeline.md) | **Matches** — server load duplicates CI item rules (schema, duplicates, Slice 1 gate). |
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M3 row updated with NEO-51 landed + README link. |
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M3 still **In Progress** (correct); footnote still says “NEO-50+ moves row” without naming NEO-51 server load (optional doc tweak). |

## Blocking issues

None.

## Suggestions

1. ~~**Test gaps vs plan** — [`NEO-51-implementation-plan.md`](../plans/NEO-51-implementation-plan.md) lists unit tests for **wrong `prototypeRole` set** and **`equip_stub` with `stackMax` ≠ 1**. Coverage today includes incomplete ids and bag slot on equip_stub, but not those two cases. Add focused loader tests (AAA) so a regression in `PrototypeSlice1ItemCatalogRules` cannot slip through on role multiset or stack max alone.~~ **Done.** — `Load_ShouldThrow_WhenSlice1PrototypeRolesDoNotMatch`, `Load_ShouldThrow_WhenEquipStubStackMaxIsNotOne` in `ItemDefinitionCatalogLoaderTests.cs`.

2. ~~**E3.M3 module page** — Add a short “Server load (NEO-51)” bullet under related implementation (or refresh the summary **Status**) pointing at `Game/Items/` and fail-fast boot, parallel to E2.M1’s NEO-34 note in the alignment table. Keeps module doc and alignment doc in sync for readers who skip the register.~~ **Done.** — [E3_M3_ItemizationAndInventorySchema.md](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) **Server load (NEO-51)** bullet; register footnote names NEO-51.

3. ~~**Early-path loader tests (optional)** — Consider tests for missing items directory and missing schema file to lock the first `ThrowIfAny` messages; low priority because host boot test covers empty catalog dir.~~ **Done.** — `Load_ShouldThrow_WhenItemsDirectoryMissing`, `Load_ShouldThrow_WhenSchemaFileMissing`.

## Nits

- ~~Nit: `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid` creates a temp directory and does not delete it (minor test hygiene).~~ **Done.** — `finally` deletes `badDir`.
- Nit: `ItemDefinitionCatalog` copies `byId` into a new `Dictionary` then wraps `ReadOnlyDictionary` — fine for startup immutability; only worth revisiting if catalogs grow large.
- Nit: Slice 1 id error strings use single-quoted lists; Python CI uses `repr()` — behavior equivalent, messages differ cosmetically in logs.

## Verification

```bash
cd server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~ItemDefinitionCatalog"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
```

Manual (optional):

```bash
cd server/NeonSprawl.Server && dotnet run
# GET http://localhost:5253/health — expect 200 after Information log with item count
```

Bruno: `bruno/neon-sprawl-server/item-catalog/Health after item catalog load.bru` after server is up.