NEO-114: Add code review for quest definition registry.

docs/reviews/2026-06-01-NEO-114.md

@@ -0,0 +1,61 @@
+# Code review — NEO-114 (E7M1-03)
+
+**Date:** 2026-06-01  
+**Scope:** Branch `NEO-114-e7m1-injectable-quest-definition-registry-di` vs `d236aee` (merge-base on `main`) — commits `9056a9b` … `0bcbb4b`  
+**Base:** `d236aee` (main at branch point, post NEO-113 merge)
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-114 adds **`IQuestDefinitionRegistry`** and **`QuestDefinitionRegistry`**, a thin adapter over the NEO-113 **`QuestDefinitionCatalog`**, mirroring the established NEO-102 encounter registry pattern: `TryGetDefinition`, `TryNormalizeKnown`, and cached `GetDefinitionsInIdOrder`. DI registers the registry in **`AddQuestDefinitionCatalog`** after the catalog singleton; no `Program.cs` change (catalog remains the eager boot surface). Fourteen AAA unit + host tests pass locally; documentation (implementation plan, alignment register, module register, E7.M1 status, backlog AC, `server/README.md`) is updated. Server-only infrastructure with no client counterpart (correct). Risk is low — predictable adapter with strong parity to encounter registry and good host integration coverage.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-114-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation accurate. |
+| `docs/plans/E7M1-prototype-backlog.md` (E7M1-03) | **Partially matches** — AC checked; **In scope** bullets still say `TryGet` and `AddQuestDefinitionRegistry` (kickoff chose `TryGetDefinition` and extend `AddQuestDefinitionCatalog`). |
+| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — Status notes E7M1-03 / NEO-114 registry landed; runtime from E7M1-04. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-114 registry; register stays Planned until E7M1-05+ runtime. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note updated for NEO-114 registry landed. |
+| `server/README.md` | **Matches** — quest catalog section documents registry as preferred lookup surface with normalization note. |
+| Full-stack epic decomposition | **N/A** — infrastructure-only; no client counterpart until NEO-122/NEO-123. |
+
+## Blocking issues
+
+(none)
+
+## Suggestions
+
+1. **Refresh E7M1-03 backlog wording** — Update `docs/plans/E7M1-prototype-backlog.md` **In scope** to match shipped API: `TryGetDefinition` (not `TryGet`), extend `AddQuestDefinitionCatalog` (not separate `AddQuestDefinitionRegistry`), and optional mention of `TryNormalizeKnown` / `GetDefinitionsInIdOrder` per the implementation plan.
+
+2. **`TryGetDefinition` empty-string test** — Implementation plan test table lists false for null/unknown/**empty**; tests cover null and unknown but not `""`. Encounter registry has the same gap; adding one unit test would close the plan loop and guard accidental empty-key catalog entries.
+
+## Nits
+
+- Nit: `GetDefinitionsInIdOrder_ShouldListAllRowsOrderedById_WhenMultipleQuests` uses three synthetic rows (ordering smoke) while the host test validates four frozen ids — intentional split; no change required.
+
+- Nit: `[Theory]` over `ExpectedQuestIds` uses minimal stub rows (`DisplayName: "Test Quest"`) while gather intro / operator chain have richer metadata tests; host test asserts non-empty display names for all four — adequate coverage.
+
+- Nit: `QuestDefinitionCatalog` XML and README correctly steer new code to `IQuestDefinitionRegistry` — good guardrail ahead of NEO-115 HTTP.
+
+## Verification
+
+```bash
+# Quest registry tests (14 — primary signal for this story)
+cd server
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~QuestDefinitionRegistryTests"
+
+# Quest catalog loader tests (unchanged; regression)
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~QuestDefinitionCatalogLoaderTests"
+
+# Full server suite (CI)
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
+```
+
+**Reviewer note:** All 14 `QuestDefinitionRegistryTests` passed locally during review.