52 lines
3.6 KiB
Markdown
52 lines
3.6 KiB
Markdown
# 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); **`skills` array order is not contractual** — clients key by **`id`**. Integration-style tests exercise **404**, frozen trio membership + defaults (**order-independent**), and Bruno no longer asserts sort order on the progression array. README, manual QA, and implementation plan document the same contract. 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, **`skills` unordered-by-contract** (+ optional stable ordinal sort server-side only), 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` — **matches** story placement and NEO-37 delivery in **Implementation snapshot**; full engine responsibilities in the module body remain description of the target module (NEO-38+). **Updated** after review (2026-05-07) so tracking does not drift.
|
|
|
|
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches** (E2.M2 row **In Progress**, NEO-37 landed narrative + follow-on backlog). **Updated** after review.
|
|
|
|
- `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.~~ **Done.**
|
|
|
|
2. ~~**`docs/plans/NEO-37-implementation-plan.md` typography:** Insert a missing space after the comma (“When grants land, prefer …”) under Open questions.~~ **Done.**
|
|
|
|
## 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.~~ **Superseded** — progression **`skills`** is explicitly **unordered by contract**; Bruno + C# tests key by **`id`** only (**2026-05-07**).
|
|
|
|
## 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`.
|