neon-sprawl/docs/reviews/2026-05-24-NEO-69.md

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	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 · E3M2-05	Matches — acceptance criteria checked; landed note present.
docs/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	Matches — E3.M2 row notes NEO-69 landed.
docs/decomposition/modules/module_dependency_register.md	Matches — E3.M2 Status updated to In Progress (was Planned).
docs/decomposition/modules/client_server_authority.md	Matches (planned) — server-authoritative inventory mutation; no client in this story.
NEO-62 / NEO-64 reference — GatherOperations	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 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.

3. Rollback order on output deny — Denied branch restored inputs before removing partial outputs, so TryAddStack could fail silently when bag was full. Done. Swapped to remove outputs first, then restore inputs (matches StoreMissing and XP-failure paths).

Verification

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.