# 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) | **Matches** — E3.M2 **Status** updated to **In Progress** (was **Planned**). |
| [`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.~~ **Done.** E3.M2 **Status** is **In Progress** in `module_dependency_register.md`.

## 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.~~ **Done.** Verification reads moved to **Assert** in all `CraftOperationsTests` methods.

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.~~ **Done.** `TryCraft_RefineScrapStandard_AtQuantityTwo_ShouldScaleInputsAndOutputs`.

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.~~ **Done.** Input loop tracks **`inputsRemoved`** and calls **`CompensatingRestoreInputs`** on mid-loop deny.

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.~~ **Done.** Two tests: bag-full deny for bag item; equipment route succeeds when bag full.

## Nits

- ~~Nit: **`module_dependency_register.md`** E3.M2 **Status** (**Planned**) lags **`E3_M2_RefinementAndRecipeExecution.md`** (**In Progress**).~~ **Done.**

- ~~Nit: No test for whitespace-only **`recipeId`** (maps to **`unknown_recipe`** via trim — fine behavior, low priority).~~ **Done.** `TryCraft_ForWhitespaceRecipeId_ShouldDenyWithUnknownRecipe`.

- Nit: Compensating helpers discard rollback outcomes (`_ = TryAddStack` / `TryRemoveStack`) — same as gather engine; acceptable for prototype.

## Bugbot follow-up (PR #103)

1. ~~**Output pre-flight ignores input removal** — simulate output placement on snapshot with inputs still present; false `inventory_full` when input removal would free slots.~~ **Done.** Pre-flight now **`TrySimulateRemoveStack`** all inputs on clone before output simulation; test **`TryCraft_WhenBagFullWithScrapInputs_ShouldSucceedByFreeingInputSlots`**.

2. ~~**Missing output rollback in store-missing branch** — output-add `StoreMissing` restored inputs but not partial outputs.~~ **Done.** `CompensatingRemoveOutputs` added before input restore on that path.

## 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).

**Results (post-review follow-up):** **11** `CraftOperationsTests` + **2** new `TrySimulateAddStack` tests pass; full suite **328** tests pass.

Manual: none required for engine-only story — NEO-70 owns HTTP / Bruno verification.