From a89aadbb4337bf8d98a1678ae945160290bc0a1f Mon Sep 17 00:00:00 2001
From: VinPropane <don@donmail.org>
Date: Wed, 6 May 2026 21:54:13 -0400
Subject: [PATCH] NEO-37: re-review notes for unordered skills contract update

---
 docs/reviews/2026-05-06-NEO-37.md | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/docs/reviews/2026-05-06-NEO-37.md b/docs/reviews/2026-05-06-NEO-37.md
index 65b745e..074feb4 100644
--- a/docs/reviews/2026-05-06-NEO-37.md
+++ b/docs/reviews/2026-05-06-NEO-37.md
@@ -1,11 +1,13 @@
 # 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`.
 
+**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
 
 **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**).
 
+## 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
 
-- Already run in review:  
-  `cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~SkillProgressionSnapshotApiTests"`
+- Re-review (2026-05-07):  
+  `cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~SkillProgressionSnapshotApiTests"` — passed (2 tests).
 
 - Recommended before merge: full server test suite:  
   `cd server && dotnet test`