From 15ab06f3e74011cf6c8bfabc65ff6fa6158d542e Mon Sep 17 00:00:00 2001 From: VinPropane Date: Mon, 4 May 2026 21:24:25 -0400 Subject: [PATCH 1/7] docs: note canonical GitHub org for Linear and doc links --- docs/plans/README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/plans/README.md b/docs/plans/README.md index 344e9ee..6533c0d 100644 --- a/docs/plans/README.md +++ b/docs/plans/README.md @@ -9,6 +9,10 @@ A small number of files keep the historical **Jira / Atlassian `NEON-*`** key in - Each such file includes a short **Historical** banner at the top pointing at the matching **`NEO-*`** plan (same scope) where kickoff and commits should align. - Prefer opening and linking **`NEO-*`** issues and plans for new contributors. +## GitHub links in Linear (and docs) + +The canonical public repo path is **`ViPro-Technologies/neon-sprawl`** (see `git remote`). Use that org in **raw GitHub** markdown links (`https://github.com/ViPro-Technologies/neon-sprawl/blob/main/...`). The path **`neon-sprawl/neon-sprawl`** is not the live repo and will **404**. + ## Conventions - New or moved plans: `docs/plans/{LINEAR_KEY}-implementation-plan.md` (see [story kickoff](../../.cursor/rules/story-kickoff.md)). From 413484000f32303cbf707faf9249aaa147f6e7af Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 6 May 2026 21:46:41 -0400 Subject: [PATCH 2/7] NEO-37: add implementation plan for skill progression snapshot API --- docs/plans/NEO-37-implementation-plan.md | 85 ++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 docs/plans/NEO-37-implementation-plan.md diff --git a/docs/plans/NEO-37-implementation-plan.md b/docs/plans/NEO-37-implementation-plan.md new file mode 100644 index 0000000..c0cd757 --- /dev/null +++ b/docs/plans/NEO-37-implementation-plan.md @@ -0,0 +1,85 @@ +# NEO-37 — Implementation plan + +## Story reference + +| Field | Value | +|--------|--------| +| **Key** | NEO-37 | +| **Title** | Skill progression snapshot (read model) | +| **Linear** | https://linear.app/neon-sprawl/issue/NEO-37/skill-progression-snapshot-read-model | +| **Module** | [E2.M2 — XpAwardAndLevelEngine](../decomposition/modules/E2_M2_XpAwardAndLevelEngine.md) | + +## Kickoff clarifications + +**Asked (Agent `AskQuestion`):** Lock plan defaults for route, 404 rule, JSON shape (`schemaVersion` + `playerId` + `skills` with `id` / `xp` / `level`), Bruno folder style, and `docs/manual-qa/NEO-37.md`. + +**Answer:** User chose **Proceed — use these defaults in the plan**. + +## 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). + +**In scope (from Linear):** + +- Versioned JSON for the agreed player-scoped path. +- One row per registered skill id; coherent defaults (0 XP, level 1). +- Unit/integration tests (AAA) and Bruno collection folder mirroring NEO-36 style. +- Server README section for the endpoint. + +**Out of scope (from Linear):** + +- Applying XP grants ([NEO-38](https://linear.app/neon-sprawl/issue/NEO-38/apply-skill-xp-grant-allowlist-level-up)). +- Data-driven level curves (separate issue; this slice uses fixed baseline level **1** for all rows). +- Gather/craft/combat integration (Epic 2 Slice 3). + +## Acceptance criteria checklist + +- [ ] Versioned JSON response for **`GET /game/players/{id}/skill-progression`** (`schemaVersion` **1**). +- [ ] One row per registered skill id; **`xp`** = **0**, **`level`** = **1** for all rows in this story; **`skills`** ordered by **`ISkillDefinitionRegistry.GetDefinitionsInIdOrder()`**. +- [ ] Automated tests (AAA); Bruno folder mirroring NEO-36 (`skill-definitions` conventions). +- [ ] **`server/README.md`** section documenting the endpoint (curl + pointers to plan / manual QA / Bruno). + +## Technical approach + +1. **Route:** **`GET /game/players/{id}/skill-progression`** alongside existing **`/game/players/{id}/…`** reads (`cooldown-snapshot`, `hotbar-loadout`, etc.). + +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. + +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. + +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. + +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. + +## Files to add + +| Path | Purpose | +|------|---------| +| `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. | +| `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. | + +## Files to modify + +| Path | Rationale | +|------|-----------| +| `server/NeonSprawl.Server/Program.cs` | Call **`app.MapSkillProgressionSnapshotApi()`** (exact name may match class) next to other `Map*Api` registrations. | +| `server/README.md` | Document **`GET /game/players/{id}/skill-progression`**, curl, and links (plan / manual QA / Bruno). | + +## Tests + +| 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). | + +Bruno scripts complement automated coverage; they do not replace it. + +## Open questions / risks + +- **NEO-38 merge point:** When grants land,prefer **replacing** the pure “zeros builder” with a store-backed projection without changing the **public JSON contract** (only values change). If **`schemaVersion`** must bump later, coordinate with clients. +- **None** otherwise. From 9cd96a72c3caec01930dab1ab8a1434698ddd4c0 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 6 May 2026 21:48:17 -0400 Subject: [PATCH 3/7] NEO-37: add skill progression snapshot GET API, tests, Bruno, docs --- .../Get skill progression.bru | 46 ++++++++++++++++ .../skill-progression/folder.bru | 3 ++ docs/manual-qa/NEO-37.md | 27 ++++++++++ docs/plans/NEO-37-implementation-plan.md | 8 +-- .../SkillProgressionSnapshotApiTests.cs | 52 +++++++++++++++++++ .../Skills/SkillProgressionSnapshotApi.cs | 50 ++++++++++++++++++ .../Skills/SkillProgressionSnapshotDtos.cs | 32 ++++++++++++ server/NeonSprawl.Server/Program.cs | 1 + server/README.md | 8 +++ 9 files changed, 223 insertions(+), 4 deletions(-) create mode 100644 bruno/neon-sprawl-server/skill-progression/Get skill progression.bru create mode 100644 bruno/neon-sprawl-server/skill-progression/folder.bru create mode 100644 docs/manual-qa/NEO-37.md create mode 100644 server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs create mode 100644 server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs create mode 100644 server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs diff --git a/bruno/neon-sprawl-server/skill-progression/Get skill progression.bru b/bruno/neon-sprawl-server/skill-progression/Get skill progression.bru new file mode 100644 index 0000000..f6fec84 --- /dev/null +++ b/bruno/neon-sprawl-server/skill-progression/Get skill progression.bru @@ -0,0 +1,46 @@ +meta { + name: GET skill progression + type: http + seq: 1 +} + +docs { + NEO-37: per-player read model; join with GET /game/world/skill-definitions for display names. +} + +get { + url: {{baseUrl}}/game/players/dev-local-1/skill-progression + body: none + auth: none +} + +tests { + test("returns 200 JSON with schema v1 and skills array", function () { + expect(res.getStatus()).to.equal(200); + expect(res.getHeader("content-type")).to.contain("application/json"); + const body = res.getBody(); + expect(body.schemaVersion).to.equal(1); + expect(body.playerId).to.equal("dev-local-1"); + expect(body.skills).to.be.an("array"); + 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"); + expect(intrusion).to.be.an("object"); + expect(intrusion.xp).to.equal(0); + expect(intrusion.level).to.equal(1); + const ids = new Set(body.skills.map((x) => x.id)); + expect(ids.has("salvage")).to.equal(true); + expect(ids.has("refine")).to.equal(true); + expect(ids.has("intrusion")).to.equal(true); + }); +} diff --git a/bruno/neon-sprawl-server/skill-progression/folder.bru b/bruno/neon-sprawl-server/skill-progression/folder.bru new file mode 100644 index 0000000..0de69bd --- /dev/null +++ b/bruno/neon-sprawl-server/skill-progression/folder.bru @@ -0,0 +1,3 @@ +meta { + name: skill-progression +} diff --git a/docs/manual-qa/NEO-37.md b/docs/manual-qa/NEO-37.md new file mode 100644 index 0000000..5e668f5 --- /dev/null +++ b/docs/manual-qa/NEO-37.md @@ -0,0 +1,27 @@ +# NEO-37 — Manual QA checklist + +| Field | Value | +|-------|-------| +| Key | NEO-37 | +| Title | Skill progression snapshot (read model) | +| Linear | https://linear.app/neon-sprawl/issue/NEO-37/skill-progression-snapshot-read-model | +| Plan | `docs/plans/NEO-37-implementation-plan.md` | +| Branch | `NEO-37-skill-progression-snapshot-read-model` | + +## 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. + +## Checklist + +1. Start **`NeonSprawl.Server`** (e.g. `dotnet run` from `server/NeonSprawl.Server`). +2. **`GET /game/players/dev-local-1/skill-progression`** — expect **200** and **`Content-Type`** containing **`application/json`**. + + ```bash + 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**. +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 c0cd757..6d79189 100644 --- a/docs/plans/NEO-37-implementation-plan.md +++ b/docs/plans/NEO-37-implementation-plan.md @@ -34,10 +34,10 @@ ## Acceptance criteria checklist -- [ ] Versioned JSON response for **`GET /game/players/{id}/skill-progression`** (`schemaVersion` **1**). -- [ ] One row per registered skill id; **`xp`** = **0**, **`level`** = **1** for all rows in this story; **`skills`** ordered by **`ISkillDefinitionRegistry.GetDefinitionsInIdOrder()`**. -- [ ] Automated tests (AAA); Bruno folder mirroring NEO-36 (`skill-definitions` conventions). -- [ ] **`server/README.md`** section documenting the endpoint (curl + pointers to plan / manual QA / Bruno). +- [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] 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). ## Technical approach diff --git a/server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs new file mode 100644 index 0000000..217c9eb --- /dev/null +++ b/server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionSnapshotApiTests.cs @@ -0,0 +1,52 @@ +using System.Linq; +using System.Net; +using System.Net.Http.Json; +using NeonSprawl.Server.Game.Skills; +using Xunit; + +namespace NeonSprawl.Server.Tests.Game.Skills; + +public sealed class SkillProgressionSnapshotApiTests +{ + [Fact] + public async Task GetSkillProgression_ShouldReturnNotFound_WhenPlayerUnknown() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + // Act + var response = await client.GetAsync("/game/players/missing-player/skill-progression"); + + // Assert + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Fact] + public async Task GetSkillProgression_ShouldReturnSchemaV1_WithDefaultXpAndLevel_ForFrozenTrioInIdOrder() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + // Act + var response = await client.GetAsync("/game/players/dev-local-1/skill-progression"); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + 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); + }); + } +} diff --git a/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs new file mode 100644 index 0000000..d1dd05a --- /dev/null +++ b/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs @@ -0,0 +1,50 @@ +using NeonSprawl.Server.Game.PositionState; + +namespace NeonSprawl.Server.Game.Skills; + +/// Maps GET /game/players/{{id}}/skill-progression (NEO-37). +public static class SkillProgressionSnapshotApi +{ + public static WebApplication MapSkillProgressionSnapshotApi(this WebApplication app) + { + app.MapGet( + "/game/players/{id}/skill-progression", + (string id, IPositionStateStore positions, ISkillDefinitionRegistry registry) => + { + if (!positions.TryGetPosition(id, out _)) + { + return Results.NotFound(); + } + + var trimmedId = id.Trim(); + return Results.Json(BuildSnapshot(trimmedId, registry)); + }); + + return app; + } + + internal static SkillProgressionSnapshotResponse BuildSnapshot( + string playerId, + ISkillDefinitionRegistry registry) + { + var defs = registry.GetDefinitionsInIdOrder(); + var skills = new List(defs.Count); + foreach (var d in defs) + { + skills.Add( + new SkillProgressionRowJson + { + Id = d.Id, + Xp = 0, + Level = 1, + }); + } + + return new SkillProgressionSnapshotResponse + { + PlayerId = playerId, + Skills = skills, + SchemaVersion = SkillProgressionSnapshotResponse.CurrentSchemaVersion, + }; + } +} diff --git a/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs new file mode 100644 index 0000000..e099ec7 --- /dev/null +++ b/server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotDtos.cs @@ -0,0 +1,32 @@ +using System.Text.Json.Serialization; + +namespace NeonSprawl.Server.Game.Skills; + +/// JSON body for GET /game/players/{{id}}/skill-progression (NEO-37). +public sealed class SkillProgressionSnapshotResponse +{ + public const int CurrentSchemaVersion = 1; + + [JsonPropertyName("schemaVersion")] + public int SchemaVersion { get; init; } = CurrentSchemaVersion; + + [JsonPropertyName("playerId")] + public required string PlayerId { get; init; } + + /// One row per registered skill, ordered by stable id (ordinal). + [JsonPropertyName("skills")] + public required IReadOnlyList Skills { get; init; } +} + +/// XP and level for one skill before grants (defaults) or after NEO-38 persistence. +public sealed class SkillProgressionRowJson +{ + [JsonPropertyName("id")] + public required string Id { get; init; } + + [JsonPropertyName("xp")] + public int Xp { get; init; } + + [JsonPropertyName("level")] + public int Level { get; init; } +} diff --git a/server/NeonSprawl.Server/Program.cs b/server/NeonSprawl.Server/Program.cs index 94ed641..b9241ec 100644 --- a/server/NeonSprawl.Server/Program.cs +++ b/server/NeonSprawl.Server/Program.cs @@ -28,6 +28,7 @@ app.MapPositionStateApi(); app.MapInteractionApi(); app.MapInteractablesWorldApi(); app.MapSkillDefinitionsWorldApi(); +app.MapSkillProgressionSnapshotApi(); app.MapTargetingApi(); app.MapHotbarLoadoutApi(); app.MapCooldownSnapshotApi(); diff --git a/server/README.md b/server/README.md index 91d27a0..1005545 100644 --- a/server/README.md +++ b/server/README.md @@ -45,6 +45,14 @@ On success, **Information** logs include the resolved skills directory path and 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/`. + +```bash +curl -sS -i "http://localhost:5253/game/players/dev-local-1/skill-progression" +``` + ## Position persistence (NEO-8) When **`ConnectionStrings:NeonSprawl`** is set to a valid PostgreSQL connection string, the server uses the **`player_position`** table (relational columns `pos_x` … `sequence`, not JSONB). DDL lives in [`server/db/migrations/V001__player_position.sql`](../db/migrations/V001__player_position.sql); the app applies that script on startup and on first store use (idempotent `CREATE TABLE IF NOT EXISTS`). The configured dev player (`Game:DevPlayerId` / `Game:DefaultPosition`) is seeded once at host startup, matching the in-memory store’s constructor seed (not on every HTTP request). From 5e180f3aa0ae5027c8680d1918a602f3a6d6c926 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 6 May 2026 21:50:37 -0400 Subject: [PATCH 4/7] NEO-37: code review snapshot for skill progression API branch --- docs/reviews/2026-05-06-NEO-37.md | 51 +++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 docs/reviews/2026-05-06-NEO-37.md diff --git a/docs/reviews/2026-05-06-NEO-37.md b/docs/reviews/2026-05-06-NEO-37.md new file mode 100644 index 0000000..b71635d --- /dev/null +++ b/docs/reviews/2026-05-06-NEO-37.md @@ -0,0 +1,51 @@ +# 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). 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. + +## 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/manual-qa/NEO-37.md` — **matches** curl steps and assertions. + +- `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — **partially matches** story placement (Slice 2, NEO-37); module body still describes the full XP engine responsibilities (grants, curves, events), which rightly remain **out of scope** for NEO-37. The implementation snapshot paragraph still states no implementation landed; **that text should be updated after merge** to reflect NEO-37 delivery (tracking note below). + +- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches** (E2.M2 row cites NEO-37); row **status / narrative after merge** may need tightening once this ships (same hygiene as E2.M2 page). + +- `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. + +2. **`docs/plans/NEO-37-implementation-plan.md` typography:** Insert a missing space after the comma (“When grants land, prefer …”) under Open questions. + +## 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. + +## 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`. From 295eb34c6853ae179904e60e33b5583f761a95bb Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 6 May 2026 21:51:38 -0400 Subject: [PATCH 5/7] NEO-37: reconcile decomposition docs + review feedback --- docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md | 6 ++++-- .../modules/documentation_and_implementation_alignment.md | 2 +- docs/decomposition/modules/module_dependency_register.md | 2 +- docs/plans/NEO-37-implementation-plan.md | 2 +- docs/reviews/2026-05-06-NEO-37.md | 8 ++++---- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md b/docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md index 8937b8e..de9c254 100644 --- a/docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md +++ b/docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md @@ -7,11 +7,13 @@ | **Module ID** | E2.M2 | | **Epic** | [Epic 2 — Skills and Progression Framework](../epics/epic_02_skills_and_progression.md) | | **Stage target** | Prototype | -| **Status** | Planned (see [dependency register](module_dependency_register.md)) | +| **Status** | In Progress (see [dependency register](module_dependency_register.md)) | ## Implementation snapshot -**Linear (label `E2.M2`):** [Epic 2 project](https://linear.app/neon-sprawl/project/epic-2-classless-skill-and-progression-framework-5200f84bf8b6) — **Slice 2:** [NEO-37](https://linear.app/neon-sprawl/issue/NEO-37) (progression read snapshot) → [NEO-38](https://linear.app/neon-sprawl/issue/NEO-38) (apply grant + allowlist + level-up) → [NEO-39](https://linear.app/neon-sprawl/issue/NEO-39) (data-driven `LevelCurve` + CI); [NEO-40](https://linear.app/neon-sprawl/issue/NEO-40) (telemetry hook sites) after NEO-38, parallel with NEO-39. **Slice 3 integration** (multi-label where noted): [NEO-41](https://linear.app/neon-sprawl/issue/NEO-41), [NEO-42](https://linear.app/neon-sprawl/issue/NEO-42), [NEO-43](https://linear.app/neon-sprawl/issue/NEO-43), [NEO-44](https://linear.app/neon-sprawl/issue/NEO-44). No implementation landed yet; update this section and the [alignment](documentation_and_implementation_alignment.md) row when stories merge. +**Linear (label `E2.M2`):** [Epic 2 project](https://linear.app/neon-sprawl/project/epic-2-classless-skill-and-progression-framework-5200f84bf8b6) — **Slice 2:** [NEO-37](https://linear.app/neon-sprawl/issue/NEO-37) (progression read snapshot) → [NEO-38](https://linear.app/neon-sprawl/issue/NEO-38) (apply grant + allowlist + level-up) → [NEO-39](https://linear.app/neon-sprawl/issue/NEO-39) (data-driven `LevelCurve` + CI); [NEO-40](https://linear.app/neon-sprawl/issue/NEO-40) (telemetry hook sites) after NEO-38, parallel with NEO-39. **Slice 3 integration** (multi-label where noted): [NEO-41](https://linear.app/neon-sprawl/issue/NEO-41), [NEO-42](https://linear.app/neon-sprawl/issue/NEO-42), [NEO-43](https://linear.app/neon-sprawl/issue/NEO-43), [NEO-44](https://linear.app/neon-sprawl/issue/NEO-44). + +**NEO-37 landed:** versioned **`GET /game/players/{id}/skill-progression`** ([implementation plan](../../plans/NEO-37-implementation-plan.md)) — `SkillProgressionSnapshotApi` / `SkillProgressionSnapshotDtos` under `server/NeonSprawl.Server/Game/Skills/`; one row per `ISkillDefinitionRegistry` skill (**xp** 0, **level** 1 until grants persist); **`404`** when the player has no position (same gate as **`GET …/cooldown-snapshot`**). Bruno **`bruno/neon-sprawl-server/skill-progression/`**; manual QA **[`NEO-37.md`](../../manual-qa/NEO-37.md)**; **[server README — Skill progression snapshot (NEO-37)](../../../server/README.md#skill-progression-snapshot-neo-37)**. **Still backlog within this module:** applying XP grants, `LevelCurve`, `LevelUpEvent`, telemetry hook sites (**NEO-38** onward). Keep this paragraph and [documentation tracking](documentation_and_implementation_alignment.md) in sync as Slice 2 stories merge. ## Purpose diff --git a/docs/decomposition/modules/documentation_and_implementation_alignment.md b/docs/decomposition/modules/documentation_and_implementation_alignment.md index 1d2bd1f..63b3c75 100644 --- a/docs/decomposition/modules/documentation_and_implementation_alignment.md +++ b/docs/decomposition/modules/documentation_and_implementation_alignment.md @@ -51,7 +51,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not | E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **NEO-25 landed:** versioned **`GET /game/world/interactables`** ([NEO-25](../../plans/NEO-25-implementation-plan.md)) — `InteractablesListResponse` / `InteractablesWorldApi` in `server/NeonSprawl.Server/Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` (fetch-driven props + glow); [server README — Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25). **NEO-26 landed:** prototype **`SelectionEvent`** — **`TargetSelectionClient.selection_event`** ([NEO-26](../../plans/NEO-26-implementation-plan.md)) when **`lockedTargetId`** changes; optional **`log_selection_events`**. **NEO-27 landed:** Slice 3 telemetry hook sites documented — `selection_event` maps to product `target_changed` in `target_selection_client.gd`; server lock transition hook comments in `InMemoryPlayerTargetLockStore`; reserved `ability_cast_requested` / `ability_cast_denied` comments in `client/scripts/main.gd` (all TODO(E9.M1)); see [NEO-27](../../plans/NEO-27-implementation-plan.md) and [`docs/manual-qa/NEO-27.md`](../../manual-qa/NEO-27.md). **Follow-on / still open:** richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact`; production telemetry ingest/catalog after E9.M1. **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25), [Interaction](../../../server/README.md#interaction-neo-9) | | E1.M4 | In Progress | **NEO-29 landed:** prototype `HotbarLoadout` v1 server contract (`GET`/`POST /game/players/{id}/hotbar-loadout`) with stable deny reason codes, per-player scope, and persistence policy matching NEO-8/NS-17 (Postgres when configured, in-memory fallback otherwise). Client boot hydration is wired via `hotbar_loadout_client.gd` + `hotbar_state.gd` from `main.gd`; manual QA checklist created. **NEO-31 landed:** prototype **`POST /game/players/{id}/ability-cast`**, digit-key cast wiring from `main.gd`, `ability_cast_client.gd`, and `hotbar_cast_slot_resolver.gd` (target id from `lockedTargetId` in cached target state); GdUnit covers resolver + HTTP client; manual QA [NEO-31](../../manual-qa/NEO-31.md). **NEO-28 landed:** cast POST validates **lock + registry + range** (`invalid_target`, `out_of_range`); **`AbilityCastClient.cast_result_received`** + **`CastFeedbackLabel`** HUD line; manual QA [NEO-28](../../manual-qa/NEO-28.md). **NEO-30 landed:** Slice 3 cast funnel telemetry **hook-site comments** in [`AbilityCastApi.cs`](../../../server/NeonSprawl.Server/Game/AbilityInput/AbilityCastApi.cs) (`ability_cast_requested` on authoritative accept, `ability_cast_denied` + non-empty `reasonCode` on each JSON deny branch); client dev hooks unchanged; manual QA [NEO-30](../../manual-qa/NEO-30.md); plan [NEO-30](../../plans/NEO-30-implementation-plan.md). **NEO-32 landed:** `GET /game/players/{id}/cooldown-snapshot` + `on_cooldown` cast deny + prototype global cooldown commit; [`cooldown_snapshot_client.gd`](../../../client/scripts/cooldown_snapshot_client.gd), [`cooldown_state.gd`](../../../client/scripts/cooldown_state.gd), **`CooldownSlotsLabel`** in [`main.gd`](../../../client/scripts/main.gd); manual QA [NEO-32](../../manual-qa/NEO-32.md); plan [NEO-32](../../plans/NEO-32-implementation-plan.md). | [NEO-29](../../plans/NEO-29-implementation-plan.md), [NEO-31](../../plans/NEO-31-implementation-plan.md), [NEO-28](../../plans/NEO-28-implementation-plan.md), [NEO-30](../../plans/NEO-30-implementation-plan.md), [NEO-32](../../plans/NEO-32-implementation-plan.md); [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md); [E1M4 prototype backlog](../../plans/E1M4-prototype-backlog.md); `server/NeonSprawl.Server/Game/AbilityInput/`; [`client/scripts/hotbar_loadout_client.gd`](../../../client/scripts/hotbar_loadout_client.gd), [`client/scripts/hotbar_state.gd`](../../../client/scripts/hotbar_state.gd), [`client/scripts/ability_cast_client.gd`](../../../client/scripts/ability_cast_client.gd), [`client/scripts/hotbar_cast_slot_resolver.gd`](../../../client/scripts/hotbar_cast_slot_resolver.gd), [`client/scripts/cooldown_snapshot_client.gd`](../../../client/scripts/cooldown_snapshot_client.gd), [`client/scripts/cooldown_state.gd`](../../../client/scripts/cooldown_state.gd); [server README — Hotbar loadout](../../../server/README.md#hotbar-loadout-neo-29), [Ability cast (NEO-31)](../../../server/README.md#ability-cast-neo-31), [Cooldown snapshot (NEO-32)](../../../server/README.md#cooldown-snapshot-neo-32) | | E2.M1 | In Progress | **NEO-33 landed:** frozen prototype trio **`salvage`** / **`refine`** / **`intrusion`** in [`content/skills/prototype_skills.json`](../../../content/skills/prototype_skills.json); PR gate + [`validate_content.py`](../../../scripts/validate_content.py) enforce schema, duplicate `id`, exact trio ids, and category coverage; designer note on **`allowedXpSourceKinds`** in [E2_M1](E2_M1_SkillDefinitionRegistry.md) + [`content/README.md`](../../../content/README.md). **NEO-34 landed:** fail-fast server load of `content/skills/*.json` at startup — `server/NeonSprawl.Server/Game/Skills/` ([NEO-34](../../plans/NEO-34-implementation-plan.md)); config + discovery in [server README — Skill catalog](../../../server/README.md#skill-catalog-contentskills-neo-34). **NEO-35 landed:** `ISkillDefinitionRegistry` / `SkillDefinitionRegistry` + DI ([NEO-35](../../plans/NEO-35-implementation-plan.md)). **NEO-36 landed:** versioned **`GET /game/world/skill-definitions`** ([NEO-36](../../plans/NEO-36-implementation-plan.md)) — `SkillDefinitionsWorldApi` + `SkillDefinitionsListDtos` in `server/NeonSprawl.Server/Game/Skills/`; Bruno `bruno/neon-sprawl-server/skill-definitions/`; manual QA [`NEO-36`](../../manual-qa/NEO-36.md). **Follow-on (E2.M2):** XP grant validation vs `allowedXpSourceKinds` per [E2_M1](E2_M1_SkillDefinitionRegistry.md). | [NEO-33](../../plans/NEO-33-implementation-plan.md), [NEO-34](../../plans/NEO-34-implementation-plan.md), [NEO-35](../../plans/NEO-35-implementation-plan.md), [NEO-36](../../plans/NEO-36-implementation-plan.md); [E2_M1](E2_M1_SkillDefinitionRegistry.md); [module_dependency_register.md](module_dependency_register.md) **E2.M1 note**; `server/NeonSprawl.Server/Game/Skills/`; [`docs/manual-qa/NEO-36.md`](../../manual-qa/NEO-36.md); [server README — Skill definitions (NEO-36)](../../../server/README.md#skill-definitions-neo-36) | -| E2.M2 | Planned | **Backlog (Linear):** Slice 2 vertical slices — [NEO-37](https://linear.app/neon-sprawl/issue/NEO-37) (player skill progression read snapshot) → [NEO-38](https://linear.app/neon-sprawl/issue/NEO-38) (apply XP grant + `allowedXpSourceKinds` validation + level-up) → [NEO-39](https://linear.app/neon-sprawl/issue/NEO-39) (data-driven `LevelCurve` + CI); [NEO-40](https://linear.app/neon-sprawl/issue/NEO-40) (telemetry hook sites, parallel with NEO-39 after NEO-38). Slice 3 integration — [NEO-41](https://linear.app/neon-sprawl/issue/NEO-41) / [NEO-42](https://linear.app/neon-sprawl/issue/NEO-42) / [NEO-43](https://linear.app/neon-sprawl/issue/NEO-43) (multi-label with **E3**/**E7**); [NEO-44](https://linear.app/neon-sprawl/issue/NEO-44) (gig XP from combat, **E5.M1**). See [epic_02 — E2.M2 + Slice 3](../epics/epic_02_skills_and_progression.md). | [E2_M2](E2_M2_XpAwardAndLevelEngine.md); label **`E2.M2`** on NEO-37–NEO-40, NEO-41–NEO-43 | +| E2.M2 | In Progress | **NEO-37 landed:** versioned **`GET /game/players/{id}/skill-progression`** ([NEO-37](../../plans/NEO-37-implementation-plan.md)) — `SkillProgressionSnapshotApi` / `SkillProgressionSnapshotDtos` in `server/NeonSprawl.Server/Game/Skills/`; bootstrap read model (**xp** 0, **level** 1) for every registered skill; known-player gate via `IPositionStateStore` (same pattern as cooldown snapshot); Bruno `bruno/neon-sprawl-server/skill-progression/`; manual QA [`NEO-37`](../../manual-qa/NEO-37.md); [server README — Skill progression snapshot (NEO-37)](../../../server/README.md#skill-progression-snapshot-neo-37). **Follow-on:** [NEO-38](https://linear.app/neon-sprawl/issue/NEO-38) (apply XP grant + `allowedXpSourceKinds` validation + level-up) → [NEO-39](https://linear.app/neon-sprawl/issue/NEO-39) (data-driven `LevelCurve` + CI); [NEO-40](https://linear.app/neon-sprawl/issue/NEO-40) (telemetry hook sites). Slice 3 integration — [NEO-41](https://linear.app/neon-sprawl/issue/NEO-41) / [NEO-42](https://linear.app/neon-sprawl/issue/NEO-42) / [NEO-43](https://linear.app/neon-sprawl/issue/NEO-43); [NEO-44](https://linear.app/neon-sprawl/issue/NEO-44). See [epic_02 — E2.M2 + Slice 3](../epics/epic_02_skills_and_progression.md). | [NEO-37](../../plans/NEO-37-implementation-plan.md), [E2_M2](E2_M2_XpAwardAndLevelEngine.md); label **`E2.M2`** on NEO-37–NEO-40, NEO-41–NEO-43 | --- diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index 16c85ab..34d539c 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -28,7 +28,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen | Module ID | Module Name | Depends On | Contract Needed | Phase Required | Status | |---|---|---|---|---|---| | E2.M1 | SkillDefinitionRegistry | None | SkillDef, SkillCategory, UnlockRequirement, allowedXpSourceKinds | Prototype | In Progress | -| E2.M2 | XpAwardAndLevelEngine | E2.M1 | XpGrantEvent, LevelCurve, LevelUpEvent | Prototype | Planned | +| E2.M2 | XpAwardAndLevelEngine | E2.M1 | XpGrantEvent, LevelCurve, LevelUpEvent | Prototype | In Progress | | E2.M3 | MasteryAndPerkUnlocks | E2.M2 | MasteryTrack, PerkUnlockEvent, PerkState | Pre-production | Planned | | E2.M4 | ProgressionPacingControls | E2.M2, E9.M2 | XpModifierProfile, CatchUpRule, PacingPolicy | Pre-production | Planned | diff --git a/docs/plans/NEO-37-implementation-plan.md b/docs/plans/NEO-37-implementation-plan.md index 6d79189..60514d2 100644 --- a/docs/plans/NEO-37-implementation-plan.md +++ b/docs/plans/NEO-37-implementation-plan.md @@ -81,5 +81,5 @@ Bruno scripts complement automated coverage; they do not replace it. ## Open questions / risks -- **NEO-38 merge point:** When grants land,prefer **replacing** the pure “zeros builder” with a store-backed projection without changing the **public JSON contract** (only values change). If **`schemaVersion`** must bump later, coordinate with clients. +- **NEO-38 merge point:** When grants land, prefer **replacing** the pure “zeros builder” with a store-backed projection without changing the **public JSON contract** (only values change). If **`schemaVersion`** must bump later, coordinate with clients. - **None** otherwise. diff --git a/docs/reviews/2026-05-06-NEO-37.md b/docs/reviews/2026-05-06-NEO-37.md index b71635d..b264cf7 100644 --- a/docs/reviews/2026-05-06-NEO-37.md +++ b/docs/reviews/2026-05-06-NEO-37.md @@ -20,9 +20,9 @@ Adds `GET /game/players/{id}/skill-progression` with server-authoritative defaul - `docs/manual-qa/NEO-37.md` — **matches** curl steps and assertions. -- `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — **partially matches** story placement (Slice 2, NEO-37); module body still describes the full XP engine responsibilities (grants, curves, events), which rightly remain **out of scope** for NEO-37. The implementation snapshot paragraph still states no implementation landed; **that text should be updated after merge** to reflect NEO-37 delivery (tracking note below). +- `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` — **partially matches** (E2.M2 row cites NEO-37); row **status / narrative after merge** may need tightening once this ships (same hygiene as E2.M2 page). +- `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). @@ -32,9 +32,9 @@ Adds `GET /game/players/{id}/skill-progression` with server-authoritative defaul ## 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. +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. +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 From a3cbef063cec912d9d59eaa6b46d060041781927 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 6 May 2026 21:53:24 -0400 Subject: [PATCH 6/7] NEO-37: treat skill progression JSON array order as non-contractual --- .../Get skill progression.bru | 7 ------- docs/manual-qa/NEO-37.md | 4 ++-- docs/plans/NEO-37-implementation-plan.md | 14 ++++++------- docs/reviews/2026-05-06-NEO-37.md | 6 +++--- .../SkillProgressionSnapshotApiTests.cs | 20 +++++++++---------- .../Skills/SkillProgressionSnapshotApi.cs | 3 +++ .../Skills/SkillProgressionSnapshotDtos.cs | 2 +- server/README.md | 2 +- 8 files changed, 27 insertions(+), 31 deletions(-) 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" From a89aadbb4337bf8d98a1678ae945160290bc0a1f Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 6 May 2026 21:54:13 -0400 Subject: [PATCH 7/7] 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`