**Scope:** Branch `NEO-36-e2m1-read-only-skill-catalog-http-projection` vs merge-base `a5f7f1b4aedfb7928440ac2e1fef7035895544a6` (`origin/main` at review time): `GET /game/world/skill-definitions`, `SkillDefinitionsWorldApi`, `SkillDefinitionsListDtos`, `SkillDefinitionsWorldApiTests`, Bruno `skill-definitions/`, `docs/plans/NEO-36-implementation-plan.md`, `docs/manual-qa/NEO-36.md`.
**Base:** `origin/main` (merge-base above)
## Verdict
**Approve with nits**
## Summary
The branch adds a versioned read-only JSON projection backed by `ISkillDefinitionRegistry.GetDefinitionsInIdOrder()`, wired next to existing world projections in `Program.cs`, with integration tests mirroring `InteractablesWorldApiTests` (HTTP 200, `ReadFromJsonAsync`, schema version, frozen trio order, and spot-checks on `allowedXpSourceKinds`). Bruno requests assert the same contract. Behavior matches the NEO-36 plan and E2.M1 catalog intent: no mutations, no duplicate source of truth beyond the registry.
## Documentation checked
| Document | Result |
|----------|--------|
| `docs/plans/NEO-36-implementation-plan.md` | **Matches** — route `/game/world/skill-definitions`, `schemaVersion` + `skills` rows with the four fields, registry-backed mapping, tests + Bruno + manual QA as listed. |
| `docs/decomposition/modules/E2_M1_SkillDefinitionRegistry.md` | **Matches** — read-only exposure of stable ids, display names, categories, and `allowedXpSourceKinds`; aligns with “dev tooling / QA” style use; no XP grant enforcement here (E2.M2). |
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E2.M1 in progress; HTTP projection is consistent with module scope. |
| `docs/decomposition/modules/contracts.md` | **N/A** (not opened end-to-end) — response uses explicit `JsonPropertyName` and a `schemaVersion` const; same pattern as interactables list. |
## Blocking issues
(none)
## Suggestions
1. ~~**`docs/manual-qa/NEO-36.md` header** — [planning-implementation-docs](../../.cursor/rules/planning-implementation-docs.md) asks for a small table with **Key**, **Title**, **Linear** link, **Plan** link, and **Branch** on ticketed manual QA files. Add that table so the checklist matches repo convention (same pattern as other `docs/manual-qa/NEO-*.md` files that follow the rule).~~ **Done.**
2. ~~**`documentation_and_implementation_alignment.md`** — Extend the E2.M1 row to note NEO-36: `GET /game/world/skill-definitions`, `Game/Skills/SkillDefinitionsWorldApi.cs` / DTOs, and pointer to `docs/plans/NEO-36-implementation-plan.md` (same PR or immediate follow-up per [documentation_and_implementation_alignment.md](../decomposition/modules/documentation_and_implementation_alignment.md)).~~ **Done.**
3. ~~**`server/README.md` (optional)** — Skill catalog section documents load policy (NEO-34) but not the read projection. A one-line anchor to the GET route would match how NEO-25 interactables are surfaced for operators; only if you want parity with other HTTP surfaces.~~ **Done.**
## Nits
1. ~~**Nit:** Bruno test sorts ids with `localeCompare`; server order is ordinal (`GetDefinitionsInIdOrder`). For current ASCII `snake_case` ids they agree; if ids ever used non-ASCII, consider documenting or aligning sort semantics in Bruno.~~**Done.** (ordinal sort + comment in Bruno.)
## Verification
- `dotnet test NeonSprawl.sln --filter "FullyQualifiedName~SkillDefinitionsWorldApiTests"` — passed at review time.
- Optional: full `dotnet test NeonSprawl.sln` before merge.
- Manual: follow `docs/manual-qa/NEO-36.md` (after adding the header table if you take suggestion 1) and/or run the Bruno request against `Local` environment.
@ -37,6 +37,14 @@ On startup the host loads every **`*.json`** under the skills directory, validat
On success, **Information** logs include the resolved skills directory path and distinct skill count.
On success, **Information** logs include the resolved skills directory path and distinct skill count.
## Skill definitions (NEO-36)
**`GET /game/world/skill-definitions`** returns a versioned JSON body (`schemaVersion` **1**, **`skills`**) backed by **`ISkillDefinitionRegistry`** — the same prototype rows loaded at startup (no second source of truth). Plan: [NEO-36 implementation plan](../../docs/plans/NEO-36-implementation-plan.md); manual QA: [`docs/manual-qa/NEO-36.md`](../../docs/manual-qa/NEO-36.md); Bruno: `bruno/neon-sprawl-server/skill-definitions/`.
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).
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).
