don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-06-NEO-38.md

5.8 KiB
Raw Permalink Blame History

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).