NEO-37: code review snapshot for skill progression API branch

docs/reviews/2026-05-06-NEO-37.md

@@ -0,0 +1,51 @@
+# Code review — NEO-37 skill progression snapshot
+
+**Date:** 2026-05-06
+
+**Scope:** Branch `NEO-37-skill-progression-snapshot-read-model`; `git` range `f89efdce..9cd96a7` (merge-base `f89efdce` with `origin/main` through HEAD). Issue **NEO-37**.
+
+**Base:** `origin/main` @ `f89efdce`.
+
+## Verdict
+
+**Approve.**
+
+## Summary
+
+Adds `GET /game/players/{id}/skill-progression` with server-authoritative defaults (every registered skill row at `xp` 0, `level` 1), gated on `IPositionStateStore.TryGetPosition` like `CooldownSnapshotApi`/`HotbarLoadoutApi`. Response DTOs follow the NEO-36-style versioned envelope (`schemaVersion`, camelCase JSON names). Integration-style tests exercise 404 vs known dev player, exact Frozen Trio id order, and per-row defaults. Bruno mirrors the `skill-definitions` folder style; README, manual QA, and implementation plan are in place alongside a small clarification in `docs/plans/README.md` about canonical GitHub org links. Risk is low: read-only projection with no persistence or grant path yet.
+
+## Documentation checked
+
+- `docs/plans/NEO-37-implementation-plan.md` — **matches** route, player gate (`404`), JSON shape, ordering via `ISkillDefinitionRegistry.GetDefinitionsInIdOrder()`, Bruno/README/manual QA expectations, and deferral of persistence to NEO-38.
+
+- `docs/manual-qa/NEO-37.md` — **matches** curl steps and assertions.
+
+- `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — **partially matches** story placement (Slice 2, NEO-37); module body still describes the full XP engine responsibilities (grants, curves, events), which rightly remain **out of scope** for NEO-37. The implementation snapshot paragraph still states no implementation landed; **that text should be updated after merge** to reflect NEO-37 delivery (tracking note below).
+
+- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches** (E2.M2 row cites NEO-37); row **status / narrative after merge** may need tightening once this ships (same hygiene as E2.M2 page).
+
+- `docs/decomposition/modules/client_server_authority.md`, `contracts.md` — **N/A** for this additive read-model slice (no new cross-cutting gameplay authority beyond existing “known player via position store” pattern).
+
+## Blocking issues
+
+(none)
+
+## Suggestions
+
+1. **Documentation follow-up after merge:** Update `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` (implementation snapshot — remove or qualify “No implementation landed yet”) and confirm `documentation_and_implementation_alignment.md` E2.M2 row still reads correctly once NEO-37 is merged. Prefer a short PR stacked on this merge or the same sprint so tracking does not drift.
+
+2. **`docs/plans/NEO-37-implementation-plan.md` typography:** Insert a missing space after the comma (“When grants land, prefer …”) under Open questions.
+
+## Nits
+
+- **Nit:** Bruno `skills are ascending by id` matches `skill-definitions` folder style but is weaker than the C# tests for **exact catalog order**; if regressions swapped registry order without changing ids, lexical sort alone would not catch misuse of a different comparator. Acceptable Bruno scope; parity with sibling collection is coherent.
+
+## Verification
+
+- Already run in review:  
+  `cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~SkillProgressionSnapshotApiTests"`
+
+- Recommended before merge: full server test suite:  
+  `cd server && dotnet test`
+
+- Manual: steps in `docs/manual-qa/NEO-37.md`; optional Bruno `bruno/neon-sprawl-server/skill-progression/Get skill progression.bru` with `{{baseUrl}}` from `environments/Local.bru`.