don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-17-NEO-51.md

4.3 KiB
Raw Blame History

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 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 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 Matches — server load duplicates CI item rules (schema, duplicates, Slice 1 gate).
docs/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 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 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

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):

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.