don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-24-NEO-70.md

6.6 KiB
Raw Permalink Blame History

Code review — NEO-70 craft HTTP + Bruno spine

Date: 2026-05-24
Scope: Branch NEO-70-craft-http-bruno-gather-refine-make-spine · commits b148db8–46393b8 vs origin/main
Base: origin/main

Verdict

Approve with nits — implementation matches the adopted plan and E3M2-06 acceptance criteria; ready to merge after optional follow-ups below.

Summary

The branch adds POST /game/players/{id}/craft via PlayerCraftApi and versioned wire DTOs in CraftWireDtos.cs, mapping request → CraftOperations.TryCraft → CraftResponse. HTTP behavior follows NEO-55 precedent: known-player gate, 400 on contract errors, 404 on unknown player or store-missing codes, 200 + structured deny for rule failures. Eight AAA integration tests cover success, major deny paths, and error statuses. Bruno craft/ folder documents the full gather→refine→make spine (11 scrap from four nodes → refine_scrap_efficient → make_field_stim_mk0) plus five deny scenarios. Docs (plan, module snapshot, backlog E3M2-06, alignment table, README) are updated and cross-link NEO-74. Risk is low for prototype scope; gaps are mostly deferred store-missing HTTP tests (explicitly allowed by plan) and minor naming / Bruno isolation nits.

Documentation checked

Document Result
docs/plans/NEO-70-implementation-plan.md Matches — CraftResult-only wire response, quantity default 1, HTTP status mapping, Bruno spine math (11 scrap + efficient refine), deny matrix, no manual QA doc (kickoff), NEO-74 cross-link, acceptance checklist complete.
docs/plans/E3M2-prototype-backlog.md · E3M2-06 Matches — acceptance criteria checked; landed note present. Generic bullet “Add manual QA when craft HTTP lands” remains (NEO-70 opted out per kickoff — see Nits).
docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md Matches — implementation snapshot includes NEO-70 craft HTTP + wire CraftResponse field table.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E3.M2 row notes NEO-70 landed; Bruno path cited.
docs/decomposition/modules/client_server_authority.md Matches — server-authoritative craft mutation; client deferred to NEO-74.
NEO-55 reference — PlayerInventoryApi Matches — same position gate, schemaVersion 400, store-missing → 404, rule deny → 200 + structured body.
Full-stack epic decomposition Matches — NEO-74 documented as client counterpart; no claim of prototype-complete player-visible craft.

Register/tracking: E3.M2 In Progress is correct; no register bump required until Slice 3 closes (NEO-71).

Blocking issues

None.

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). 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 doc’s 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. 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). Done. folder.bru documents run order; insufficient-materials request is seq 1 with pre-request empty-bag guard; spine moved to seq 5.

Nits

  • Nit: 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). 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. Done.

Verification

cd server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerCraftApiTests"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerCraftApiTests|FullyQualifiedName~CraftOperationsTests"

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.

Manual (Bruno):

  • Start server; run bruno/neon-sprawl-server/craft/Post craft gather refine make spine.bru against environments/Local.bru — 200, success: true, field_stim_mk0 in inventory, refine XP ≥ 20.
  • Run deny requests in craft/ folder (inventory full / invalid quantity use inventory POST for setup only — per plan).