57 lines
4.2 KiB
Markdown
57 lines
4.2 KiB
Markdown
# Code review — NEO-67 recipe definition registry + DI
|
||
|
||
**Date:** 2026-05-24
|
||
**Scope:** Branch `NEO-67-recipe-definition-registry-di` · commits `7188c5b`–`81b0e3f` vs `origin/main`
|
||
**Base:** `origin/main`
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-67 delivers **E3M2-03**: a thin **`IRecipeDefinitionRegistry`** / **`RecipeDefinitionRegistry`** adapter over the NEO-66 startup-loaded **`RecipeDefinitionCatalog`**, registered in DI alongside the catalog singleton. The interface mirrors **`IItemDefinitionRegistry`** (NEO-52): `TryGetDefinition(string? recipeId, …)` and `GetDefinitionsInIdOrder()` with ordinal id ordering. Seven AAA unit/host tests pass; docs (plan, E3M2 backlog, E3.M2 snapshot, alignment register, `server/README.md`) are updated. No HTTP, craft engine, or client work — correctly deferred to NEO-68+. Risk is very low: pure adapter + DI wiring over immutable catalog data; no behavior change at boot beyond an extra singleton registration.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-67-implementation-plan.md`](../plans/NEO-67-implementation-plan.md) | **Matches** — interface, adapter, DI extension, comment/README touches, acceptance checklist complete; HTTP/craft/loader changes correctly out of scope. |
|
||
| [`docs/plans/E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) | **Matches** — E3M2-03 acceptance checkbox + landed note. |
|
||
| [`docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) | **Matches** — implementation snapshot references NEO-67 registry. |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M2 row updated with NEO-67 landed note; next backlog item NEO-68. |
|
||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — table **Status** still **Planned** for E3.M2 (same pattern as post–NEO-66); optional bump when Slice 3 server spine completes. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — server DI/read surface only; no client mutation. |
|
||
| [`server/README.md`](../../server/README.md) | **Matches** — recipe catalog section documents `IRecipeDefinitionRegistry` as preferred lookup surface. |
|
||
| Manual QA | **N/A** — plan documents skip rationale: automated unit/host tests (same as NEO-52 / NEO-59); no player-visible HTTP. |
|
||
|
||
Register/tracking: alignment table E3.M2 **In Progress** with NEO-67 note is correct.
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
None.
|
||
|
||
## Nits
|
||
|
||
- ~~Nit: `RecipeDefinitionRegistry.TryGetDefinition` reads `catalog.ById` directly instead of delegating to `catalog.TryGetRecipe` — consistent with `ItemDefinitionRegistry` (NEO-52), but both bypass the catalog’s public lookup helper; harmless for immutable data.~~ **Done.** Registry now delegates to `catalog.TryGetRecipe`.
|
||
|
||
- ~~Nit: `GetDefinitionsInIdOrder()` allocates a new sorted list on every call — acceptable for prototype HTTP/craft volume; optional future optimization is to cache the ordered list at catalog/registry construction time if hot-path profiling warrants it (same as item registry).~~ **Done.** Ordered list built once in `RecipeDefinitionRegistry` constructor.
|
||
|
||
- ~~Nit: `Host_ShouldResolveRegistryFromDi_WhenStartupSucceeds` asserts a known id and enumeration count but not an unknown id in the host context — unit tests cover unknown/null; host test could add one line for symmetry with E3M2-03 acceptance wording (“distinguishable from valid ids in tests”).~~ **Done.** Host test asserts unknown `recipeId` returns false without throw.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
cd /home/don/neon-sprawl/server
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~RecipeDefinitionRegistryTests"
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
|
||
```
|
||
|
||
Manual (optional):
|
||
|
||
- Start server from repo root; confirm existing NEO-66 Information log (recipe count **8**) unchanged — registry does not alter boot semantics.
|