From 6656853473ce9b995633fbabbdf554e91c622ee2 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 14:03:19 -0400 Subject: [PATCH] NEO-58: add code review for resource-node catalog load --- docs/reviews/2026-05-24-NEO-58.md | 57 +++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 docs/reviews/2026-05-24-NEO-58.md diff --git a/docs/reviews/2026-05-24-NEO-58.md b/docs/reviews/2026-05-24-NEO-58.md new file mode 100644 index 0000000..443e9ce --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-58.md @@ -0,0 +1,57 @@ +# Code review — NEO-58 server resource-node catalog load (fail-fast) + +**Date:** 2026-05-24 +**Scope:** Branch `NEO-58-e3m1-server-resource-node-catalog-load-fail-fast` · commits `2de699b`–`fdaf5b7` vs `main` (plan + implementation + rebase note) +**Base:** `main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-58 adds fail-fast startup loading for `content/resource-nodes/*_resource_nodes.json` and `*_resource_yields.json` under `Game/Gathering/`, mirroring the NEO-51 item-catalog pattern: path discovery via `ContentPathsOptions`, in-process JSON Schema validation, duplicate `nodeDefId` detection, prototype Slice 2 gates (`PrototypeSlice2ResourceNodeCatalogRules` synced with `scripts/validate_content.py`), and yield `itemId` cross-check against `ItemDefinitionCatalog`. DI registers `ResourceNodeCatalog` after items; `Program.cs` eagerly resolves it at boot. Seventeen focused loader/host tests pass with full AAA. Docs (implementation plan, E3M1 backlog, alignment table, server README) match the landed behavior. Risk is low; main drift surface is duplicated Slice 2 constants in C# vs Python (documented, same as NEO-50/NEO-57). + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-58-implementation-plan.md`](../plans/NEO-58-implementation-plan.md) | **Matches** — scope, acceptance checklist, technical approach, and file list align with the diff; checklist marked complete. | +| [`docs/plans/E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) | **Matches** — E3M1-02 acceptance flipped with NEO-58 landed note. | +| [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Matches** — content contracts and Slice 2 freeze; server load is the planned E3M1-02 step (registry/depletion still open). | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-58 server loader + README link. | +| [`docs/plans/NEO-57-implementation-plan.md`](../plans/NEO-57-implementation-plan.md) | **Matches** — C# loader enforces the same gates CI added in NEO-57. | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — no runtime gather authority change; startup validation only. | +| [`docs/decomposition/modules/CT_M1_ContentValidationPipeline.md`](../decomposition/modules/CT_M1_ContentValidationPipeline.md) | **Matches** — PR gate remains `validate_content.py`; server tests guard C#/Python Slice 2 drift. | + +Register/tracking: alignment table updated; no further register change required beyond this row. + +## Blocking issues + +None. + +## Suggestions + +1. **Plan vs Bruno** — [`NEO-58-implementation-plan.md`](../plans/NEO-58-implementation-plan.md) lists Bruno as out of scope, but `bruno/neon-sprawl-server/resource-node-catalog/` adds a health smoke request. Either add a one-line note under **Tests** (“optional Bruno smoke”) or drop the folder to keep plan scope literal; the request itself is fine for manual boot verification. + +2. **Explicit empty-catalog file test (optional)** — `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid` covers an empty directory; a loader unit test asserting `no *_resource_nodes.json files` (and yields) would mirror item-catalog negative coverage and make failure messages grep-friendly in CI logs. + +## Nits + +- Nit: Slice 2 lens gate checks **set** equality (same as Python/NEO-57); duplicate `gatherLens` on two nodes still fails via set mismatch — acceptable for prototype. + +- Nit: `Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds` uses `/health` in **Act** (NEO-51 item-catalog host test does the same); strict AAA would move the HTTP call to **Arrange** and keep **Act** as `GetRequiredService()` only. + +- Nit: No `docs/manual-qa/NEO-58.md` — consistent with plan (server boot-only, no user-facing HTTP for resource nodes); Bruno health folder is sufficient smoke. + +- Nit: C#/Python constant drift remains a maintenance coupling; sync comments on `PrototypeSlice2ResourceNodeCatalogRules` are present (NEO-50/NEO-57 precedent). + +## Verification + +```bash +cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~ResourceNodeCatalog" +cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj +python3 scripts/validate_content.py +``` + +Manual: start server from repo root (content discovery); confirm Information log with node/yield counts; point `Content__ResourceNodesDirectory` at an empty dir and confirm process refuses boot with `Resource node catalog validation failed`.