# Code review — NEO-40 (skill progression telemetry hook sites)

**Date:** 2026-05-10

**Scope:** Branch `NEO-40-skill-xp-level-up-telemetry-hook-sites` vs `origin/main` (commits through `effd234`).

**Base:** `origin/main`

**Follow-up:** Suggestions and nits below are **done** (strikethrough + **Done.**).

## Verdict

**Approve**

## Summary

The branch adds comment-only telemetry hook markers on the authoritative `POST …/skill-progression` path after a successful `TryApplyXpDelta`, plus a second marker on the level-up branch. Behavior and JSON contracts are unchanged; documentation (implementation plan, manual QA, E2.M2 module note, Bruno doc string, implementation alignment table E2.M2 row) matches the comments-only kickoff decision. Server tests pass.

## Documentation checked

| Path | Result |
|------|--------|
| `docs/plans/NEO-40-implementation-plan.md` | **Matches** — hook sites after successful apply + on `nextLevel > prevLevel`; comments-only; no `ILogger`; time-to-first-level-up called out; single authoritative layer (HTTP handler). |
| `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` | **Matches** — NEO-40 landed paragraph + backlog narrowed to NEO-41–NEO-43. |
| `docs/decomposition/modules/module_dependency_register.md` | **N/A** for diff content (no table edit); E2.M2 row unchanged, still appropriate. |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E2.M2 row includes **NEO-40 landed** (hook sites + plan + manual QA links); follow-on line removed. |
| `docs/decomposition/modules/contracts.md` / `client_server_authority.md` / `data_and_ops_policy.md` | **N/A** — no contract or authority change; telemetry ingest remains E9.M1 backlog, consistent with comments. |
| `docs/manual-qa/NEO-40.md` | **Matches** — API parity + comment spot-check aligns with plan. |

~~**Register / tracking table:** After merge, update the E2.M2 cell in `documentation_and_implementation_alignment.md` to a “**NEO-40 landed:** …” bullet (and trim the old “Follow-on: NEO-40” line) so it matches `E2_M2_XpAwardAndLevelEngine.md`.~~ **Done.** (E2.M2 snapshot + Plans column updated in-repo.)

## Blocking issues

(none)

## Suggestions

1. ~~**`documentation_and_implementation_alignment.md` (E2.M2 row):** Replace the remaining “Follow-on: NEO-40” wording with a landed summary (hook sites in `SkillProgressionSnapshotApi`, link to `NEO-40` plan + manual QA), mirroring the style used for NEO-37–NEO-39 in the same cell. This satisfies the module doc’s instruction to keep that table in sync.~~ **Done.**

## Nits

- ~~**Nit:** The implementation plan’s technical approach for `level_up` mentions “deltas” in the payload sketch; the in-code comment lists `playerId`, `skillId`, `previousLevel`, `newLevel` but not XP/total deltas. Optional one-word addition (`prevXp`/`newXp` or total XP) if you want comment parity with the plan.~~ **Done.** (`SkillProgressionSnapshotApi` `level_up` hook comment lists `prevXp`, `newXp`.)

## Verification

- `dotnet test NeonSprawl.sln` — **Passed** (133 tests) on review machine.
- Optional: follow `docs/manual-qa/NEO-40.md` curl steps against a running server.