NEO-54: add code review for inventory store and rules engine.

docs/reviews/2026-05-23-NEO-54.md

@@ -0,0 +1,58 @@
+# Code review — NEO-54 player inventory store + stack/slot rules
+
+**Date:** 2026-05-23  
+**Scope:** Branch `NEO-54-player-inventory-store-stackslot-rules` · commits `e5ffa1b`–`3724a54` vs `origin/main`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-54 delivers the planned split between **`IPlayerInventoryStore`** (in-memory + Postgres sparse rows, `V005` migration) and **`PlayerInventoryOperations`** (catalog-routed add/remove, all-or-nothing adds, stable deny reason codes). The engine mirrors NEO-38’s store + operations pattern, chains DI from **`AddItemCatalog`**, and tests cover merge/stack max, bag full, equipment single slot, remove drain order, deny immutability, and Postgres persistence across a fresh factory. Docs (plan, E3.M3 slice, alignment table, README) match the implementation. Risk is moderate-low for merge: no HTTP yet, but persistence and rule correctness are well exercised; the main follow-ups are test hygiene (AAA), optional coverage gaps, and register footnote parity.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| [`docs/plans/NEO-54-implementation-plan.md`](../plans/NEO-54-implementation-plan.md) | **Matches** — 24 bag + 1 equipment, split store/engine, reason codes, `V005`, Postgres/in-memory parity tests, DI via catalog extensions, docs/README updates; acceptance checklist complete. |
+| [`docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Matches** — NEO-54 bullet under **Related implementation slices**; frozen roster/stack limits align with tests. |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M3 row notes NEO-54 landed; NEO-55 HTTP still planned. |
+| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M3 **In Progress**; footnote still lists NEO-50–NEO-53 only (see suggestion). |
+| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-authoritative inventory mutations; no client-trusted placement in this slice. |
+| [`server/README.md`](../../server/README.md) | **Matches** — Player inventory store section: containers, reason codes, persistence note, NEO-55 deferral. |
+
+Register/tracking: E3.M3 alignment row is updated appropriately; module **Status** remains **In Progress** until NEO-55+.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **AAA in Postgres deny test** — In `PlayerInventoryPersistenceIntegrationTests.TryAddStack_WhenDenied_ShouldNotPersistPartialState`, move `Assert.Equal(PlayerInventorySnapshot.BagSlotCount, OccupiedBagSlotCount(afterDeny!))` from the first **Act** block into **Assert** (or a dedicated assert phase after both act steps), per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert).
+
+2. **`StoreMissing` coverage (optional)** — `PlayerInventoryMutationKind.StoreMissing` is documented on `PlayerInventoryMutationOutcome` but untested. A small operations test for an unknown `playerId` (in-memory store without that bucket) would lock the contract before NEO-55 HTTP maps it.
+
+3. **Multi-slot add (optional)** — Add a test that places quantity **> `stackMax`** across **multiple new** empty slots (e.g. 41 × `field_stim_mk0` on empty bag → three occupied slots). Merge and bag-full paths are covered; this would explicitly guard the `FillEmptySlots` loop beyond single-slot adds.
+
+4. **Dependency register footnote (optional)** — Extend the **E3.M3 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to mention **NEO-54** alongside NEO-50–NEO-53 (same pattern as NEO-52/NEO-53 reviews).
+
+## Nits
+
+- Nit: **`PlayerInventoryMutationOutcome.cs`** — enum and record parameters use **2-space** indent; neighboring outcome types (e.g. `SkillProgressionGrantApplyOutcome.cs`) use **4-space**. Format-only tidy.
+
+- Nit: **`PostgresPlayerInventoryStore.TryReplaceSnapshot`** — delete-all + insert per mutation is correct for snapshot replace; concurrent writers are last-write-wins (same class of risk as other NEO-29-style stores). Fine for prototype; NEO-55 HTTP may want documenting or serializing per player if load tests show races.
+
+- Nit: **`ContainerFromSlotKind`** — any non-`equipment` catalog value routes to **bag**. Safe for the frozen six-item catalog; if new `inventorySlotKind` values appear, consider an explicit deny rather than silent bag routing.
+
+## Verification
+
+```bash
+cd server
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerInventory"
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
+```
+
+With Postgres configured (`ConnectionStrings__NeonSprawl`), confirm `[RequirePostgresFact]` persistence tests run (not skipped). No Bruno/manual QA for this story (no HTTP).