From 1b904b594d48c50883390d05801d2b1de4b63a09 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 20 Jun 2026 19:34:37 -0400 Subject: [PATCH] NEO-145: add code review for contract catalog load branch. --- docs/reviews/2026-06-20-NEO-145.md | 68 ++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) create mode 100644 docs/reviews/2026-06-20-NEO-145.md diff --git a/docs/reviews/2026-06-20-NEO-145.md b/docs/reviews/2026-06-20-NEO-145.md new file mode 100644 index 0000000..097787f --- /dev/null +++ b/docs/reviews/2026-06-20-NEO-145.md @@ -0,0 +1,68 @@ +# Code review — NEO-145 (E7M4-02 server contract template catalog load) + +**Date:** 2026-06-20 +**Scope:** Branch `NEO-145-e7m4-02-server-contract-template-catalog-load-fail-fast` — commits `215493f`…`937412e` (3 commits) +**Base:** `main` + +## Verdict + +**Approve with nits** + +## Summary + +NEO-145 delivers fail-fast startup loading of `content/contracts/*_contract_templates.json` with CI-parity gates ported to C# (`PrototypeE7M4ContractCatalogRules`), JSON Schema validation with quest/faction bundle `$ref`s, duplicate-id rejection, and cross-ref checks against encounter, faction, frozen prototype item/skill catalogs, and the E7M2 `mission_reward` allowlist. `IContractTemplateRegistry` + DI registration follow established faction/encounter patterns; `Program.cs` registers and eagerly resolves the catalog after the quest catalog. Tests cover loader happy/negative paths, registry lookups, and host boot via `InMemoryWebApplicationFactory`; `dotnet test` (839) and `validate_content.py` both pass. Risk is low: infrastructure-only, no player-facing HTTP yet; primary failure mode is startup exit with sorted `error:` lines — appropriate for this slice. + +## Documentation checked + +| Document | Alignment | +|----------|-----------| +| `docs/plans/NEO-145-implementation-plan.md` | **Matches** — AC checklist complete; reconciliation section reflects shipped loader, registry, DI, tests, README. | +| `docs/plans/E7M4-pre-production-backlog.md` (E7M4-02) | **Matches** — loader, registry, cross-ref, tests, README in scope; instance store/HTTP/Godot correctly out of scope. | +| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Partially matches** — freeze table and CI (NEO-144) documented; no note that server fail-fast load landed (NEO-145). Plan marked this optional. | +| `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | **Partially matches** — Purpose mentions server-side smoke loading; contract paragraph still NEO-144/CI-only — should add NEO-145 server mirror. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M4 row cites E7M4-01/NEO-144 only; should mention E7M4-02 server catalog load after merge. | +| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E7.M4 In Progress spine unchanged; acceptable until alignment pass, but register note could cite NEO-145. | +| `docs/decomposition/modules/client_server_authority.md` | **N/A** — read-only catalog load; no authority boundary change. | +| Full-stack epic decomposition | **N/A** — infrastructure-only; client counterpart correctly none (NEO-154 capstone). | + +Register/tracking tables were **not** updated for NEO-145 in this branch — **should fix** (non-blocking) per plan §6 optional module note and alignment doc pattern from NEO-144. + +## Blocking issues + +None. + +## Suggestions + +1. **Update decomposition tracking for E7M4-02** — After merge, extend `documentation_and_implementation_alignment.md` E7.M4 row and optionally `module_dependency_register.md` / `E7_M4_ContractMissionGenerator.md` with one line: server fail-fast catalog load + `IContractTemplateRegistry` (NEO-145). Mirrors NEO-144 alignment follow-up. + +2. **CT_M1 server-load paragraph** — Add a sentence under the contract-catalog CI bullet that the host mirrors the same gates at startup via `ContractTemplateCatalogLoader` (NEO-145), matching faction/quest catalog precedent in that doc. + +3. **Band-cap negative through full loader** — Plan §5 lists band-cap violation via loader; shipped coverage tests `TryGetBandCapGateError` in isolation only. Consider one temp-fixture test that writes qty **11** and asserts `ContractTemplateCatalogLoader.Load` throws (same pattern as freeze/roster negatives) so the loader→rules wiring is exercised end-to-end. + +4. **Missing schema path test** — Plan §5 lists missing schema; only missing **directory** is tested. A temp layout with contracts dir present but schema file removed would close the gap cheaply. + +## Nits + +- Nit: `Load_ShouldThrow_WhenBundleItemIdUnknown` calls `TryGetCrossRefError` directly rather than `Load`; rename to `TryGetCrossRefError_ShouldReturnError_WhenBundleItemIdUnknown` (or route through the loader) for clarity. + +- Nit: Bruno folder `bruno/neon-sprawl-server/contract-catalog/` (health smoke) is a nice addition but not listed in the plan — harmless; optional one-line in plan reconciliation if you want doc parity. + +- Nit: `AddContractTemplateCatalog` re-binds `ContentPathsOptions` (same as faction/quest extensions) — fine; no action needed. + +## Verification + +Commands run during review (all green): + +```bash +dotnet test NeonSprawl.sln +python3 scripts/validate_content.py +``` + +Author should confirm fail-fast startup manually once (optional): + +```bash +# Temporarily break encounterTemplateId in prototype_contract_templates.json +dotnet run --project server/NeonSprawl.Server # expect startup InvalidOperationException +``` + +Before merge: skim decomposition alignment suggestions above — functional code is merge-ready without them.