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