# 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) | **Matches** — E3.M3 **In Progress**; footnote lists NEO-50–NEO-54. |
| [`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).~~ **Done.** — removed in-Act assertion; slot count verified only in **Assert** via fresh factory read-back.

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.~~ **Done.** — `TryAddStack_ForUnknownPlayer_ShouldReturnStoreMissing` in `PlayerInventoryOperationsTests.cs`.

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.~~ **Done.** — `TryAddStack_WhenQuantityExceedsStackMax_ShouldSpanMultipleEmptySlots` asserts 20+20+1 across three slots.

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).~~ **Done.**

## 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.~~ **Done.**

- 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).