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-06-20-NEO-145.md

5.6 KiB
Raw Blame History

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 $refs, 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. Done. Server load (NEO-145) paragraph added.
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. Done. NEO-145 server startup mirror sentence added.
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. Done. E7M4-02 / NEO-145 row extended.
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. Done. E7.M4 note cites 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. Done.

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. Done.

  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. Done.

  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. Done. Load_ShouldThrow_WhenItemQuantityExceedsPrototypeBundleLimits exercises loader→freeze gate (qty 11 fails freeze before band-cap on the single-template roster); TryGetBandCapGateError_ShouldReturnError_WhenItemQuantityExceedsBandCap covers band-cap rule directly.

  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. Done. Load_ShouldThrow_WhenSchemaFileMissing added.

Nits

  • Nit: Load_ShouldThrow_WhenBundleItemIdUnknown calls TryGetCrossRefError directly rather than Load; rename to TryGetCrossRefError_ShouldReturnError_WhenBundleItemIdUnknown (or route through the loader) for clarity. Done.

  • 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. Done. Plan reconciliation notes Bruno health smoke.

  • Nit: AddContractTemplateCatalog re-binds ContentPathsOptions (same as faction/quest extensions) — fine; no action needed.

Verification

Commands run during review (all green):

dotnet test NeonSprawl.sln
python3 scripts/validate_content.py

Author should confirm fail-fast startup manually once (optional):

# 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.