NEO-61: add code review document.

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

@@ -0,0 +1,62 @@
+# Code review — NEO-61 world node instance depletion store
+
+**Date:** 2026-05-24  
+**Scope:** Branch `NEO-61-e3m1-world-node-instance-depletion-store` · commits `869d411`–`6d2d3c0` vs `origin/main`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-61 adds server-authoritative **remaining gathers** per world node instance: **`IResourceNodeInstanceStore`** (in-memory + Postgres **`V006`**) and **`ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement`** with registry-backed lazy init, atomic decrement, and stable **`node_depleted`** / **`unknown_node`** reason codes. The split-store pattern mirrors NEO-54; DI chains from **`AddResourceNodeCatalog`**, and **`InMemoryWebApplicationFactory`** forces the in-memory store in tests. Ten automated tests cover lazy init, depletion loop, unknown-node deny, store normalization/idempotency, and Postgres cross-factory persistence. Risk is low: no HTTP surface, no interact wiring, no inventory mutation — persistence + rules only, ready for NEO-62/NEO-63.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| [`docs/plans/NEO-61-implementation-plan.md`](../plans/NEO-61-implementation-plan.md) | **Matches** — split store + operations, lazy init, keep depleted rows at 0, reason codes, **`V006`**, DI chain, README, acceptance checklist complete. Minor: plan listed in-memory **concurrency tests** and an `out outcome` signature; implementation uses per-id locks (no concurrent test) and returns the outcome struct directly. |
+| [`docs/plans/E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) | **Matches** — E3M1-05 acceptance items flipped with NEO-61 landed note. |
+| [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Matches** — NEO-61 depletion-store bullet under **Related implementation slices**; per-instance capacity, world-wide keying, **`node_depleted`** align with **Purpose** / **Responsibilities**. |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-61 landed with README link; **Still planned** correctly lists gather engine / interact wiring. |
+| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M1 row **In Progress** is correct, but **E3.M1 note** footnote still lists NEO-57–NEO-60 only and says “until depletion … slices land” without NEO-61 (alignment table already updated). |
+| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-authoritative capacity; no client-trusted depletion state. |
+| [`server/README.md`](../../server/README.md) | **Matches** — Resource node instance depletion subsection: lazy init, reason-code table, persistence split, NEO-63 deferral. |
+
+Register/tracking: alignment table update is done; **E3.M1 note** footnote should mention NEO-61 when touched (same pattern as NEO-60 register suggestion).
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **AAA layout in stateful tests** — Two new test methods mix **Assert** into **Act**:
+   - `ResourceNodeInstanceOperationsTests.TryCommitSuccessfulGatherDecrement_RepeatedUntilCapacity_ShouldReturnNodeDepletedWithoutGoingNegative` — assertions inside the decrement loop.
+   - `ResourceNodeInstancePersistenceIntegrationTests.TryCommitSuccessfulGatherDecrement_WhenDepleted_ShouldPersistZeroRow` — assertions inside the final `using` Act block.
+   Move loop invariants / final expectations to a dedicated **Assert** section (collect last applied outcome during Act, assert after) per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert).
+
+2. **Dependency register footnote (optional)** — Extend **E3.M1 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to include **NEO-61** (E3M1-05 depletion store) so the register matches the alignment table — same follow-up as NEO-60 for NEO-57–NEO-60.
+
+3. **Concurrency test gap (optional)** — Plan §8 listed in-memory get/ensure/decrement **concurrency hygiene** tests; implementation uses per-id locks but has no parallel test. NEO-54 inventory store also omits concurrent tests, so non-blocking; add a lightweight `Parallel.For` smoke test if you want plan parity.
+
+## Nits
+
+- Nit: **`NormalizeInteractableId`** lives as `internal static` on **`InMemoryResourceNodeInstanceStore`** but is called from **`PostgresResourceNodeInstanceStore`** and **`ResourceNodeInstanceOperations`** — works, but a small shared helper (e.g. `ResourceNodeInstanceIds.Normalize`) would reduce cross-type coupling if this grows.
+
+- Nit: **`TryEnsureInitialized` failure → `unknown_node`** in `ResourceNodeInstanceOperations` (lines 26–31) is unreachable after a registry hit with non-negative **`maxGathers`**; a distinct internal reason or `Debug.Assert` would clarify intent vs catalog miss.
+
+- Nit: Postgres **`TryDecrementRemainingGathers`** opens a second connection via **`TryGetRemainingGathers`** on decrement miss to distinguish depleted-at-zero — correct behavior, slightly extra round-trip on deny path only.
+
+## Verification
+
+```bash
+cd server
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~ResourceNodeInstance"
+# Passed! Failed: 0, Passed: 10, Skipped: 0
+
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
+```
+
+Postgres integration (when `ConnectionStrings__NeonSprawl` is set): `ResourceNodeInstancePersistenceIntegrationTests` — cross-factory read and depleted-row-at-zero persistence.