NEO-43: close code review doc follow-ups (alignment + E7 status)

docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md

@@ -7,7 +7,7 @@
 | **Module ID** | E7.M2 |
 | **Epic** | [Epic 7 — Quest / Faction](../epics/epic_07_quest_faction.md) |
 | **Stage target** | Prototype |
-| **Status** | Planned (see [dependency register](module_dependency_register.md)) |
+| **Status** | Planned (see [dependency register](module_dependency_register.md)). **NEO-43 prep:** server `MissionRewardSkillXpGrant` for scripted skill XP from bundles (`mission_reward`); full router + `QuestRewardBundle` apply still TBD. |
 
 ## Purpose
 

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). **E2.M2 consumer (NEO-38 landed):** grant path validates `skillId` + `sourceKind` vs catalog **`allowedXpSourceKinds`** on **`POST …/skill-progression`** ([NEO-38](../../plans/NEO-38-implementation-plan.md)); see [server README — Skill progression grant](../../../server/README.md#skill-progression-grant-neo-38) and [E2_M2](E2_M2_XpAwardAndLevelEngine.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 | In Progress | **NEO-37 landed:** versioned **`GET /game/players/{id}/skill-progression`** ([NEO-37](../../plans/NEO-37-implementation-plan.md)) — read model for every registered skill; known-player gate via `IPositionStateStore`; [server README — Skill progression snapshot (NEO-37)](../../../server/README.md#skill-progression-snapshot-neo-37); manual QA [`NEO-37`](../../manual-qa/NEO-37.md). **NEO-38 landed:** **`POST`** same path — grant apply, persistence (`IPlayerSkillProgressionStore`, `V003` migration), structured denies + **`levelUps`** ([NEO-38](../../plans/NEO-38-implementation-plan.md)); manual QA [`NEO-38`](../../manual-qa/NEO-38.md); Bruno `bruno/neon-sprawl-server/skill-progression/`; [server README — Skill progression grant (NEO-38)](../../../server/README.md#skill-progression-grant-neo-38). **NEO-39 landed:** data-driven `LevelCurve` content + schema + CI validation (`*_level_curve.json`) with fail-fast startup schema checks; progression GET/POST level resolution now uses `ISkillLevelCurve` content-backed thresholds ([NEO-39](../../plans/NEO-39-implementation-plan.md)); manual QA [`NEO-39`](../../manual-qa/NEO-39.md). **NEO-40 landed:** comment-only telemetry hook sites in [`SkillProgressionSnapshotApi.cs`](../../../server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs) on **`POST …/skill-progression`** for future **`xp_grant`** / **`level_up`** ([NEO-40](../../plans/NEO-40-implementation-plan.md), [`NEO-40` manual QA](../../manual-qa/NEO-40.md)). **NEO-41 landed:** gather prototype — **`POST …/interact`** with **`kind: resource_node`** grants **`salvage`** + **`activity`** (10 XP) via [`SkillProgressionGrantOperations`](../../../server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs) ([NEO-41](../../plans/NEO-41-implementation-plan.md), [`NEO-41` manual QA](../../manual-qa/NEO-41.md)); [server README — Interaction](../../../server/README.md#interaction-neo-9). **NEO-42 landed (prep):** **`RefineActivitySkillXpGrant`** / **`RefineSkillXpConstants`** + deny-registry factory + tests — same NEO-38 grant stack; **E3.M2** must invoke on craft/refine success ([NEO-42](../../plans/NEO-42-implementation-plan.md), [`NEO-42` manual QA](../../manual-qa/NEO-42.md)); [server README — Craft / refine hook (NEO-42)](../../../server/README.md#craft--refine-hook--skill-xp-neo-42). **Slice 3 still open:** [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), [NEO-38](../../plans/NEO-38-implementation-plan.md), [NEO-39](../../plans/NEO-39-implementation-plan.md), [NEO-40](../../plans/NEO-40-implementation-plan.md), [NEO-41](../../plans/NEO-41-implementation-plan.md), [NEO-42](../../plans/NEO-42-implementation-plan.md), [E2_M2](E2_M2_XpAwardAndLevelEngine.md); label **`E2.M2`** on NEO-37–NEO-41, NEO-42–NEO-43 |
+| E2.M2 | In Progress | **NEO-37 landed:** versioned **`GET /game/players/{id}/skill-progression`** ([NEO-37](../../plans/NEO-37-implementation-plan.md)) — read model for every registered skill; known-player gate via `IPositionStateStore`; [server README — Skill progression snapshot (NEO-37)](../../../server/README.md#skill-progression-snapshot-neo-37); manual QA [`NEO-37`](../../manual-qa/NEO-37.md). **NEO-38 landed:** **`POST`** same path — grant apply, persistence (`IPlayerSkillProgressionStore`, `V003` migration), structured denies + **`levelUps`** ([NEO-38](../../plans/NEO-38-implementation-plan.md)); manual QA [`NEO-38`](../../manual-qa/NEO-38.md); Bruno `bruno/neon-sprawl-server/skill-progression/`; [server README — Skill progression grant (NEO-38)](../../../server/README.md#skill-progression-grant-neo-38). **NEO-39 landed:** data-driven `LevelCurve` content + schema + CI validation (`*_level_curve.json`) with fail-fast startup schema checks; progression GET/POST level resolution now uses `ISkillLevelCurve` content-backed thresholds ([NEO-39](../../plans/NEO-39-implementation-plan.md)); manual QA [`NEO-39`](../../manual-qa/NEO-39.md). **NEO-40 landed:** comment-only telemetry hook sites in [`SkillProgressionSnapshotApi.cs`](../../../server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs) on **`POST …/skill-progression`** for future **`xp_grant`** / **`level_up`** ([NEO-40](../../plans/NEO-40-implementation-plan.md), [`NEO-40` manual QA](../../manual-qa/NEO-40.md)). **NEO-41 landed:** gather prototype — **`POST …/interact`** with **`kind: resource_node`** grants **`salvage`** + **`activity`** (10 XP) via [`SkillProgressionGrantOperations`](../../../server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs) ([NEO-41](../../plans/NEO-41-implementation-plan.md), [`NEO-41` manual QA](../../manual-qa/NEO-41.md)); [server README — Interaction](../../../server/README.md#interaction-neo-9). **NEO-42 landed (prep):** **`RefineActivitySkillXpGrant`** / **`RefineSkillXpConstants`** + deny-registry factory + tests — same NEO-38 grant stack; **E3.M2** must invoke on craft/refine success ([NEO-42](../../plans/NEO-42-implementation-plan.md), [`NEO-42` manual QA](../../manual-qa/NEO-42.md)); [server README — Craft / refine hook (NEO-42)](../../../server/README.md#craft--refine-hook--skill-xp-neo-42). **NEO-43 landed (prep):** **`MissionRewardSkillXpGrant`** / **`MissionRewardSkillXpConstants`** + deny-registry factory + tests — same NEO-38 grant stack with fixed **`sourceKind: mission_reward`**; **E7.M2** must invoke from quest hand-in ([NEO-43](../../plans/NEO-43-implementation-plan.md), [`NEO-43` manual QA](../../manual-qa/NEO-43.md)); [server README — Mission / quest reward (NEO-43)](../../../server/README.md#mission--quest-reward--skill-xp-neo-43). **Slice 3 still open:** [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), [NEO-38](../../plans/NEO-38-implementation-plan.md), [NEO-39](../../plans/NEO-39-implementation-plan.md), [NEO-40](../../plans/NEO-40-implementation-plan.md), [NEO-41](../../plans/NEO-41-implementation-plan.md), [NEO-42](../../plans/NEO-42-implementation-plan.md), [NEO-43](../../plans/NEO-43-implementation-plan.md), [E2_M2](E2_M2_XpAwardAndLevelEngine.md); label **`E2.M2`** on NEO-37–NEO-41, NEO-42–NEO-44 |
 | E3.M1 | In Progress | **NEO-41 landed (prototype):** `POST …/interact` success on **`resource_node`** (`prototype_resource_node_alpha`) applies **`salvage`** skill XP (**`sourceKind: activity`**, 10 XP) via shared NEO-38 grant operations. **Still planned:** `GatherResult`, yields, inventory per [E3_M1](E3_M1_ResourceNodeAndGatherLoop.md). | [NEO-41](../../plans/NEO-41-implementation-plan.md), [E3_M1](E3_M1_ResourceNodeAndGatherLoop.md); `server/NeonSprawl.Server/Game/Interaction/`, `Game/Skills/` |
 
 ---

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

@@ -43,13 +43,14 @@
 
 - **`MissionRewardSkillXpConstants.MissionRewardSourceKind`** — single canonical string for the helper and docs.
 - **`MissionRewardDeniedRegistryWebApplicationFactory`** — **`salvage`** stub omits **`mission_reward`** (deny coverage); happy paths use disk catalog via **`InMemoryWebApplicationFactory`**.
+- **Code review follow-up (2026-05-15):** **`documentation_and_implementation_alignment.md`** E2.M2 row lists **NEO-43 landed** and **Slice 3 still open** → **NEO-44** only; **`E7_M2_RewardAndUnlockRouter.md`** Summary **Status** notes NEO-43 prep; **`NEO-43-implementation-plan.md`** technical approach uses **`MissionRewardSkillXpConstants.MissionRewardSourceKind`** (matches code).
 
 ## Technical approach
 
 1. Add **`MissionRewardSkillXpConstants`** (or equivalent single type) holding the canonical **`mission_reward`** string used by the helper (avoid string drift; match **`prototype_skills.json`** / NEO-38 docs).
 
 2. Add **`MissionRewardSkillXpGrant`** (static class, parallel to **`RefineActivitySkillXpGrant`**):
-   - Method e.g. **`GrantFromMissionReward(playerId, skillId, amount, registry, xpStore, levelCurve)`** calling **`TryApplyGrant`** with **`MissionRewardSkillXpConstants.SourceKind`**.
+   - Method e.g. **`GrantFromMissionReward(playerId, skillId, amount, registry, xpStore, levelCurve)`** calling **`TryApplyGrant`** with **`MissionRewardSkillXpConstants.MissionRewardSourceKind`**.
    - **Caller contract:** `playerId` normalized like other grant hooks; **`amount`** > 0 (delegate invalid amount to existing deny behavior); outcome may be ignored on “fire and forget” success paths or inspected when E7.M2 needs telemetry/deny handling.
 
 3. **Tests** — factory with registry where a prototype skill **excludes** **`mission_reward`** for deny coverage; in-memory progression store; assert snapshot / store deltas consistent with **`RefineActivitySkillXpGrantTests`** style.

docs/reviews/2026-05-15-NEO-43.md

@@ -0,0 +1,52 @@
+# Code review — NEO-43 (mission / quest reward skill XP prep)
+
+**Date:** 2026-05-15
+
+**Scope:** Branch `NEO-43-mission-quest-rewards-skill-xp` (commits on top of `origin/main` through `2af9b1c`), working tree clean at review time. Issue **NEO-43**.
+
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve**
+
+## Summary
+
+The branch adds a small **NEO-42-style** server prep slice: `MissionRewardSkillXpConstants`, `MissionRewardSkillXpGrant.GrantFromMissionReward` delegating to `SkillProgressionGrantOperations.TryApplyGrant` with fixed `sourceKind: mission_reward`, a denied-registry `WebApplicationFactory` plus three tests, manual QA, README, implementation plan, and an **E7.M2** decomposition anchor. Behavior matches the NEO-38 grant stack and prototype catalog (`salvage` allows `mission_reward`). Risk is low: additive API, no new HTTP routes, helper unused until E7.M2 wires quest payout.
+
+## Documentation checked
+
+| Document | Assessment |
+|----------|------------|
+| `docs/plans/NEO-43-implementation-plan.md` | **Matches** — helper, constants, factory pattern, tests (happy + GET alignment + deny), README, manual QA, E7 anchor align with plan and kickoff decisions. |
+| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — new **Implementation anchor (server)** points to `MissionRewardSkillXpGrant.GrantFromMissionReward` and the NEO-43 plan. |
+| `docs/decomposition/modules/module_dependency_register.md` | **N/A for code conflict** — E7.M2 remains **Planned**; prep hook does not require register churn. Optional follow-up: if the team treats any server hook toward E7.M2 as “started,” consider **In Progress** + Summary sync (not required for correctness). |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E2.M2 row updated: **NEO-43 landed** (prep) + **Slice 3 still open** lists **NEO-44** only. |
+| `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` | **N/A** — not edited; NEO-43 correctly extends the existing POST/grant story via shared operations. |
+| `docs/game-design/progression.md` (referenced) | **Matches** — README / manual QA echo mission vs combat XP separation. |
+
+~~**Register / tracking table:** Updating **`documentation_and_implementation_alignment.md`** (E2.M2 snapshot) is the main doc hygiene gap; see suggestions.~~ **Done.** (E2.M2 row updated; see **Suggestions** §1.)
+
+## Blocking issues
+
+_None._
+
+## Suggestions
+
+1. ~~**Implementation tracking table** — In `docs/decomposition/modules/documentation_and_implementation_alignment.md`, the **E2.M2** row still says **Slice 3 still open** and links NEO-43 as future work. After this story merges, add a **NEO-43 landed** line (mirror NEO-42: `MissionRewardSkillXpGrant`, tests, manual QA, README) and adjust the “still open” list so the inventory matches the repo (per **Status column** rule in that file).~~ **Done.** E2.M2 row: NEO-43 landed (prep) + Slice 3 still open → NEO-44 only; References column includes [NEO-43 plan](../plans/NEO-43-implementation-plan.md).
+
+2. ~~**E7.M2 Summary table (optional)** — `E7_M2_RewardAndUnlockRouter.md` **Status** remains **Planned** in the Summary table, consistent with the register. No change required unless you want a one-line note under Summary that server-side **skill XP from quest bundles** has a named hook (the new anchor already covers this).~~ **Done.** Status cell notes NEO-43 prep hook; module remains Planned.
+
+## Nits
+
+- ~~**Nit:** In `NEO-43-implementation-plan.md` **Technical approach** §1, the constant is named `MissionRewardSourceKind` in code, not `SourceKind` as in one plan sentence — trivial naming drift inside the plan only.~~ **Done.** Plan now references `MissionRewardSkillXpConstants.MissionRewardSourceKind` (matches `MissionRewardSkillXpConstants.cs`).
+
+## Verification
+
+Already run during review:
+
+```bash
+cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~MissionRewardSkillXpGrantTests"
+```
+
+Author should run full `dotnet test` on `NeonSprawl.Server.Tests` before merge if not already part of CI. Manual spot-check: follow `docs/manual-qa/NEO-43.md` (move + POST `mission_reward` + deny control with `trainer`).