diff --git a/docs/reviews/2026-05-17-NEO-49.md b/docs/reviews/2026-05-17-NEO-49.md new file mode 100644 index 0000000..d897b8a --- /dev/null +++ b/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.