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

Code review — NEO-55 player inventory GET/POST + Bruno

Date: 2026-05-23
Scope: Branch NEO-55-player-inventory-getpost-bruno · commit 69327f5 vs origin/main
Base: origin/main

Verdict

Approve with nits

Summary

NEO-55 adds versioned GET / POST /game/players/{id}/inventory with DTOs, position-gated reads, engine-backed add/remove mutations, and structured deny responses mirroring the NEO-37/38 skill-progression pattern. Implementation delegates all rules to PlayerInventoryOperations (NEO-54), maps HTTP status codes per plan (400 schema/mutationKind, 404 unknown player/store, 200 + applied/reasonCode for engine denies), and includes eight AAA integration tests plus three Bruno requests. Docs (plan, E3.M3 slice, alignment table, README) are updated and match the code. Risk is low for merge: thin HTTP adapter over a well-tested engine; remaining gaps are optional coverage and register footnote parity.

Documentation checked

Document	Result
docs/plans/NEO-55-implementation-plan.md	Matches — single POST + mutationKind, fixed 24+1 slot arrays, applied/reasonCode/snapshot echo, status mapping, Bruno + README, no manual QA doc per kickoff; acceptance checklist complete.
docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md	Matches — NEO-55 HTTP bullet under Related implementation slices.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches — E3.M3 row notes NEO-55 landed with README + Bruno pointers; NEO-56 telemetry still planned.
docs/decomposition/modules/module_dependency_register.md	Matches — E3.M3 In Progress; footnote lists NEO-50–NEO-55.
docs/decomposition/modules/client_server_authority.md	Matches — inventory mutations remain server-authoritative; HTTP is read/apply surface only.
server/README.md	Matches — Player inventory section documents GET/POST shapes, curl examples, reason codes, persistence; NEO-54 deferral removed.

Register/tracking: alignment table and dependency register footnote updated for NEO-55.

Blocking issues

None.

Suggestions

1. Dependency register footnote (optional) — Extend the E3.M3 note in module_dependency_register.md to mention NEO-55 (inventory HTTP) alongside NEO-50–NEO-54, matching the alignment table and prior NEO-54 review pattern. Done.

2. POST remove happy path (optional) — Integration tests cover add success and remove deny (insufficient_quantity) but not add-then-remove success. A short round-trip test would lock the full POST contract for both mutationKind values. Done. — PostInventory_ShouldApplyRemove_AfterAdd_WithUpdatedSnapshot in PlayerInventoryApiTests.cs.

3. invalid_quantity HTTP smoke (optional) — Engine returns invalid_quantity for non-positive quantity; no HTTP test asserts 200 + applied: false + that code. Low risk (engine tested in NEO-54) but would complete the documented reason-code surface at the API layer. Done. — PostInventory_ShouldDenyNonPositiveQuantity_WithInvalidQuantity in PlayerInventoryApiTests.cs.

Nits

• Nit: Post inventory add.bru uses expect(scrap.quantity).to.be.at.least(5) — correct for a cumulative dev server, but an exact === 5 assertion would fail if prior manual runs left stacks. Acceptable for prototype Bruno; document “fresh dev player” in folder docs if flakes appear. Done. — folder.bru docs note fresh-server baseline and cumulative stacks.

• Nit: Plan referenced a BuildSnapshot helper name (NEO-37 mirror); implementation uses MapSnapshot — behavior is equivalent, naming only.

• Nit: No Bruno request for remove happy path — plan minimum (one deny) is met via invalid_item; a remove-after-add Bruno would complement manual QA. Done. — Post inventory remove.bru (seq 4).

Verification

cd server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerInventoryApiTests"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerInventory"

Run Bruno folder bruno/neon-sprawl-server/inventory/ against a local dev server (dotnet run, player dev-local-1).