NEO-121: Add code review for quest telemetry hook sites.

docs/reviews/2026-06-07-NEO-121.md

@@ -0,0 +1,57 @@
+# Code review — NEO-121 (E7M1-10)
+
+**Date:** 2026-06-07  
+**Scope:** Branch `NEO-121-quest-telemetry-hooks` vs `origin/main` — commits `72b98a6` … `705a7cd`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-121 adds **comment-only** E9.M1 telemetry hook sites to **`QuestStateOperations`** for Epic 7 Slice 1 events **`quest_start`**, **`quest_step_complete`**, **`quest_complete`**, and **`quest_accept_denied`**. Hooks are placed at mutation commit sites (after successful store writes) and accept-deny paths funnel through a new **`DenyAccept`** helper (mirroring NEO-56 **`inventory_transfer_denied`**). **`DenyFromProgressSnapshotAccept`** ensures activate-failure rereads also hit the deny hook. No runtime behavior, wire contract, or API surface changes — only comments plus accept-path deny routing that still delegates to **`Deny`**. Documentation is thorough: implementation plan reconciliation, **`server/README.md`** event table, E7.M1 module snapshot, alignment register, backlog checkboxes, and **`docs/manual-qa/NEO-121.md`**. Server-only infrastructure slice; Godot HUD correctly deferred to NEO-122. Risk is negligible.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-121-implementation-plan.md` | **Matches** — kickoff decisions adopted (engine-only anchors, deny helper, idempotent complete exclusion); acceptance checklist checked; reconciliation accurate. |
+| `docs/plans/E7M1-prototype-backlog.md` (E7M1-10) | **Matches** — acceptance checkboxes checked; landed note + README link. |
+| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — NEO-121 implementation snapshot + Risks and telemetry bullet. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7M1-10 / NEO-121 landed note appended. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7M1-10 note appended; row stays **Planned** until NEO-123 (per plan). |
+| `server/README.md` | **Matches** — Quest telemetry hooks subsection with event table and delegation note. |
+| `docs/manual-qa/NEO-121.md` | **Matches** — comment-only verification checklist; optional Bruno/regression steps. |
+| Full-stack epic decomposition | **N/A** — infrastructure-only; plan explicitly documents no client counterpart. |
+
+**Register / tracking:** E7.M1 correctly remains **Planned** until client capstone NEO-123; E7M1-10 completion noted in register note only — no further status change required.
+
+## Blocking issues
+
+(none)
+
+## Suggestions
+
+(none)
+
+## Nits
+
+1. **Nit:** Plan technical approach lists optional **`schemaVersion`** in **`quest_start`** planned payload; the inline comment omits it. Harmless for comment-only work; consider adding for parity when touching the file again.
+
+2. **Nit:** **`DenyFromProgressSnapshotAccept`** duplicates **`DenyFromProgressSnapshot`** except for **`DenyAccept`** vs **`Deny`** — intentional funneling, but E9.M1 wiring may want **`playerId`** / **`questId`** threaded into **`DenyAccept`** (comment already documents those fields; implementer will need caller context).
+
+## Verification
+
+```bash
+# Full regression (691 tests passed at review time)
+dotnet test NeonSprawl.sln
+
+# Quest-focused subset
+dotnet test NeonSprawl.sln --filter "FullyQualifiedName~Quest"
+```
+
+Manual (per `docs/manual-qa/NEO-121.md`):
+
+- Walk **`QuestStateOperations.cs`** hook comment blocks and confirm no duplicate hooks in **`QuestAcceptApi`**, **`QuestProgressApi`**, or **`QuestObjectiveWiring`**.
+- Optional Bruno sanity in `bruno/neon-sprawl-server/quest-progress/` — accept happy path + duplicate deny unchanged.