53 lines
5.8 KiB
Markdown
53 lines
5.8 KiB
Markdown
# 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.~~ **Done.** (2026-05-07) — Implementation snapshot refreshed for **NEO-38** landed + remaining backlog (**NEO-39** onward). |
|
||
| `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).~~ **Done.** (2026-05-07) — E2.M2 row includes **NEO-38**; E2.M1 row references consumer grant validation. |
|
||
| `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).~~ **Done.** (2026-05-07) — **E2.M2 note** added (NEO-37/NEO-38 pointers). |
|
||
|
||
**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.~~ **Done.**
|
||
|
||
## 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`**.~~ **Done.**
|
||
|
||
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.~~ **Done.** (clarified in **`server/README.md`** grant section; optional future backlog called out.)
|
||
|
||
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.~~ **Done.** — `Post skill progression grant deny source kind.bru`; manual QA updated.
|
||
|
||
## 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.~~ **Done.** — GET/POST use **`trimmedId`** (+ empty ⇒ **404**) for **`TryGetPosition`**.
|
||
|
||
- ~~**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.~~ **Done.** — two **`// Act`** comment blocks (**write** / **read-back**).
|
||
|
||
## 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).
|