55 lines
4.3 KiB
Markdown
55 lines
4.3 KiB
Markdown
# 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`
|
||
**Follow-up:** Suggestions and actionable nits below are **done** (strikethrough + **Done.**).
|
||
|
||
## 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) | **Matches** — E2.M3 note refreshed (NEO-45 → NEO-49 landed; NEO-48 still planned). |
|
||
|
||
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.~~ **Done.** `module_dependency_register.md` E2.M3 note updated.
|
||
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).~~ **Done.** Documented in [NEO-49 plan](../plans/NEO-49-implementation-plan.md) **Decisions (post-review)**.
|
||
|
||
## 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.~~ **Done.** Hook-site summary on `PerkUnlockEvent` record; enum has its own one-line summary.
|
||
- 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.
|