NEO-58: add code review for resource-node catalog load

pull/93/head
VinPropane 2026-05-24 14:03:19 -04:00
parent fdaf5b7231
commit 6656853473
1 changed files with 57 additions and 0 deletions

View File

@ -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<ResourceNodeCatalog>()` 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`.