diff --git a/bruno/neon-sprawl-server/skill-progression/Get skill progression.bru b/bruno/neon-sprawl-server/skill-progression/Get skill progression.bru index f6fec84..86fc043 100644 --- a/bruno/neon-sprawl-server/skill-progression/Get skill progression.bru +++ b/bruno/neon-sprawl-server/skill-progression/Get skill progression.bru @@ -25,13 +25,6 @@ tests { expect(body.skills.length).to.equal(3); }); - test("skills are ascending by id (ordinal)", function () { - const body = res.getBody(); - const ids = body.skills.map((x) => x.id); - const sorted = [...ids].sort((a, b) => (a < b ? -1 : a > b ? 1 : 0)); - expect(ids).to.eql(sorted); - }); - test("frozen prototype trio with default xp and level", function () { const body = res.getBody(); const intrusion = body.skills.find((x) => x.id === "intrusion"); diff --git a/docs/manual-qa/NEO-37.md b/docs/manual-qa/NEO-37.md index 5e668f5..de9e27d 100644 --- a/docs/manual-qa/NEO-37.md +++ b/docs/manual-qa/NEO-37.md @@ -11,7 +11,7 @@ ## Preconditions - Server running with default dev player seeded (same as other **`/game/players/dev-local-1/...`** flows). -- Skill catalog loaded (prototype trio); progression rows mirror registry order. +- Skill catalog loaded (prototype trio); **`skills`** is key-by-**`id`** (array order is not contractual). ## Checklist @@ -22,6 +22,6 @@ curl -sS -i "http://localhost:5253/game/players/dev-local-1/skill-progression" ``` -3. Parse JSON — **`schemaVersion`** **1**, **`playerId`** **`dev-local-1`**, **`skills`** length **3**, **`id`** order **`intrusion`**, **`refine`**, **`salvage`**; each row **`xp`** **0**, **`level`** **1**. +3. Parse JSON — **`schemaVersion`** **1**, **`playerId`** **`dev-local-1`**, **`skills`** length **3** with **`id`**s **`intrusion`**, **`refine`**, **`salvage`** (any order); each row **`xp`** **0**, **`level`** **1**. 4. Unknown player — **`GET /game/players/missing-player/skill-progression`** — expect **404**. 5. Optional: run **`bruno/neon-sprawl-server/skill-progression/Get skill progression.bru`** (see `environments/Local.bru` for **`{{baseUrl}}`**). diff --git a/docs/plans/NEO-37-implementation-plan.md b/docs/plans/NEO-37-implementation-plan.md index 60514d2..49920e3 100644 --- a/docs/plans/NEO-37-implementation-plan.md +++ b/docs/plans/NEO-37-implementation-plan.md @@ -17,7 +17,7 @@ ## Goal, scope, and out-of-scope -**Goal:** Ship a versioned **GET** read model for per-player skill XP and level for every `SkillDef` id from **`ISkillDefinitionRegistry`**, bootstrapping **xp = 0** and **level = 1** before any grants exist. Match **NEO-36** JSON envelope patterns (`schemaVersion`, ordered rows, `System.Text.Json` property names). +**Goal:** Ship a versioned **GET** read model for per-player skill XP and level for every `SkillDef` id from **`ISkillDefinitionRegistry`**, bootstrapping **xp = 0** and **level = 1** before any grants exist. Match **NEO-36** envelope patterns (`schemaVersion`, `System.Text.Json` property names). **`skills`** is semantically **unordered** — clients **key by `id`** (unlike the world skill catalog projection where id order is pinned for discovery). **In scope (from Linear):** @@ -35,7 +35,7 @@ ## Acceptance criteria checklist - [x] Versioned JSON response for **`GET /game/players/{id}/skill-progression`** (`schemaVersion` **1**). -- [x] One row per registered skill id; **`xp`** = **0**, **`level`** = **1** for all rows in this story; **`skills`** ordered by **`ISkillDefinitionRegistry.GetDefinitionsInIdOrder()`**. +- [x] One row per registered skill id; **`xp`** = **0**, **`level`** = **1** for all rows in this story; consumers treat **`skills`** as unordered and key by **`id`** (wire order not contractual). - [x] Automated tests (AAA); Bruno folder mirroring NEO-36 (`skill-definitions` conventions). - [x] **`server/README.md`** section documenting the endpoint (curl + pointers to plan / manual QA / Bruno). @@ -45,11 +45,11 @@ 2. **Player gate:** **`404`** when **`!IPositionStateStore.TryGetPosition(id, …)`**, matching **`CooldownSnapshotApi`** (known player/session only). -3. **Response:** Top-level **`schemaVersion`** (**1**, const on a response type like NEO-36’s **`SkillDefinitionsListResponse.CurrentSchemaVersion`**), **`playerId`** (**`id.Trim()`**, consistent with **`HotbarLoadoutResponse`**), and **`skills`**: array of objects **`id`** (skill def id), **`xp`** (int, **0**), **`level`** (int, **1**). Do **not** duplicate catalog fields (`displayName`, etc.); clients join with **`GET /game/world/skill-definitions`** if needed. +3. **Response:** Top-level **`schemaVersion`** (**1**, const on a response type like NEO-36’s **`SkillDefinitionsListResponse.CurrentSchemaVersion`**), **`playerId`** (**`id.Trim()`**, consistent with **`HotbarLoadoutResponse`**), and **`skills`**: array of objects **`id`** (skill def id), **`xp`** (int, **0**), **`level`** (int, **1**). **Array order is not part of the public contract** — clients index by **`id`**. Do **not** duplicate catalog fields (`displayName`, etc.); clients join with **`GET /game/world/skill-definitions`** if needed. -4. **Data source:** For NEO-37, build the array **only** from the registry (no persistence). **NEO-38** will introduce stored progression and merge/override defaults; until then there is **no separate store interface** unless implementation discovers a need for testing seams—prefer a small private/static builder method next to the route to keep diff small. +4. **Data source:** For NEO-37, build rows from **`ISkillDefinitionRegistry`** (no persistence). **Sort by `id` (ordinal) before serialization** for stable diffs only — semantics remain keyed by **`id`**, not by array index. **NEO-38** will introduce stored progression and merge/override defaults; until then there is **no separate store interface** unless implementation discovers a need for testing seams—prefer a small private/static builder method next to the route to keep diff small. -5. **Bruno:** Add **`bruno/neon-sprawl-server/skill-progression/`** with **`Get skill progression.bru`** (optional **`folder.bru`** matching sibling folders). Use **`{{baseUrl}}`**; tests assert **200**, JSON, **`schemaVersion`**, **`playerId`**, **`skills`** length **3**, ordinal **id** order (`intrusion`, `refine`, `salvage`), and **`xp`/`level`** defaults on one row. +5. **Bruno:** **`bruno/neon-sprawl-server/skill-progression/`** with **`Get skill progression.bru`**. Tests assert **200**, **`schemaVersion`**, **`skills`** length **3**, frozen ids present + **`xp`/`level`** defaults — **do not assert array order**. 6. **README:** Add a subsection after the NEO-36 skill definitions block — curl example for **`dev-local-1`** (or generic `{id}`) and links to this plan, **`docs/manual-qa/NEO-37.md`**, and Bruno path. @@ -59,7 +59,7 @@ |------|---------| | `server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs` | `Map*` extension registering the GET route; position check + registry → JSON. | | `server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs` | Versioned response + per-skill row DTOs (`schemaVersion`, `playerId`, `skills`). | -| `server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs` | **`InMemoryWebApplicationFactory`**: success for **`dev-local-1`**; **404** for unknown player; **ReadFromJsonAsync** asserts schema, order, defaults. | +| `server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs` | **`InMemoryWebApplicationFactory`**: success for **`dev-local-1`**; **404** for unknown player; **ReadFromJsonAsync** asserts schema, trio by **`id`** (map), defaults (**order-independent**). | | `bruno/neon-sprawl-server/skill-progression/Get skill progression.bru` | Manual / Bruno verification (mirror **`skill-definitions/Get skill definitions.bru`** style). | | `bruno/neon-sprawl-server/skill-progression/folder.bru` | Optional; align with Bruno folder metadata used beside **`skill-definitions`**. | | `docs/manual-qa/NEO-37.md` | Short checklist: run server, GET with known vs unknown player id. | @@ -75,7 +75,7 @@ | Test file | What it covers | |-----------|------------------| -| `server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs` | **`GET`** returns **404** for **`missing-player`** (or equivalent unknown id consistent with **`CooldownSnapshotApiTests`**). **`GET`** for **`dev-local-1`** returns **200**, **`schemaVersion`** **1**, **`playerId`** matches, **`skills`** ids **`intrusion`**, **`refine`**, **`salvage`**, each **`xp`** **0**, **`level`** **1**. **AAA** comments per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert). | +| `server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs` | **`GET`** returns **404** for **`missing-player`**. **`GET`** for **`dev-local-1`** returns **200**; body has those three **`id`**s with **`xp`** **0**, **`level`** **1**, **without asserting JSON array order**. **AAA** comments per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert). | Bruno scripts complement automated coverage; they do not replace it. diff --git a/docs/reviews/2026-05-06-NEO-37.md b/docs/reviews/2026-05-06-NEO-37.md index b264cf7..65b745e 100644 --- a/docs/reviews/2026-05-06-NEO-37.md +++ b/docs/reviews/2026-05-06-NEO-37.md @@ -12,11 +12,11 @@ ## 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). Integration-style tests exercise 404 vs known dev player, exact Frozen Trio id order, and per-row defaults. Bruno mirrors the `skill-definitions` folder style; README, manual QA, and implementation plan are in place alongside a small clarification in `docs/plans/README.md` about canonical GitHub org links. Risk is low: read-only projection with no persistence or grant path yet. +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, ordering via `ISkillDefinitionRegistry.GetDefinitionsInIdOrder()`, Bruno/README/manual QA expectations, and deferral of persistence to NEO-38. +- `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. @@ -38,7 +38,7 @@ Adds `GET /game/players/{id}/skill-progression` with server-authoritative defaul ## 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. +- ~~**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 diff --git a/server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs index 217c9eb..c2a512b 100644 --- a/server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs @@ -23,7 +23,7 @@ public sealed class SkillProgressionSnapshotApiTests } [Fact] - public async Task GetSkillProgression_ShouldReturnSchemaV1_WithDefaultXpAndLevel_ForFrozenTrioInIdOrder() + public async Task GetSkillProgression_ShouldReturnSchemaV1_WithDefaultXpAndLevel_ForFrozenTrio() { // Arrange await using var factory = new InMemoryWebApplicationFactory(); @@ -39,14 +39,14 @@ public sealed class SkillProgressionSnapshotApiTests Assert.Equal(SkillProgressionSnapshotResponse.CurrentSchemaVersion, body!.SchemaVersion); Assert.Equal("dev-local-1", body.PlayerId); Assert.NotNull(body.Skills); - var ids = body.Skills.Select(static s => s.Id).ToList(); - Assert.Equal(new[] { "intrusion", "refine", "salvage" }, ids); - Assert.All( - body.Skills, - s => - { - Assert.Equal(0, s.Xp); - Assert.Equal(1, s.Level); - }); + Assert.Equal(3, body.Skills!.Count); + var byId = body.Skills.ToDictionary(static s => s.Id, StringComparer.Ordinal); + Assert.Equal(3, byId.Count); + foreach (var id in new[] { "intrusion", "refine", "salvage" }) + { + Assert.True(byId.TryGetValue(id, out var row)); + Assert.Equal(0, row!.Xp); + Assert.Equal(1, row.Level); + } } } diff --git a/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs index d1dd05a..7631190 100644 --- a/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs +++ b/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs @@ -40,6 +40,9 @@ public static class SkillProgressionSnapshotApi }); } + // Ordinal sort for stable serialization; consumers must index rows by skill id (NEO-37). + skills.Sort(static (a, b) => string.CompareOrdinal(a.Id, b.Id)); + return new SkillProgressionSnapshotResponse { PlayerId = playerId, diff --git a/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs index e099ec7..6595b1f 100644 --- a/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs +++ b/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs @@ -13,7 +13,7 @@ public sealed class SkillProgressionSnapshotResponse [JsonPropertyName("playerId")] public required string PlayerId { get; init; } - /// One row per registered skill, ordered by stable id (ordinal). + /// One row per registered skill id. Consumers must treat this as unordered and key rows by — array order is not part of the public contract. [JsonPropertyName("skills")] public required IReadOnlyList Skills { get; init; } } diff --git a/server/README.md b/server/README.md index 1005545..2f19752 100644 --- a/server/README.md +++ b/server/README.md @@ -47,7 +47,7 @@ curl -sS -i "http://localhost:5253/game/world/skill-definitions" ## Skill progression snapshot (NEO-37) -**`GET /game/players/{id}/skill-progression`** returns a versioned JSON body (`schemaVersion` **1**, **`playerId`**, **`skills`**) with one row per registered skill (**`id`**, **`xp`**, **`level`**). Rows are ordered like **`ISkillDefinitionRegistry.GetDefinitionsInIdOrder()`**; before XP grants (**NEO-38**) each row boots with **`xp`** **0** and **`level`** **1**. Unknown players (**no row in position state**) receive **404**, same gate as **`GET …/cooldown-snapshot`**. Plan: [NEO-37 implementation plan](../../docs/plans/NEO-37-implementation-plan.md); manual QA: [`docs/manual-qa/NEO-37.md`](../../docs/manual-qa/NEO-37.md); Bruno: `bruno/neon-sprawl-server/skill-progression/`. +**`GET /game/players/{id}/skill-progression`** returns a versioned JSON body (`schemaVersion` **1**, **`playerId`**, **`skills`**) with one row per registered skill (**`id`**, **`xp`**, **`level`**). Treat **`skills`** as **unordered** and index rows by **`id`** — array order is not part of the contract (the server may sort for stable serialization). Before XP grants (**NEO-38**) each row boots with **`xp`** **0** and **`level`** **1**. Unknown players (**no row in position state**) receive **404**, same gate as **`GET …/cooldown-snapshot`**. Plan: [NEO-37 implementation plan](../../docs/plans/NEO-37-implementation-plan.md); manual QA: [`docs/manual-qa/NEO-37.md`](../../docs/manual-qa/NEO-37.md); Bruno: `bruno/neon-sprawl-server/skill-progression/`. ```bash curl -sS -i "http://localhost:5253/game/players/dev-local-1/skill-progression"