From 1c9c93513e81bc65b791b8b171046790cb59a4c9 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 17 May 2026 22:01:47 -0400 Subject: [PATCH] NEO-51: Add code review for item catalog fail-fast load. --- docs/reviews/2026-05-17-NEO-51.md | 58 +++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 docs/reviews/2026-05-17-NEO-51.md diff --git a/docs/reviews/2026-05-17-NEO-51.md b/docs/reviews/2026-05-17-NEO-51.md new file mode 100644 index 0000000..fe9b4a7 --- /dev/null +++ b/docs/reviews/2026-05-17-NEO-51.md @@ -0,0 +1,58 @@ +# 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** + +## 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. + +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. + +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. + +## Nits + +- Nit: `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid` creates a temp directory and does not delete it (minor test hygiene). +- 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.