From fddbb95a45e57be24b7b5ba2fae493230f56d9fa Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 23 May 2026 22:51:59 -0400 Subject: [PATCH] NEO-55: address code review suggestions for inventory HTTP Add remove round-trip and invalid_quantity API tests, Bruno remove request, dependency register NEO-55 footnote, and strike-through review follow-ups. --- .../inventory/Post inventory remove.bru | 41 ++++++++++++++++ bruno/neon-sprawl-server/inventory/folder.bru | 7 +++ .../modules/module_dependency_register.md | 2 +- docs/reviews/2026-05-23-NEO-55.md | 14 +++--- .../Game/Items/PlayerInventoryApiTests.cs | 48 +++++++++++++++++++ 5 files changed, 104 insertions(+), 8 deletions(-) create mode 100644 bruno/neon-sprawl-server/inventory/Post inventory remove.bru diff --git a/bruno/neon-sprawl-server/inventory/Post inventory remove.bru b/bruno/neon-sprawl-server/inventory/Post inventory remove.bru new file mode 100644 index 0000000..1e3fbd7 --- /dev/null +++ b/bruno/neon-sprawl-server/inventory/Post inventory remove.bru @@ -0,0 +1,41 @@ +meta { + name: POST inventory remove + type: http + seq: 4 +} + +docs { + NEO-55: remove stack via mutationKind remove. Run Post inventory add first on a fresh server, + or expect applied true when dev-local-1 already holds scrap_metal_bulk. +} + +post { + url: {{baseUrl}}/game/players/dev-local-1/inventory + body: json + auth: none +} + +body:json { + { + "schemaVersion": 1, + "mutationKind": "remove", + "itemId": "scrap_metal_bulk", + "quantity": 2 + } +} + +tests { + test("remove returns 200 with applied true and reduced quantity", function () { + expect(res.getStatus()).to.equal(200); + const body = res.getBody(); + expect(body.schemaVersion).to.equal(1); + expect(body.applied).to.equal(true); + expect(body.reasonCode).to.equal(null); + expect(body.inventory).to.be.an("object"); + const scrap = body.inventory.bagSlots.find((s) => s.itemId === "scrap_metal_bulk"); + if (scrap) { + expect(scrap.quantity).to.be.a("number"); + expect(scrap.quantity).to.be.at.least(0); + } + }); +} diff --git a/bruno/neon-sprawl-server/inventory/folder.bru b/bruno/neon-sprawl-server/inventory/folder.bru index 12b2b71..238dd15 100644 --- a/bruno/neon-sprawl-server/inventory/folder.bru +++ b/bruno/neon-sprawl-server/inventory/folder.bru @@ -1,3 +1,10 @@ meta { name: inventory } + +docs { + NEO-55 inventory HTTP. Run GET first on a fresh dev server for empty 24+1 grid. + POST add/remove mutate dev-local-1; cumulative stacks from prior manual runs can make + Post inventory add assertions use at.least(5). Restart server (or truncate DB in Postgres + mode) for a clean inventory baseline. +} diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index a6b1fe1..6b59763 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -48,7 +48,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen | E3.M4 | SinkAndDurabilityLifecycle | E3.M3, E8.M3 | DurabilityState, ItemSinkEvent, RepairCostRule | Pre-production | Planned | | E3.M5 | EconomyBalancePolicy | E3.M4, E9.M2 | EconomyPolicy, PriceBandRule, FaucetSinkRatio | Pre-production | Planned | -**E3.M3 note:** Epic 3 **Slice 1** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) → [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56); label **`E3.M3`**. See [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md), [E3_M3_ItemizationAndInventorySchema.md](E3_M3_ItemizationAndInventorySchema.md). **NEO-50** (content + CI), **NEO-51** (server fail-fast load), **NEO-52** (`IItemDefinitionRegistry` + DI), **NEO-53** (`GET /game/world/item-definitions`), and **NEO-54** (inventory store + stack/slot rules engine) moved the register row to **In Progress**; later slices update the alignment table as they land. +**E3.M3 note:** Epic 3 **Slice 1** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) → [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56); label **`E3.M3`**. See [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md), [E3_M3_ItemizationAndInventorySchema.md](E3_M3_ItemizationAndInventorySchema.md). **NEO-50** (content + CI), **NEO-51** (server fail-fast load), **NEO-52** (`IItemDefinitionRegistry` + DI), **NEO-53** (`GET /game/world/item-definitions`), **NEO-54** (inventory store + stack/slot rules engine), and **NEO-55** (`GET`/`POST /game/players/{id}/inventory`) moved the register row to **In Progress**; later slices update the alignment table as they land. ### Epic 4 — World Topology diff --git a/docs/reviews/2026-05-23-NEO-55.md b/docs/reviews/2026-05-23-NEO-55.md index 4261f48..b94942f 100644 --- a/docs/reviews/2026-05-23-NEO-55.md +++ b/docs/reviews/2026-05-23-NEO-55.md @@ -19,11 +19,11 @@ NEO-55 adds versioned **`GET` / `POST /game/players/{id}/inventory`** with DTOs, | [`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/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E3.M3 **In Progress**; footnote lists NEO-50–NEO-55. | | [`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. +Register/tracking: alignment table and dependency register footnote updated for NEO-55. ## Blocking issues @@ -31,19 +31,19 @@ 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. +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.~~ **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. +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. +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. +- ~~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. +- ~~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 diff --git a/server/NeonSprawl.Server.Tests/Game/Items/PlayerInventoryApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Items/PlayerInventoryApiTests.cs index 218d1ea..5d20d3f 100644 --- a/server/NeonSprawl.Server.Tests/Game/Items/PlayerInventoryApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Items/PlayerInventoryApiTests.cs @@ -139,6 +139,54 @@ public sealed class PlayerInventoryApiTests Assert.Equal(10, occupied[0].Quantity); } + [Fact] + public async Task PostInventory_ShouldApplyRemove_AfterAdd_WithUpdatedSnapshot() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + // Act + _ = await client.PostAsJsonAsync( + "/game/players/dev-local-1/inventory", + Mutation("add", "scrap_metal_bulk", quantity: 10)); + var response = await client.PostAsJsonAsync( + "/game/players/dev-local-1/inventory", + Mutation("remove", "scrap_metal_bulk", quantity: 4)); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + Assert.True(body!.Applied); + Assert.Null(body.ReasonCode); + var occupied = body.Inventory.BagSlots.Where(static s => s.Quantity > 0).ToList(); + Assert.Single(occupied); + Assert.Equal("scrap_metal_bulk", occupied[0].ItemId); + Assert.Equal(6, occupied[0].Quantity); + } + + [Fact] + public async Task PostInventory_ShouldDenyNonPositiveQuantity_WithInvalidQuantity() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + // Act + var response = await client.PostAsJsonAsync( + "/game/players/dev-local-1/inventory", + Mutation("add", "scrap_metal_bulk", quantity: 0)); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + Assert.False(body!.Applied); + Assert.Equal(PlayerInventoryReasonCodes.InvalidQuantity, body.ReasonCode); + Assert.All(body.Inventory.BagSlots, static slot => Assert.Equal(0, slot.Quantity)); + } + [Fact] public async Task PostInventory_ShouldDenyUnknownItem_WithInvalidItem_AndLeaveInventoryEmpty() {