From 2be35c164060b45b6c746c4af027448f638560e9 Mon Sep 17 00:00:00 2001
From: VinPropane <don@donmail.org>
Date: Sun, 24 May 2026 18:30:46 -0400
Subject: [PATCH] NEO-71: Add code review for craft telemetry hook sites.

---
 docs/reviews/2026-05-24-NEO-71.md | 51 +++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 docs/reviews/2026-05-24-NEO-71.md

diff --git a/docs/reviews/2026-05-24-NEO-71.md b/docs/reviews/2026-05-24-NEO-71.md
new file mode 100644
index 0000000..1fe76f2
--- /dev/null
+++ b/docs/reviews/2026-05-24-NEO-71.md
@@ -0,0 +1,51 @@
+# Code review — NEO-71 craft telemetry hook sites
+
+**Date:** 2026-05-24  
+**Scope:** Branch `NEO-71-item-crafted-craft-failed-telemetry-hooks` · commits `194078d`–`2d47510` vs `origin/main`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits** — comment-only hook sites and documentation match the adopted plan and E3M2-07 acceptance criteria; ready to merge after optional `CraftResult.cs` wording tweak.
+
+## Summary
+
+The branch adds **comment-only E9.M1 telemetry hook sites** in **`CraftOperations.TryCraft`**: **`item_crafted`** immediately before the successful return (after refine XP grant), and **`craft_failed`** in the centralized private **`Deny`** helper covering all **`CraftReasonCodes`** paths including store-missing and post-mutation rollback denies. Hook style, payload field notes, and **`TODO(E9.M1): catalog emit`** markers follow NEO-64 / NEO-56 precedent. **`PlayerCraftApi`** correctly has no duplicate hooks. Docs (implementation plan, **`server/README.md`** craft telemetry subsection, E3_M2 module snapshot, E3M2-07 backlog, alignment register, dependency register **Ready** bump) are updated. No runtime or wire contract change; existing craft tests pass. Risk is negligible.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| [`docs/plans/NEO-71-implementation-plan.md`](../plans/NEO-71-implementation-plan.md) | **Matches** — engine-only hooks, central **`Deny`**, comments-only, no manual QA doc, **`time-to-first-craft`** derived at E9.M1, acceptance checklist complete. |
+| [`docs/plans/E3M2-prototype-backlog.md`](../plans/E3M2-prototype-backlog.md) · **E3M2-07** | **Matches** — acceptance criteria checked; landed note present. |
+| [`docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md`](../decomposition/modules/E3_M2_RefinementAndRecipeExecution.md) | **Matches** — NEO-71 landed line; telemetry deferral removed; module **Ready** (Slice 3 server backlog complete). |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M2 row notes NEO-71; E3M2-01–07 complete. |
+| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E3.M2 **Status** → **Ready**; E3M2-07 note updated. |
+| [`docs/decomposition/epics/epic_03_crafting_economy.md`](../decomposition/epics/epic_03_crafting_economy.md) · Slice 3 | **Matches** — **`item_crafted`**, **`craft_failed`** with reason documented; **`time-to-first-craft`** noted as derived (no separate hook). |
+| NEO-64 / NEO-56 reference — [`GatherOperations`](../../server/NeonSprawl.Server/Game/Gathering/GatherOperations.cs), [`PlayerInventoryOperations`](../../server/NeonSprawl.Server/Game/Items/PlayerInventoryOperations.cs) | **Matches** — same comment block structure, **`TODO(E9.M1)`**, central **`Deny`** for structured denies, engine-only anchor. |
+| Full-stack epic decomposition | **Matches** — no client counterpart (server comment-only); NEO-74 cited as Slice 5 craft UI; no claim of player-visible prototype completion. |
+
+Register/tracking: E3.M2 **Ready** bump is appropriate — Slice 3 server backlog E3M2-01–E3M2-07 complete per plan.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **`CraftResult.cs` summary tense** — Plan item 4 asked to confirm cross-reference wording after hooks land. File was not updated; summary still reads “NEO-71 telemetry hook sites **will live** in **`TryCraft`**”. Update to present tense (e.g. “live in **`CraftOperations.TryCraft`** (success path + **`Deny`**)”) so it matches landed hooks and the class summary on **`CraftOperations`**.
+
+## Nits
+
+- Nit: README craft telemetry subsection is a single dense paragraph; a two-bullet list mirroring the code hook sites (`item_crafted` / `craft_failed`) would match NEO-64 gather README scanability — optional only.
+
+## Verification
+
+```bash
+cd server
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~PlayerCraftApiTests|FullyQualifiedName~CraftOperationsTests"
+```
+
+**Results (2026-05-24):** **21** craft-related tests pass (`PlayerCraftApiTests` + `CraftOperationsTests`).
+
+Optional Bruno regression (behavior unchanged): run `bruno/neon-sprawl-server/craft/Post craft gather refine make spine.bru` against a running dev server.