NEO-59: add code review document

docs/reviews/2026-05-24-NEO-59.md

@@ -0,0 +1,56 @@
+# Code review — NEO-59 resource-node definition registry + DI
+
+**Date:** 2026-05-24  
+**Scope:** Branch `NEO-59-e3m1-resource-node-definition-registry-di` · commits `4921f2c`–`01dec2b` vs `main`  
+**Base:** `main`
+
+## Verdict
+
+**Approve**
+
+## Summary
+
+NEO-59 adds `IResourceNodeDefinitionRegistry` backed by the startup-loaded `ResourceNodeCatalog` (NEO-58), following the NEO-52 `IItemDefinitionRegistry` pattern exactly. The registry exposes `TryGetDefinition`, `TryGetYield`, and `GetDefinitionsInIdOrder`; DI is wired inside the existing `AddResourceNodeCatalog` extension so no `Program.cs` change is needed. Nine focused AAA tests (null id, unknown id, known id with metadata, ordinal enumeration, loader-backed fixture, host DI resolution) all pass. Docs (implementation plan, alignment table, server README, E3M1 backlog) match the landed behavior.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| [`docs/plans/NEO-59-implementation-plan.md`](../plans/NEO-59-implementation-plan.md) | **Matches** — scope, acceptance checklist, technical approach, and file list align with the diff; all four checklist items marked complete. |
+| [`docs/plans/E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) | **Matches** — E3M1-03 acceptance items flipped with NEO-59 landed note. |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-59 registry + plan link; "Still planned" updated to drop registry. |
+| [`server/README.md`](../../server/README.md) | **Matches** — resource-node catalog section updated: registry guidance and "do not inject `ResourceNodeCatalog` in new game code" advisory added. |
+
+Register/tracking: alignment table updated; no further register change required.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+None.
+
+## Nits
+
+- `GetDefinitionsInIdOrder()` iterates `.Keys` to build a sorted array then does a second dictionary lookup per key. Iterating key-value pairs directly is slightly more idiomatic and avoids the double lookup:
+
+  ```csharp
+  return catalog.NodesById
+      .OrderBy(kv => kv.Key, StringComparer.Ordinal)
+      .Select(kv => kv.Value)
+      .ToList();
+  ```
+
+  The current form matches `ItemDefinitionRegistry` exactly, so consistency may outweigh the change. No action required.
+
+- The null-id and unknown-id tests each build a fully populated `nodes`/`yields` dictionary even though the exercised code paths exit before those entries are touched. Consistent with `ItemDefinitionRegistryTests` — no action required.
+
+- No `docs/manual-qa/NEO-59.md` — consistent with plan (no user-facing HTTP endpoint in this ticket; NEO-60 covers that).
+
+## Verification
+
+```bash
+dotnet test server/NeonSprawl.Server.Tests --filter "FullyQualifiedName~ResourceNodeDefinitionRegistry"
+# Passed! Failed: 0, Passed: 9, Skipped: 0
+```