From b381cadf2366acc8c6064aeac4d67bcf10a804c7 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 17:17:48 -0400 Subject: [PATCH] NEO-69: Add code review for craft engine implementation. --- docs/reviews/2026-05-24-NEO-69.md | 62 +++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 docs/reviews/2026-05-24-NEO-69.md diff --git a/docs/reviews/2026-05-24-NEO-69.md b/docs/reviews/2026-05-24-NEO-69.md new file mode 100644 index 0000000..22b7b3d --- /dev/null +++ b/docs/reviews/2026-05-24-NEO-69.md @@ -0,0 +1,62 @@ +# Code review — NEO-69 CraftResult engine + +**Date:** 2026-05-24 +**Scope:** Branch `NEO-69-craftresult-engine-inputs-outputs-refine-xp` · commits `bba1415`–`308de6b` vs `main` +**Base:** `main` + +## Verdict + +**Approve with nits** — implementation matches the adopted plan and E3M2-05 acceptance criteria; ready for NEO-70 HTTP wiring after minor test-layout cleanup. + +## Summary + +The branch adds **`CraftOperations.TryCraft`** and supporting **`CraftResult`** / reason-code types under `server/NeonSprawl.Server/Game/Crafting/`, plus read-only inventory helpers **`GetHeldQuantity`** and **`TrySimulateAddStack`** on **`PlayerInventoryOperations`**. The orchestration flow follows the kickoff decisions: quantity validation, recipe resolve, input pre-flight, output simulation on a cloned snapshot (no input consumption when full), commit remove → add → refine XP, and compensating rollback on output or XP failure. Eight AAA integration tests cover the primary deny paths, equipment routing, and XP rollback. Docs (plan, module snapshot, backlog, alignment, README) are updated and E3M2-05 is marked landed. Risk is low for prototype scope; the main gaps are test AAA layout (verification reads in Act) and missing coverage for **`quantity > 1`** scaling. + +## Documentation checked + +| Document | Result | +|----------|--------| +| [`docs/plans/NEO-69-implementation-plan.md`](../plans/NEO-69-implementation-plan.md) | **Matches** — pre-flight output strategy, craft-layer reason codes, quantity parameter, XP rollback, engine-only scope, NEO-74 cross-link, acceptance checklist complete. Helper named **`TrySimulateAddStack`** instead of plan’s **`CanPlaceStack`** — equivalent behavior. | +| [`docs/plans/E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) · **E3M2-05** | **Matches** — acceptance criteria checked; landed note present. | +| [`docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) | **Matches** — implementation snapshot + **`CraftResult`** field table; NEO-69 craft engine documented. | +| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M2 row notes NEO-69 landed. | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M2 **Status** still **Planned** while module doc / alignment say **In Progress** (optional register bump when Slice 3 spine completes). | +| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** (planned) — server-authoritative inventory mutation; no client in this story. | +| [NEO-62 / NEO-64 reference — `GatherOperations`](../../server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs) | **Matches** — same **`SkillProgressionGrantOperations.TryApplyGrant`** + compensating rollback pattern; craft correctly uses **`TryApplyGrant`** (not void **`RefineActivitySkillXpGrant`**) so XP deny can roll back inventory. | +| Full-stack epic decomposition | **Matches** — NEO-74 client counterpart linked; no claim of prototype-complete player-visible craft. | + +Register/tracking: optional E3.M2 **Status** → **In Progress** in dependency register when authors next touch alignment docs. + +## Blocking issues + +None. + +## Suggestions + +1. **AAA — move verification reads to Assert** — In `CraftOperationsTests`, `TryGetSnapshot` / `GetXpTotals` after `TryCraft` sit in the **Act** block; [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert) requires outcome verification reads in **Assert** (Act = SUT invocation only). Mirror the rule even though `GatherOperationsTests` has the same legacy layout. + +2. **Test `quantity > 1` scaling** — Plan and kickoff adopt batch **`quantity`** with scaled I/O. Add one test (e.g. **`refine_scrap_standard`** at **`quantity: 2`**) asserting 10× scrap consumed and 2× plate granted. + +3. **Defensive rollback on partial input removal** — If **`TryRemoveStack`** fails mid-loop after earlier inputs were removed (e.g. **`inventory_store_missing`**), **`TryCraft`** returns deny without restoring already-removed inputs. Pre-flight makes **`Denied`** unlikely; consider compensating restore for symmetry with output/XP rollback paths. + +4. **Optional `TrySimulateAddStack` unit test** — Plan listed an optional dedicated test in **`PlayerInventoryOperationsTests`**; one bag-full vs equipment-route case would lock pre-flight simulation to **`TryAddStack`** behavior without a full craft scenario. + +## Nits + +- Nit: **`module_dependency_register.md`** E3.M2 **Status** (**Planned**) lags **`E3_M2_RefinementAndRecipeExecution.md`** (**In Progress**). + +- Nit: No test for whitespace-only **`recipeId`** (maps to **`unknown_recipe`** via trim — fine behavior, low priority). + +- Nit: Compensating helpers discard rollback outcomes (`_ = TryAddStack` / `TryRemoveStack`) — same as gather engine; acceptable for prototype. + +## Verification + +```bash +cd server +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~CraftOperationsTests" +dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj +``` + +**Results (2026-05-24):** all **8** `CraftOperationsTests` pass. Full suite reported **2** failures in `SkillProgressionGrantApiTests` (unrelated to this branch; NEO-69 diff does not touch that file). + +Manual: none required for engine-only story — NEO-70 owns HTTP / Bruno verification.