4.1 KiB

Raw Blame History

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)	Matches — AC checked; In scope wording aligned with shipped API (NEO-114 review follow-up).
`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

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. Done.
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. Done. — TryGetDefinition_ShouldReturnFalse_WhenQuestIdIsEmpty.

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

# 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. After follow-up: 15 tests (empty-string TryGetDefinition case).

4.1 KiB Raw Blame History