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-69.md

5.5 KiB
Raw Blame History

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

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.

  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

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.