NEO-145: add code review for contract catalog load branch.
parent
937412eda6
commit
1b904b594d
|
|
@ -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.
|
||||||
Loading…
Reference in New Issue