64 lines
4.6 KiB
Markdown
64 lines
4.6 KiB
Markdown
# Code review — NEO-113 (E7M1-02)
|
||
|
||
**Date:** 2026-05-31
|
||
**Scope:** Branch `NEO-113-e7m1-server-quest-catalog-load-fail-fast` vs `df24281` (merge-base on `main`) — commits `cb4761b` … `1044547`
|
||
**Base:** `df24281` (main at branch point, post NEO-112 merge)
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-113 delivers fail-fast server startup loading for `content/quests/*_quests.json`, mirroring the NEO-101 / NEO-112 pattern: row DTOs, `QuestDefinitionCatalogLoader` with JsonSchema.Net validation, duplicate id gates, and `PrototypeE7M1QuestCatalogRules` for CI-parity E7M1 gates (four-id allowlist, acyclic prerequisites, item/recipe/encounter cross-refs against upstream catalogs, operator-chain terminal token). DI registers `QuestDefinitionCatalog` after item, recipe, and encounter catalogs; `Program.cs` eager-resolves before `Run()`. Eighteen AAA loader + host boot tests pass locally; documentation (implementation plan, alignment register, module register note, backlog AC, `server/README.md`) is thorough. No client work (correct). Risk is low — established catalog-load pattern with strong negative coverage; Bruno health smoke is a nice bonus.
|
||
|
||
## Documentation checked
|
||
|
||
| Path | Result |
|
||
|------|--------|
|
||
| `docs/plans/NEO-113-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. |
|
||
| `docs/plans/E7M1-prototype-backlog.md` (E7M1-02) | **Matches** — AC checked; client counterpart none. |
|
||
| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — freeze table, CI enforcement + server-load line, Status notes NEO-113 landed. |
|
||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-113 server load; register stays Planned until NEO-114. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note updated for NEO-113 server load. |
|
||
| `server/README.md` | **Matches** — quest catalog section (config keys, discovery, fail-fast, load order, NEO-114 forward reference). |
|
||
| Full-stack epic decomposition | **N/A** — infrastructure-only; no client counterpart until NEO-122/NEO-123. |
|
||
|
||
## Blocking issues
|
||
|
||
(none)
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Refresh `E7_M1_QuestStateMachine.md` Status line** — After merge, note that **E7M1-02 / NEO-113** server load is **landed** and runtime/registry work continues from **NEO-114** (mirror the alignment register wording). Optionally add a one-line “server load (NEO-113)” bullet under **CI enforcement** pointing at `server/README.md#quest-catalog-contentquests-neo-113`.~~ **Done.**
|
||
|
||
2. ~~**Add `Load_ShouldThrow_WhenRowViolatesSchema` test** — The implementation plan and NEO-101 precedent (`EncounterDefinitionCatalogLoaderTests.Load_ShouldThrow_WhenRowViolatesSchema`) call for a row-level schema failure case (e.g. empty `displayName` or missing required objective field). Other negative paths are well covered; this is the main gap vs the plan’s test table.~~ **Done.**
|
||
|
||
3. ~~**Chain terminal quantity negative test** — Plan lists “wrong chain terminal objective (kind / item / quantity)”; kind and item are covered; adding a quantity ≠ 1 case would close the loop on `TryGetChainTerminalGateError`.~~ **Done.**
|
||
|
||
## Nits
|
||
|
||
- ~~Nit: Bruno `bruno/neon-sprawl-server/quest-catalog/` is not listed in the implementation plan file table — harmless smoke; consider a one-line mention in plan reconciliation if you want docs to mirror NEO-103/NEO-78 Bruno folders.~~ **Done.** — plan reconciliation lists Bruno boot smoke.
|
||
|
||
- Nit: E7M1 four-id gate error strings differ slightly from Python (`'id'` quoted list vs `repr` list) — acceptable; both are actionable.
|
||
|
||
- Nit: `QuestDefinitionCatalog` XML doc correctly defers callers to `IQuestDefinitionRegistry` (NEO-114) — good guardrail for the strict NEO-113/114 split.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
# Quest catalog tests (20 — primary signal for this story)
|
||
cd server
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
|
||
--filter "FullyQualifiedName~QuestDefinitionCatalogLoaderTests"
|
||
|
||
# Full server suite (CI; requires Docker for Postgres integration tests)
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
|
||
|
||
# CI parity — Python content gate still passes with repo catalog
|
||
python3 -m pip install -r scripts/requirements-content.txt
|
||
python3 scripts/validate_content.py
|
||
```
|
||
|
||
**Reviewer note:** All 20 `QuestDefinitionCatalogLoaderTests` passed locally after review follow-up. Full server suite: 602 passed; 12 Postgres/docker integration tests skipped/failed in this environment (no Docker daemon) — unrelated to NEO-113.
|