NEO-49: add code review for perk_unlock telemetry hooks

docs/reviews/2026-05-17-NEO-49.md

@@ -0,0 +1,53 @@
+# Code review — NEO-49 perk_unlock telemetry hook sites
+
+**Date:** 2026-05-17  
+**Scope:** Branch `NEO-49-perk-unlock-telemetry-hook-sites` · commits `f3e48f4`–`38ef54c` vs `main` (includes unrelated `0d63965` chore: `.vscodecsdt/` gitignore)  
+**Base:** `main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-49 adds a **comments-only** E9.M1 **`perk_unlock`** hook anchor in `PerkUnlockEngine.TryUnlockPerks` immediately after `PerkUnlockEvent` instances are built and perks are persisted—matching the adopted single-anchor plan and Epic 2 Slice 4 telemetry vocabulary. Documentation updates (implementation plan, manual QA, `server/README.md`, E2.M3 module, alignment row) are consistent. No runtime, API, or test changes; **177** server tests pass. Low merge risk; E9.M1 ingest remains deferred.
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| [`docs/plans/NEO-49-implementation-plan.md`](../plans/NEO-49-implementation-plan.md) | **Matches** — single anchor in `TryUnlockPerks`, comments-only, `TODO(E9.M1)`, README + module + manual QA; acceptance checklist complete. |
+| [`docs/plans/NEO-47-implementation-plan.md`](../plans/NEO-47-implementation-plan.md) | **Matches** — hook does not alter engine contracts; unlock funnel unchanged; NEO-47 note on future `IPerkUnlockEventSink` still valid. |
+| [`docs/decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md`](../decomposition/modules/E2_M3_MasteryAndPerkUnlocks.md) | **Matches** — NEO-49 handoff section; telemetry bullet updated. |
+| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E2.M3 row cites NEO-49 hook + manual QA. |
+| [`docs/decomposition/epics/epic_02_skills_and_progression.md`](../decomposition/epics/epic_02_skills_and_progression.md) | **Matches** — Slice 4 lists `perk_unlock` telemetry hook; implementation is comment-only until E9.M1. |
+| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — no client or authority boundary change. |
+| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E2.M3 note still says **NEO-47 blocked by NEO-46** (stale vs `main`); alignment doc is current. Optional register refresh (see Suggestions). |
+
+Register/tracking: alignment table and E2.M3 module updated; register footnote not touched in this branch.
+
+## Blocking issues
+
+None.
+
+## Suggestions
+
+1. **Refresh `module_dependency_register` E2.M3 note** — The footnote still ends with “**NEO-47** blocked by NEO-46…” while NEO-46/47/49 are landed on this branch’s doc trail. Mirror the E2.M3 row in `documentation_and_implementation_alignment.md` (NEO-46 → NEO-47 → NEO-49) so the register does not contradict merge-ready `main` + this PR.
+2. **E9.M1 `source` semantics** — NEO-47 re-review noted path-auto unlocks triggered from `TrySelectBranch` may carry `PerkUnlockSource.LevelUp`. The hook comment already lists `BranchPick | LevelUp`; when wiring ingest, document whether retroactive path-auto from branch pick should remain `LevelUp` or gain a distinct catalog value (product/E9.M1 decision, not blocking NEO-49).
+
+## Nits
+
+- Nit: `PerkUnlockEvent.cs` XML summary sits on **`PerkUnlockSource`** enum, not the **`PerkUnlockEvent`** record—manual QA says “Open `PerkUnlockEvent.cs`”; moving the summary to the record (or duplicating a one-liner on the record) would match reader intent.
+- Nit: Branch includes chore commit `0d63965` (`.vscodecsdt/` gitignore)—harmless; already isolated if reviewers prefer a cherry-pick-only view of NEO-49 commits.
+
+## Verification
+
+```bash
+# From repo root — regression (177 tests at review)
+dotnet test NeonSprawl.sln
+
+# Manual QA checklist
+# docs/manual-qa/NEO-49.md
+```
+
+Confirm `PerkUnlockEngine.TryUnlockPerks` hook block cites **`perk_unlock`**, **`TODO(E9.M1)`**, payload fields, and no new `ILogger`/metrics calls.