don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEO-37: re-review notes for unordered skills contract update

pull/69/head
VinPropane 2026-05-06 21:54:13 -04:00
parent a3cbef063c
commit a89aadbb43
1 changed files with 12 additions and 4 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
docs/reviews/2026-05-06-NEO-37.md
View File

@ -1,11 +1,13 @@
# Code review — NEO-37 skill progression snapshot # Code review — NEO-37 skill progression snapshot
**Date:** 2026-05-06 **Date:** 2026-05-06 (initial); **re-reviewed** 2026-05-07.
**Scope:** Branch `NEO-37-skill-progression-snapshot-read-model`; `git` range `f89efdce..9cd96a7` (merge-base `f89efdce` with `origin/main` through HEAD). Issue **NEO-37**. **Scope:** Branch `NEO-37-skill-progression-snapshot-read-model`; `git` range `f89efdce..a3cbef0` (merge-base `f89efdce` with `origin/main` through current HEAD `a3cbef0`). Issue **NEO-37**.
**Base:** `origin/main` @ `f89efdce`. **Base:** `origin/main` @ `f89efdce`.
**Follow-up commits since first review:** `295eb34` (decomposition docs + review feedback); `a3cbef0` (**`skills`** non-contractual ordering, ordinal sort for stable serialization only, tests keyed by **`id`**, Bruno + README + plan + manual QA aligned).
## Verdict ## Verdict
**Approve.** **Approve.**
@ -40,10 +42,16 @@ Adds `GET /game/players/{id}/skill-progression` with server-authoritative defaul
- ~~**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**). - ~~**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**).
## Re-review notes
Explicit **unordered-by-contract** semantics for **`skills`** are coherent: they differentiate this read snapshot from **`GET …/skill-definitions`**, whose catalog order stays pinned for discovery. Server-side **`string.CompareOrdinal` sort before JSON** preserves stable payloads without inviting clients to depend on index ordering—documented on the response DTO, README, plan, manual QA, and `BuildSnapshot`.
Prior review items (module snapshot, alignment row, plan typo, Bruno-vs-order nit) are **addressed or superseded**; no new blocking findings.
## Verification ## Verification
- Already run in review: - Re-review (2026-05-07):
`cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~SkillProgressionSnapshotApiTests"` `cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~SkillProgressionSnapshotApiTests"` — passed (2 tests).
- Recommended before merge: full server test suite: - Recommended before merge: full server test suite:
`cd server && dotnet test` `cd server && dotnet test`