From 362608ed5f2f4807b76f0ef98edb7a181480867c Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 23 May 2026 19:29:37 -0400 Subject: [PATCH] NEO-54: add code review for inventory store and rules engine. --- docs/reviews/2026-05-23-NEO-54.md | 58 +++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 docs/reviews/2026-05-23-NEO-54.md diff --git a/docs/reviews/2026-05-23-NEO-54.md b/docs/reviews/2026-05-23-NEO-54.md new file mode 100644 index 0000000..4e36849 --- /dev/null +++ b/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).