NEO-70: Address code review findings for craft HTTP.

Rename wire DTO to PlayerCraftRequest, add store-missing and default-quantity
API tests, improve Bruno deny isolation, README success sample, and doc nits.
pull/104/head
VinPropane 2026-05-24 18:00:26 -04:00
parent 9d6d358956
commit 5a3904c6bc
11 changed files with 119 additions and 22 deletions

View File

@ -1,11 +1,24 @@
meta { meta {
name: POST craft deny insufficient materials name: POST craft deny insufficient materials
type: http type: http
seq: 3 seq: 1
} }
docs { docs {
NEO-70: craft without seeded materials denies with insufficient_materials on dev-local-1 empty inventory. NEO-70: craft without seeded materials denies with insufficient_materials. Requires empty bag (run before spine or on fresh server).
}
script:pre-request {
const { getInventory, sumItemQuantity } = require("./scripts/inventory-api-helper.js");
const inventory = await getInventory(bru);
const scrap = sumItemQuantity(inventory, "scrap_metal_bulk");
const refined = sumItemQuantity(inventory, "refined_plate_stock");
if (scrap >= 5 || refined > 0) {
throw new Error(
`expected empty craft inputs before insufficient_materials test; scrap=${scrap} refined=${refined} — restart server or run this request before spine`,
);
}
bru.setVar("scrapBeforeInsufficientDeny", scrap);
} }
post { post {

View File

@ -9,8 +9,10 @@ docs {
} }
script:pre-request { script:pre-request {
const { ensureItemQuantity } = require("./scripts/inventory-api-helper.js"); const { ensureItemQuantity, getInventory, sumItemQuantity } = require("./scripts/inventory-api-helper.js");
await ensureItemQuantity(bru, "scrap_metal_bulk", 5); await ensureItemQuantity(bru, "scrap_metal_bulk", 5);
const inventory = await getInventory(bru);
bru.setVar("scrapBeforeInvalidQuantityDeny", sumItemQuantity(inventory, "scrap_metal_bulk"));
} }
post { post {
@ -38,5 +40,7 @@ tests {
expect(body.schemaVersion).to.equal(1); expect(body.schemaVersion).to.equal(1);
expect(body.success).to.equal(false); expect(body.success).to.equal(false);
expect(body.reasonCode).to.equal("invalid_quantity"); expect(body.reasonCode).to.equal("invalid_quantity");
const before = bru.getVar("scrapBeforeInvalidQuantityDeny");
expect(before).to.equal(5);
}); });
} }

View File

@ -1,7 +1,7 @@
meta { meta {
name: POST craft gather refine make spine name: POST craft gather refine make spine
type: http type: http
seq: 1 seq: 5
} }
docs { docs {

View File

@ -3,5 +3,5 @@ meta {
} }
docs { docs {
NEO-70 craft HTTP. Spine uses gather interact (no inventory POST shortcuts). Deny requests may use inventory POST for setup only. NEO-70 craft HTTP. Run order: deny requests first (seq 14), then spine last (seq 5). Spine uses gather interact (no inventory POST shortcuts). Deny setup requests may use inventory POST only where noted. On persistent Postgres dev stores, run insufficient_materials before spine or restart the server so dev-local-1 bag is empty.
} }

View File

@ -242,7 +242,7 @@ Working backlog for **Epic 3 — Slice 3** ([recipes and crafting pipeline](../d
- Slice 4 (**E3.M4** + **E3.M5**) sinks/policy remains pre-production. - Slice 4 (**E3.M4** + **E3.M5**) sinks/policy remains pre-production.
- Track delivery in Linear; keep `blockedBy` links synchronized if scope changes. - Track delivery in Linear; keep `blockedBy` links synchronized if scope changes.
- For each issue kickoff, add `docs/plans/{NEO-XX}-implementation-plan.md` per [story-kickoff](../../.cursor/rules/story-kickoff.md). - For each issue kickoff, add `docs/plans/{NEO-XX}-implementation-plan.md` per [story-kickoff](../../.cursor/rules/story-kickoff.md).
- Add `docs/manual-qa/{NEO-XX}.md` when craft HTTP lands (E3M2-06+). - Add `docs/manual-qa/{NEO-XX}.md` when craft HTTP lands (E3M2-06+)**[NEO-70](https://linear.app/neon-sprawl/issue/NEO-70) waived** manual QA at kickoff (Bruno + README only).
## Related docs ## Related docs

View File

@ -52,7 +52,7 @@ No other blocking decisions — request shape (`recipeId` + optional `quantity`
1. **Route:** **`POST /game/players/{id}/craft`** — trim **`id`**; **404** when empty/unknown position (NEO-55 gate). 1. **Route:** **`POST /game/players/{id}/craft`** — trim **`id`**; **404** when empty/unknown position (NEO-55 gate).
2. **Request (`CraftRequest`, `schemaVersion` **1**):** 2. **Request (`PlayerCraftRequest`, `schemaVersion` **1**):**
- **`recipeId`** (required, non-empty after trim). - **`recipeId`** (required, non-empty after trim).
- **`quantity`** (optional, default **1** when omitted or null) — passed to **`CraftOperations.TryCraft`**. - **`quantity`** (optional, default **1** when omitted or null) — passed to **`CraftOperations.TryCraft`**.

View File

@ -32,21 +32,21 @@ None.
## Suggestions ## Suggestions
1. **Store-missing HTTP → 404** — Plan allows API test *or* README for **`inventory_store_missing`** / **`progression_store_missing`**. README documents **404** paths; engine tests cover progression store missing. Optional: add one **`PlayerCraftApiTests`** case with a test double store returning **`InventoryStoreMissing`** to lock HTTP mapping (mirror **`PlayerInventoryApi`** **`StoreMissing` → 404**). 1. ~~**Store-missing HTTP → 404** — Plan allows API test *or* README for **`inventory_store_missing`** / **`progression_store_missing`**. README documents **404** paths; engine tests cover progression store missing. Optional: add one **`PlayerCraftApiTests`** case with a test double store returning **`InventoryStoreMissing`** to lock HTTP mapping (mirror **`PlayerInventoryApi`** **`StoreMissing` → 404**).~~ **Done.** Added **`PostCraft_WhenInventoryStoreMissing_ShouldReturnNotFound`** with **`InventoryStoreAlwaysMissing`** test double via **`WithWebHostBuilder`**.
2. **Wire request naming** — Wire type is **`CraftRequest`** while inventory uses **`PlayerInventoryMutationRequest`**. Consider **`PlayerCraftRequest`** (or **`CraftWireRequest`**) to avoid collision with the module docs conceptual **`CraftRequest`** contract (actor + recipe + quantities) and to match player-scoped mutation DTO naming. 2. ~~**Wire request naming** — Wire type is **`CraftRequest`** while inventory uses **`PlayerInventoryMutationRequest`**. Consider **`PlayerCraftRequest`** (or **`CraftWireRequest`**) to avoid collision with the module docs conceptual **`CraftRequest`** contract (actor + recipe + quantities) and to match player-scoped mutation DTO naming.~~ **Done.** Renamed wire DTO to **`PlayerCraftRequest`**.
3. **Default `quantity` test** — Plan adopts omitted/null **`quantity` → 1**. Add one API test POST without **`quantity`** (or explicit **`null`**) asserting success with same outcome as **`quantity: 1`**. 3. ~~**Default `quantity` test** — Plan adopts omitted/null **`quantity` → 1**. Add one API test POST without **`quantity`** (or explicit **`null`**) asserting success with same outcome as **`quantity: 1`**.~~ **Done.** Added **`PostCraft_WhenQuantityOmitted_ShouldDefaultToOneAndSucceed`**.
4. **Bruno deny isolation****`Post craft deny insufficient materials.bru`** assumes empty bag on **`dev-local-1`**. On a persistent Postgres dev store, prior spine/inventory requests can seed scrap and cause a false pass. Document collection run order in **`folder.bru`** or add a pre-request inventory baseline assert (same pattern as NEO-63 delta-node isolation). 4. ~~**Bruno deny isolation** — **`Post craft deny insufficient materials.bru`** assumes empty bag on **`dev-local-1`**. On a persistent Postgres dev store, prior spine/inventory requests can seed scrap and cause a false pass. Document collection run order in **`folder.bru`** or add a pre-request inventory baseline assert (same pattern as NEO-63 delta-node isolation).~~ **Done.** **`folder.bru`** documents run order; insufficient-materials request is **seq 1** with pre-request empty-bag guard; spine moved to **seq 5**.
## Nits ## Nits
- Nit: [`E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) line “Add `docs/manual-qa/{NEO-XX}.md` when craft HTTP lands” is generic; NEO-70 kickoff explicitly waived manual QA — consider a footnote so future readers do not expect **`NEO-70.md`**. - ~~Nit: [`E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) line “Add `docs/manual-qa/{NEO-XX}.md` when craft HTTP lands” is generic; NEO-70 kickoff explicitly waived manual QA — consider a footnote so future readers do not expect **`NEO-70.md`**.~~ **Done.**
- Nit: Bruno **`Post craft deny invalid quantity.bru`** does not assert scrap quantity unchanged (API test does via **`CountBagItem`**). - ~~Nit: Bruno **`Post craft deny invalid quantity.bru`** does not assert scrap quantity unchanged (API test does via **`CountBagItem`**).~~ **Done.** Pre-request snapshots scrap total; test asserts quantity still **5**.
- Nit: README craft section has curl + reason table but no sample **200** success JSON body; a one-liner example would help Bruno adopters. - ~~Nit: README craft section has curl + reason table but no sample **200** success JSON body; a one-liner example would help Bruno adopters.~~ **Done.**
## Verification ## Verification
@ -56,7 +56,7 @@ dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "Ful
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerCraftApiTests|FullyQualifiedName~CraftOperationsTests" dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerCraftApiTests|FullyQualifiedName~CraftOperationsTests"
``` ```
**Results (2026-05-24):** **8** `PlayerCraftApiTests` pass; **19** craft-related tests (`PlayerCraftApiTests` + `CraftOperationsTests`) pass. **Results (2026-05-24):** **10** `PlayerCraftApiTests` pass; **21** craft-related tests (`PlayerCraftApiTests` + `CraftOperationsTests`) pass.
Full suite: **11** failures in `AbilityCastApiTests` (host startup / unrelated to this branch diff); **327** pass. Re-run full suite before merge if CI gates on it — failures appear pre-existing on `main`, not introduced by NEO-70. Full suite: **11** failures in `AbilityCastApiTests` (host startup / unrelated to this branch diff); **327** pass. Re-run full suite before merge if CI gates on it — failures appear pre-existing on `main`, not introduced by NEO-70.

View File

@ -14,10 +14,10 @@ public sealed class PlayerCraftApiTests
{ {
private const string PlayerId = "dev-local-1"; private const string PlayerId = "dev-local-1";
private static CraftRequest Request( private static PlayerCraftRequest Request(
string recipeId, string recipeId,
int? quantity = null, int? quantity = null,
int schemaVersion = CraftRequest.CurrentSchemaVersion) => int schemaVersion = PlayerCraftRequest.CurrentSchemaVersion) =>
new() new()
{ {
SchemaVersion = schemaVersion, SchemaVersion = schemaVersion,
@ -67,7 +67,7 @@ public sealed class PlayerCraftApiTests
// Act // Act
var response = await client.PostAsJsonAsync( var response = await client.PostAsJsonAsync(
$"/game/players/{PlayerId}/craft", $"/game/players/{PlayerId}/craft",
new CraftRequest { SchemaVersion = CraftRequest.CurrentSchemaVersion, RecipeId = " " }); new PlayerCraftRequest { SchemaVersion = PlayerCraftRequest.CurrentSchemaVersion, RecipeId = " " });
// Assert // Assert
Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
@ -105,6 +105,30 @@ public sealed class PlayerCraftApiTests
Assert.Equal(RefineSkillXpConstants.ActivitySourceKind, body.XpGrantSummary.SourceKind); Assert.Equal(RefineSkillXpConstants.ActivitySourceKind, body.XpGrantSummary.SourceKind);
} }
[Fact]
public async Task PostCraft_WhenQuantityOmitted_ShouldDefaultToOneAndSucceed()
{
// Arrange
await using var factory = new InMemoryWebApplicationFactory();
var client = factory.CreateClient();
SeedStack(factory, "scrap_metal_bulk", 5);
// Act
var response = await client.PostAsJsonAsync(
$"/game/players/{PlayerId}/craft",
new { schemaVersion = PlayerCraftRequest.CurrentSchemaVersion, recipeId = "refine_scrap_standard" });
// Assert
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
var body = await response.Content.ReadFromJsonAsync<CraftResponse>();
Assert.NotNull(body);
Assert.True(body!.Success);
Assert.Single(body.InputsConsumed);
Assert.Equal(5, body.InputsConsumed[0].Quantity);
Assert.Single(body.OutputsGranted);
Assert.Equal(1, body.OutputsGranted[0].Quantity);
}
[Fact] [Fact]
public async Task PostCraft_WhenMaterialsMissing_ShouldDenyWithInsufficientMaterials() public async Task PostCraft_WhenMaterialsMissing_ShouldDenyWithInsufficientMaterials()
{ {
@ -206,6 +230,36 @@ public sealed class PlayerCraftApiTests
Assert.Equal(TotalBagQuantity(beforeInventory!), TotalBagQuantity(afterInventory!)); Assert.Equal(TotalBagQuantity(beforeInventory!), TotalBagQuantity(afterInventory!));
} }
[Fact]
public async Task PostCraft_WhenInventoryStoreMissing_ShouldReturnNotFound()
{
// Arrange
await using var factory = new InMemoryWebApplicationFactory().WithWebHostBuilder(builder =>
{
builder.ConfigureTestServices(services =>
{
for (var i = services.Count - 1; i >= 0; i--)
{
if (services[i].ServiceType == typeof(IPlayerInventoryStore))
{
services.RemoveAt(i);
}
}
services.AddSingleton<IPlayerInventoryStore, InventoryStoreAlwaysMissing>();
});
});
var client = factory.CreateClient();
// Act
var response = await client.PostAsJsonAsync(
$"/game/players/{PlayerId}/craft",
Request("refine_scrap_standard"));
// Assert
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
}
private static void SeedStack(InMemoryWebApplicationFactory factory, string itemId, int quantity) private static void SeedStack(InMemoryWebApplicationFactory factory, string itemId, int quantity)
{ {
var itemRegistry = factory.Services.GetRequiredService<IItemDefinitionRegistry>(); var itemRegistry = factory.Services.GetRequiredService<IItemDefinitionRegistry>();
@ -245,4 +299,24 @@ public sealed class PlayerCraftApiTests
return total; return total;
} }
private sealed class InventoryStoreAlwaysMissing : IPlayerInventoryStore
{
public bool TryGetSnapshot(string playerId, out PlayerInventorySnapshot snapshot)
{
snapshot = PlayerInventorySnapshot.Empty();
return false;
}
public bool TryReplaceSnapshot(string playerId, PlayerInventorySnapshot snapshot) => false;
public bool TryMutateSnapshot(
string playerId,
Func<PlayerInventorySnapshot, PlayerInventoryMutationWrite> mutator,
out PlayerInventorySnapshot result)
{
result = PlayerInventorySnapshot.Empty();
return false;
}
}
} }

View File

@ -3,7 +3,7 @@ using System.Text.Json.Serialization;
namespace NeonSprawl.Server.Game.Crafting; namespace NeonSprawl.Server.Game.Crafting;
/// <summary>POST body for <c>POST /game/players/{{id}}/craft</c> (NEO-70).</summary> /// <summary>POST body for <c>POST /game/players/{{id}}/craft</c> (NEO-70).</summary>
public sealed class CraftRequest public sealed class PlayerCraftRequest
{ {
public const int CurrentSchemaVersion = 1; public const int CurrentSchemaVersion = 1;

View File

@ -12,13 +12,13 @@ public static class PlayerCraftApi
{ {
app.MapPost( app.MapPost(
"/game/players/{id}/craft", "/game/players/{id}/craft",
(string id, CraftRequest? body, IPositionStateStore positions, (string id, PlayerCraftRequest? body, IPositionStateStore positions,
IRecipeDefinitionRegistry recipeRegistry, IItemDefinitionRegistry itemRegistry, IRecipeDefinitionRegistry recipeRegistry, IItemDefinitionRegistry itemRegistry,
IPlayerInventoryStore inventoryStore, ISkillDefinitionRegistry skillRegistry, IPlayerInventoryStore inventoryStore, ISkillDefinitionRegistry skillRegistry,
IPlayerSkillProgressionStore xpStore, ISkillLevelCurve levelCurve, IPlayerSkillProgressionStore xpStore, ISkillLevelCurve levelCurve,
PerkUnlockEngine perkUnlockEngine) => PerkUnlockEngine perkUnlockEngine) =>
{ {
if (body is null || body.SchemaVersion != CraftRequest.CurrentSchemaVersion) if (body is null || body.SchemaVersion != PlayerCraftRequest.CurrentSchemaVersion)
{ {
return Results.BadRequest(); return Results.BadRequest();
} }

View File

@ -101,7 +101,7 @@ curl -sS -i "http://localhost:5253/game/world/recipe-definitions"
| **`progression_store_missing`** | Skill progression store could not apply XP (compensating inventory rollback). | | **`progression_store_missing`** | Skill progression store could not apply XP (compensating inventory rollback). |
| **`inventory_store_missing`** | Player inventory store could not read or write. | | **`inventory_store_missing`** | Player inventory store could not read or write. |
**POST** body (**`schemaVersion` 1**): **`recipeId`**, optional **`quantity`** (defaults to **1**). Response: **`success`**, **`reasonCode`** (`null` on success), **`inputsConsumed`**, **`outputsGranted`**, **`xpGrantSummary`** on success. **400** on schema mismatch or empty **`recipeId`**; **404** on unknown player or store-missing codes; **200** with **`success: false`** + stable deny codes on rule failure. **POST** body (**`schemaVersion` 1**): **`recipeId`**, optional **`quantity`** (defaults to **1** when omitted). Wire request type: **`PlayerCraftRequest`**. Response: **`success`**, **`reasonCode`** (`null` on success), **`inputsConsumed`**, **`outputsGranted`**, **`xpGrantSummary`** on success. **400** on schema mismatch or empty **`recipeId`**; **404** on unknown player or store-missing codes; **200** with **`success: false`** + stable deny codes on rule failure.
```bash ```bash
curl -sS -X POST "http://localhost:5253/game/players/dev-local-1/craft" \ curl -sS -X POST "http://localhost:5253/game/players/dev-local-1/craft" \
@ -109,6 +109,12 @@ curl -sS -X POST "http://localhost:5253/game/players/dev-local-1/craft" \
-d '{"schemaVersion":1,"recipeId":"refine_scrap_standard"}' -d '{"schemaVersion":1,"recipeId":"refine_scrap_standard"}'
``` ```
Sample **200** success body (after seeding **5×** **`scrap_metal_bulk`** via gather or inventory POST):
```json
{"schemaVersion":1,"success":true,"inputsConsumed":[{"itemId":"scrap_metal_bulk","quantity":5}],"outputsGranted":[{"itemId":"refined_plate_stock","quantity":1}],"xpGrantSummary":{"skillId":"refine","amount":10,"sourceKind":"activity"}}
```
Plan: [NEO-69 implementation plan](../../docs/plans/NEO-69-implementation-plan.md); [NEO-70 implementation plan](../../docs/plans/NEO-70-implementation-plan.md). Bruno: `bruno/neon-sprawl-server/craft/`. Telemetry hook sites (**`item_crafted`**, **`craft_failed`**) are **NEO-71**. Plan: [NEO-69 implementation plan](../../docs/plans/NEO-69-implementation-plan.md); [NEO-70 implementation plan](../../docs/plans/NEO-70-implementation-plan.md). Bruno: `bruno/neon-sprawl-server/craft/`. Telemetry hook sites (**`item_crafted`**, **`craft_failed`**) are **NEO-71**.
## Resource node definitions (NEO-60) ## Resource node definitions (NEO-60)