From 9ed4034140963ec4991cfa5ba16fb566b4ccf297 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 6 May 2026 22:24:17 -0400 Subject: [PATCH] NEO-38: add code review (2026-05-06) --- docs/reviews/2026-05-06-NEO-38.md | 49 +++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 docs/reviews/2026-05-06-NEO-38.md diff --git a/docs/reviews/2026-05-06-NEO-38.md b/docs/reviews/2026-05-06-NEO-38.md new file mode 100644 index 0000000..74a00c3 --- /dev/null +++ b/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).