NEO-38: add code review (2026-05-06)

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

@@ -0,0 +1,49 @@
+# Code review — NEO-38 (skill XP grant)
+
+**Date:** 2026-05-06
+
+**Scope:** Branch `NEO-38-apply-skill-xp-grant-allowlist-level-up` vs `origin/main` (four commits ending `bab7336`; working tree clean at review time).
+
+**Base:** `origin/main`
+
+---
+
+## Verdict
+
+**Approve**
+
+## Summary
+
+The branch implements `POST /game/players/{id}/skill-progression` with versioned bodies, stable deny reason codes (`unknown_skill`, `source_kind_not_allowed`, `invalid_amount`), hotbar-style **200 + `granted: false`** denies with echoed **`progression`**, dual persistence (**`InMemoryPlayerSkillProgressionStore`** / **`PostgresPlayerSkillProgressionStore`** + **`V003__player_skill_progression.sql`**), and GET snapshot merge of registry + stored XP via **`SkillLevelCurvePlaceholder`**. Integration tests (**`SkillProgressionGrantApiTests`**), Postgres persistence (**`SkillProgressionGrantPersistenceIntegrationTests`**), Bruno, **`server/README.md`**, **`docs/manual-qa/NEO-38.md`**, and **`docs/plans/NEO-38-implementation-plan.md`** are in place. **`dotnet test`** for `NeonSprawl.Server.Tests` passed (123 tests). Overall risk is **low** with normal prototype caveats (placeholder curve, synchronous ADO.NET in stores).
+
+## Documentation checked
+
+| Document | Assessment |
+|---------|-------------|
+| `docs/plans/NEO-38-implementation-plan.md` | **Matches** — routing, gates, persistence shape, denial pattern, Bruno/README/manual QA covered by implementation. Minor filename drift (`SkillProgressionServiceCollectionExtensions.cs` shipped vs plan’s wording; benign). |
+| `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` | **Partially matches** — responsibilities (validate skill + `sourceKind`, derive level, surface level-ups) align. The page’s **Implementation snapshot** paragraph still reads as if **NEO-38** is backlog (“Still backlog … applying XP grants”). **Should update after merge** (or in same PR) so the slice-2 narrative matches landed code — see Suggestions. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E2.M2 tracking row still lists **NEO-38** under “Follow-on” only. Merge should refresh that row (**NEO-38** landed paths, README anchors, Bruno) per this doc’s own rule (**§ Status column / tracking table**). E2.M1 row’s **Follow-on (E2.M2): XP grant validation** line is stale once NEO-38 merges (**should fix** in same sweep). |
+| `docs/decomposition/modules/module_dependency_register.md` | Referenced implicitly via alignment doc; register row likely needs the same E2.M2 status/snapshot tweak when NEO-38 merges — treat as **post-merge bookkeeping** alongside the alignment table (not reviewed line-by-line in this pass). |
+
+**Register / tracking table:** yes — **`documentation_and_implementation_alignment.md`** (and **`E2_M2_…` Implementation snapshot**) should be updated so Status / snapshot bullets do not disagree with merged code.
+
+## Blocking issues
+
+_None._
+
+## Suggestions
+
+1. **Decomposition freshness (same PR or immediate follow-up):** Update **`E2_M2_XpAwardAndLevelEngine.md`** implementation snapshot plus **`documentation_and_implementation_alignment.md`** E2.M2 (and tighten E2.M1 “follow-on”) so they describe **NEO-38** as landed with links to **`NEO-38-implementation-plan.md`**, **`docs/manual-qa/NEO-38.md`**, and **`server/README.md`** grant section — matching the checklist in **`planning-implementation-docs`**.
+2. **`levelUps` semantics for multi-step jumps:** A single grant that crosses more than one level boundary (e.g. +250 XP under `level = 1 + floor(xp/100)`) emits **one** `levelUps` row `{ previousLevel: 1, newLevel: 3 }`. If callers need one entry per crossed level later, note that explicitly in README or backlog; otherwise a one-line clarification that “span reflects final levels after grant” avoids ambiguity.
+3. **Bruno coverage:** **`Post skill progression grant.bru`** exercises the happy path only; **`docs/manual-qa/NEO-38.md`** covers deny via curl. Optional: add a second Bruno request for **`source_kind_not_allowed`** to mirror hotbar-style smoke patterns.
+
+## Nits
+
+- **Nit:** `MapPost` uses `positions.TryGetPosition(id, …)` with the raw route parameter while snapshots use `trimmedId`; `MapGet` does the same. Harmless if ids are never padded with whitespace; using the same trimmed key everywhere would be slightly tidier.
+- **Nit:** **`SkillProgressionGrantPersistenceIntegrationTests`** uses a broad `// Act` block that includes opening a second factory and GET — acceptable for an integration test, but strict AAA purists might split “write” vs “read-back” with clearer phase comments.
+
+## Verification
+
+- `cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj` — **passed** (123 tests) at review time.
+- Optional: run **`docs/manual-qa/NEO-38.md`** curl steps against a local host; run Bruno folder `bruno/neon-sprawl-server/skill-progression/` for smoke.
+- With Postgres: confirm **`V003__player_skill_progression.sql`** applied in real migrations flow if something other than bootstrap applies DDL in your environment (app uses bootstrap + test harness re-applies file — OK for tests).