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

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	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	Matches — NEO-54 bullet under Related implementation slices; frozen roster/stack limits align with tests.
docs/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	Matches — E3.M3 In Progress; footnote lists NEO-50–NEO-54.
docs/decomposition/modules/client_server_authority.md	Matches — server-authoritative inventory mutations; no client-trusted placement in this slice.
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. 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 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

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