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-05-31-NEO-113.md

4.6 KiB
Raw Permalink Blame History

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

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