diff --git a/docs/reviews/2026-05-31-NEO-101.md b/docs/reviews/2026-05-31-NEO-101.md new file mode 100644 index 0000000..cc1a2ec --- /dev/null +++ b/docs/reviews/2026-05-31-NEO-101.md @@ -0,0 +1,57 @@ +# Code review — NEO-101 (E5M3-02) + +**Date:** 2026-05-31 +**Scope:** Branch `NEO-101-encounter-reward-catalog-load-fail-fast` vs `7c0e2e8` (merge-base on `main`) — commits `06c5899` … `f921095` +**Base:** `7c0e2e8` (main at branch point) + +## Verdict + +**Approve with nits** + +## Summary + +NEO-101 adds fail-fast startup loading for **`content/reward-tables/*_reward_tables.json`** and **`content/encounters/*_encounters.json`**, mirroring the established NEO-51 / NEO-66 / NEO-88 catalog pattern: path discovery, JsonSchema.Net row validation, duplicate-id rejection, prototype E5M3 allowlist gates, and cross-refs (item ids → item catalog; NPC instance sets → `PrototypeNpcRegistry`; `rewardTableId` → reward-table catalog). DI registers reward tables after items and encounters after reward tables; `Program.cs` eagerly resolves both singletons before `Run()`. Nineteen AAA loader + host boot tests pass locally. Docs (`server/README.md`, alignment register, module register, E5M3 backlog) are updated. Risk is low: server-only, no HTTP surface, no registry interfaces yet (correctly deferred to NEO-102). + +## Documentation checked + +| Path | Result | +|------|--------| +| `docs/plans/NEO-101-implementation-plan.md` | **Matches** — shipped loaders, DI order, config keys, tests, README, alignment register; acceptance checklist checked; reconciliation section accurate. | +| `docs/plans/E5M3-prototype-backlog.md` (E5M3-02) | **Matches** — landed note cites NEO-101 plan and 19 tests. | +| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Partially matches** — freeze table and CI paragraph reference NEO-101 loader sync; Summary **Status** still generic “In Progress — Slice 3 backlog” without an explicit **NEO-101 server load landed** bullet (alignment register was updated; module page could mirror). | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-101** server load; runtime NEO-102+ still outstanding. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-101 landed** + README anchor. | +| `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | **N/A** — no server-load paragraph required for this story; CI gates unchanged. | +| Full-stack epic decomposition | **N/A** — plan explicitly excludes client counterpart until NEO-110 / NEO-111. | +| `server/README.md` | **Matches** — reward-table and encounter catalog sections document config keys, discovery, fail-fast, load order, NEO-102 deferral. | + +## Blocking issues + +None. + +## Suggestions + +1. **Add `Load_ShouldThrow_WhenRowViolatesSchema` tests** — The implementation plan test table lists **schema violation** for both loaders; peer catalog tests (`ItemDefinitionCatalogLoaderTests`, `RecipeDefinitionCatalogLoaderTests`, etc.) use empty `displayName` or similar. Current negative coverage uses wrong top-level shape / `schemaVersion` / cross-refs but not a row-level JsonSchema failure. Add one case per loader (e.g. empty `displayName` on a otherwise-valid prototype row) to lock schema messaging and match precedent. + +2. **Optional module page snapshot** — Add a short **NEO-101 landed** bullet under `E5_M3_EncounterAndRewardTables.md` (or an **Implementation snapshot** paragraph) pointing at `server/NeonSprawl.Server/Game/Encounters/` and the README sections, so the module page stays aligned with the register without opening the alignment doc. + +## Nits + +- Nit: Host boot failure test (`Host_ShouldFailStartup_WhenEncountersDirectoryInvalid`) covers empty encounters dir only; a symmetric empty **reward-tables** dir case would mirror NEO-88 completeness but is not required by the plan. + +- Nit: `AddEncounterAndRewardCatalogs` re-binds `ContentPathsOptions` (same as other catalog extensions); harmless but duplicates registration if consolidated later. + +## Verification + +```bash +# NEO-101-focused (19 tests — all passed locally) +dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Encounters" + +# CI parity +python3 scripts/validate_content.py + +# Full server suite (Postgres integration tests need Docker locally) +dotnet test NeonSprawl.sln +``` + +**Local run:** 19/19 encounter-catalog tests passed. Full suite: 519 passed, 11 failed — all Postgres/docker-compose harness failures unrelated to this diff.