diff --git a/docs/reviews/2026-06-01-NEO-114.md b/docs/reviews/2026-06-01-NEO-114.md new file mode 100644 index 0000000..784cbe9 --- /dev/null +++ b/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.