NEO-34: address code review follow-ups (docs, appsettings, AAA)

Update E2.M1 alignment table for landed catalog load; add Content placeholders
to appsettings.json; align implementation plan tests/appsettings notes; expand
manual QA header to project standard; strike review suggestions; tighten host
catalog test Arrange/Act/Assert.

docs/decomposition/modules/documentation_and_implementation_alignment.md

@@ -50,7 +50,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not
 | E1.M2 | Ready | **Slice 2 prototype slice closed:** follow + `CameraState` ([NEO-15](https://linear.app/neon-sprawl/issue/NEO-15)); zoom bands ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); occlusion ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); occluder pick-through ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); contracts + hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). Client-local; no server use of camera pose. | [NEO-15](../../plans/NEO-15-implementation-plan.md), [NEO-16](../../plans/NEO-16-implementation-plan.md), [NEO-17](../../plans/NEO-17-implementation-plan.md), [NEO-18](../../plans/NEO-18-implementation-plan.md), [NEO-20](../../plans/NEO-20-implementation-plan.md); `client/scripts/isometric_follow_camera.gd`, `camera_state.gd`, `zoom_band_config.gd`, `occlusion_policy.gd`, `ground_pick.gd`; [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md) |
 | 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). **Follow-on:** server load ([NEO-34](https://linear.app/neon-sprawl/issue/NEO-34)), registry service ([NEO-35](https://linear.app/neon-sprawl/issue/NEO-35)), read HTTP + Bruno ([NEO-36](https://linear.app/neon-sprawl/issue/NEO-36)). | [NEO-33](../../plans/NEO-33-implementation-plan.md); [E2_M1](E2_M1_SkillDefinitionRegistry.md); [module_dependency_register.md](module_dependency_register.md) **E2.M1 note** |
+| 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). **Follow-on:** registry service ([NEO-35](https://linear.app/neon-sprawl/issue/NEO-35)), read HTTP + Bruno ([NEO-36](https://linear.app/neon-sprawl/issue/NEO-36)). | [NEO-33](../../plans/NEO-33-implementation-plan.md), [NEO-34](../../plans/NEO-34-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-34.md

@@ -1,12 +1,23 @@
-# Manual QA — NEO-34 (skill catalog at startup)
+# NEO-34 — Manual QA checklist
 
-## Prerequisites
+| Field | Value |
+|-------|-------|
+| Key | NEO-34 |
+| Title | E2.M1: Server loads SkillDef catalog at startup (fail-fast) |
+| Linear | https://linear.app/neon-sprawl/issue/NEO-34/e2m1-server-loads-skilldef-catalog-at-startup-fail-fast |
+| Plan | `docs/plans/NEO-34-implementation-plan.md` |
+| Branch | `NEO-34-e2m1-skill-catalog-startup` |
 
-- Repo clone with `content/skills` and `content/schemas/skill-def.schema.json` present.
+## 1) Happy path — discovery + health
 
-## Checks
+- [ ] Repo has `content/skills` and `content/schemas/skill-def.schema.json`.
+- [ ] From `server/NeonSprawl.Server`, run `dotnet run` with **no** `Content__*` overrides. Startup logs an **Information** line from category `NeonSprawl.Server.Game.Skills.SkillCatalog` with the **absolute** skills directory path and **3** skills (prototype catalog).
+- [ ] `GET /health` (or Bruno `core/Health`) returns **200** and JSON includes `service: "NeonSprawl.Server"`.
 
-- [ ] From `server/NeonSprawl.Server`, run `dotnet run` (no extra `Content` env). Startup logs an **Information** line from category `NeonSprawl.Server.Game.Skills.SkillCatalog` with the **absolute** skills directory path and **3** skills (prototype catalog).
+## 2) Fail-fast — invalid catalog
-- [ ] `GET /health` returns **200** after startup.
+
-- [ ] Introduce a deliberate catalog error (e.g. temporarily duplicate an `id` in `content/skills/prototype_skills.json`). `dotnet run` **fails** before listening; stderr shows **Skill catalog validation failed** and a path/rule hint. Revert the edit after the check.
+- [ ] Temporarily introduce a catalog error (e.g. duplicate an `id` in `content/skills/prototype_skills.json`). `dotnet run` **exits** before listening; stderr includes **Skill catalog validation failed** and a path/rule hint. **Revert** the edit after the check.
-- [ ] Optional: set `Content__SkillsDirectory` to a temp directory with invalid JSON and confirm the same fail-fast behavior.
+
+## 3) Optional — explicit config
+
+- [ ] Set `Content__SkillsDirectory` to a temp directory with invalid or empty catalogs and confirm the same fail-fast behavior (see [server README — Skill catalog](../../server/README.md#skill-catalog-contentskills-neo-34)).

docs/plans/NEO-34-implementation-plan.md

@@ -61,7 +61,7 @@
 |------|-----------|
 | `server/NeonSprawl.Server/NeonSprawl.Server.csproj` | Add JSON Schema NuGet package; optionally `Content` link to copy or embed schema path strategy (prefer reading schema from resolved repo `content/schemas` next to skills dir, or single `ContentRoot` option — align with loader). |
 | `server/NeonSprawl.Server/Program.cs` | Register catalog; **eager-resolve** after build so startup fails before listen; no new public routes. |
-| `server/NeonSprawl.Server/appsettings.json` | Optional commented or empty `Content` section documenting `SkillsDirectory`. |
+| `server/NeonSprawl.Server/appsettings.json` | Optional `Content` section: `SkillsDirectory` / `SkillDefSchemaPath` as empty strings (auto-discovery); override via env or user secrets per [server README](../../server/README.md#skill-catalog-contentskills-neo-34). |
 | `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | Set default `SkillsDirectory` / content overrides so existing tests use a **valid temp or repo-relative** catalog path (tests must not depend on undeclared cwd). |
 | `server/README.md` | Document `Content:SkillsDirectory`, default discovery, and Docker/CI override. |
 
@@ -69,10 +69,7 @@
 
 | Test file | What it covers |
 |-----------|------------------|
-| `server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionCatalogLoaderTests.cs` | **Unit:** loader against temp directories — valid minimal catalog passes; cases for bad `skills` type, schema error, duplicate id across files, Slice-1 wrong ids / missing category coverage; assert exception messages mention file path. |
+| `server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionCatalogLoaderTests.cs` | **Unit:** loader against temp directories — valid minimal catalog passes; cases for bad `skills` type, schema error, duplicate id across files, Slice-1 wrong ids / missing category coverage; assert exception messages mention file path. **Host:** `InMemoryWebApplicationFactory` + `/health` + DI catalog resolution; `WebApplicationFactory` negative case for invalid skills directory (merged here instead of a separate `SkillCatalogStartupIntegrationTests.cs`, per kickoff plan). |
-| `server/NeonSprawl.Server.Tests/Game/Skills/SkillCatalogStartupIntegrationTests.cs` | **Host:** `WebApplicationFactory` subclass with `ConfigureWebHost` + `UseSetting` pointing at good vs bad temp paths — good starts (`Server` accessible or health GET); bad throws on factory/server startup when host is built. |
-
-If a single file is preferred, merge loader unit tests + factory test into one `SkillCatalogTests.cs` with clear regions.
 
 ## Open questions / risks
 

docs/reviews/2026-05-02-NEO-34.md

@@ -18,12 +18,12 @@ The branch adds fail-fast loading of `content/skills/*.json` at server startup:
 
 | Document | Result | Notes |
 |----------|--------|--------|
-| `docs/plans/NEO-34-implementation-plan.md` | Partially matches | Acceptance criteria and technical approach are largely implemented. The plan’s **Files to modify** still lists `appsettings.json` for an optional `Content` section; the diff does **not** change `appsettings.json` (README documents config instead—either add a commented `Content` block or amend the plan). The plan’s **Tests** table names `SkillCatalogStartupIntegrationTests.cs`; implementation merged host tests into `SkillDefinitionCatalogLoaderTests.cs`, which the plan explicitly allows as an alternative. |
+| `docs/plans/NEO-34-implementation-plan.md` | Matches | Acceptance criteria and technical approach match implementation; `appsettings.json` includes optional `Content` keys; **Tests** table documents merged host coverage in `SkillDefinitionCatalogLoaderTests.cs`. Done. |
 | `docs/decomposition/modules/E2_M1_SkillDefinitionRegistry.md` | Matches | Load-time validation against `skill-def.schema.json`, duplicate `id`, and Slice 1 roster rules align with module intent. |
 | `docs/decomposition/modules/CT_M1_ContentValidationPipeline.md` | Matches | Server-side validation complements CI `validate_content.py`; same schema and gate semantics. |
 | `docs/decomposition/modules/module_dependency_register.md` | Matches | E2.M1 remains an appropriate home; no contract contradiction. |
-| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | Partially matches | The **Implementation tracking table** E2.M1 row still describes NEO-34 server load as **follow-on** only. After merge, that row should be updated to record **NEO-34 landed** (paths: `server/NeonSprawl.Server/Game/Skills/`, README section) per the doc’s rule on updating the table when slices land. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | Matches | E2.M1 tracking row updated for **NEO-34 landed** + pointers. Done. |
-| `docs/manual-qa/NEO-34.md` | Partially matches | Checklist content is useful, but it does not follow the project’s usual manual QA header (**Key / Title / Linear / Plan / Branch** table) described in `planning-implementation-docs.md` and exemplified by e.g. `docs/manual-qa/NEO-31.md`. |
+| `docs/manual-qa/NEO-34.md` | Matches | Standard header table and numbered sections aligned with NEO-31 style. Done. |
 
 ## Blocking issues
 
@@ -31,14 +31,14 @@ The branch adds fail-fast loading of `content/skills/*.json` at server startup:
 
 ## Suggestions
 
-1. After merge (or in this PR): update **`documentation_and_implementation_alignment.md`** E2.M1 snapshot to include NEO-34 server load and pointers to `Game/Skills/` + README, so the tracking table matches reality.
+1. ~~After merge (or in this PR): update **`documentation_and_implementation_alignment.md`** E2.M1 snapshot to include NEO-34 server load and pointers to `Game/Skills/` + README, so the tracking table matches reality.~~ Done.
-2. Either add a short commented **`Content`** subsection to **`server/NeonSprawl.Server/appsettings.json`** as the plan described, or remove/adjust that row in **`NEO-34-implementation-plan.md`** so the plan matches what shipped.
+2. ~~Either add a short commented **`Content`** subsection to **`server/NeonSprawl.Server/appsettings.json`** as the plan described, or remove/adjust that row in **`NEO-34-implementation-plan.md`** so the plan matches what shipped.~~ Done (placeholder `Content` keys in `appsettings.json` + plan row aligned).
-3. Expand **`docs/manual-qa/NEO-34.md`** with the standard header table (Key, Title, Linear, Plan, Branch) and numbered sections for consistency with other story QA files.
+3. ~~Expand **`docs/manual-qa/NEO-34.md`** with the standard header table (Key, Title, Linear, Plan, Branch) and numbered sections for consistency with other story QA files.~~ Done.
 
 ## Nits
 
-- **`bruno/neon-sprawl-server/core/Health.bru`**: Added Bruno `tests` blocks for status and `service` identity. Useful, but not called out in the NEO-34 plan; consider mentioning in the PR description or splitting if you want a strictly single-concern PR.
+- **`bruno/neon-sprawl-server/core/Health.bru`**: Added Bruno `tests` blocks for status and `service` identity. Useful, but not called out in the NEO-34 plan; consider mentioning in the PR description or splitting if you want a strictly single-concern PR. *(Unchanged — optional PR hygiene.)*
-- **`SkillDefinitionCatalogLoaderTests.cs`**: `Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds` puts `GetRequiredService<SkillDefinitionCatalog>()` under **Act** alongside `GetAsync("/health")`; strict AAA would treat DI resolution as **Assert** only (style only).
+- ~~**`SkillDefinitionCatalogLoaderTests.cs`**: `Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds` puts `GetRequiredService<SkillDefinitionCatalog>()` under **Act** alongside `GetAsync("/health")`; strict AAA would treat DI resolution as **Assert** only (style only).~~ Done.
 
 ## Verification
 

server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionCatalogLoaderTests.cs

@@ -173,9 +173,9 @@ public class SkillDefinitionCatalogLoaderTests
         using var client = factory.CreateClient();
         // Act
         var response = await client.GetAsync("/health");
-        var catalog = factory.Services.GetRequiredService<SkillDefinitionCatalog>();
         // Assert
         Assert.Equal(HttpStatusCode.OK, response.StatusCode);
+        var catalog = factory.Services.GetRequiredService<SkillDefinitionCatalog>();
         Assert.Equal(3, catalog.DistinctSkillCount);
     }
 

server/NeonSprawl.Server/appsettings.json

@@ -6,6 +6,10 @@
     }
   },
   "AllowedHosts": "*",
+  "Content": {
+    "SkillsDirectory": "",
+    "SkillDefSchemaPath": ""
+  },
   "Game": {
     "DevPlayerId": "dev-local-1",
     "DefaultPosition": {