NEO-55: add code review for inventory GET/POST HTTP and Bruno.

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

@@ -0,0 +1,56 @@
+# 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`](../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`](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) | **Matches** — NEO-55 HTTP bullet under **Related implementation slices**. |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../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`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M3 **In Progress** is correct; **E3.M3 note** footnote still lists through **NEO-54** only (NEO-55 not mentioned). |
+| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — inventory mutations remain server-authoritative; HTTP is read/apply surface only. |
+| [`server/README.md`](../../server/README.md) | **Matches** — Player inventory section documents GET/POST shapes, curl examples, reason codes, persistence; NEO-54 deferral removed. |
+
+Register/tracking: alignment table updated; optional footnote extension for NEO-55 in dependency register.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Dependency register footnote (optional)** — Extend the **E3.M3 note** in [`module_dependency_register.md`](../decomposition/modules/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.
+
+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.
+
+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.
+
+## 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.
+
+- 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.
+
+## Verification
+
+```bash
+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`).