NEO-42: address review — decomposition docs, plan AC, grant helper, test assert
parent
15bf598228
commit
caa400f43d
|
|
@ -23,7 +23,9 @@
|
|||
|
||||
**NEO-41 landed:** prototype **gather → skill XP** — successful **`POST …/interact`** when interactable **`kind`** is **`resource_node`** grants **`salvage`** XP (**`sourceKind: activity`**, **10** XP per interact) via shared **`SkillProgressionGrantOperations`** ([implementation plan](../../plans/NEO-41-implementation-plan.md)); manual QA **[`NEO-41.md`](../../manual-qa/NEO-41.md)**; **[server README — Interaction (gather)](../../../server/README.md#interaction-neo-9)**.
|
||||
|
||||
**Still backlog within this module:** Slice 3 **NEO-42**, **NEO-43** (and **NEO-44** gig path). Keep this paragraph and [documentation tracking](documentation_and_implementation_alignment.md) in sync as Slice 2 stories merge.
|
||||
**NEO-42 landed (prep):** **`RefineActivitySkillXpGrant`** + **`RefineSkillXpConstants`** + test **`RefineActivityDeniedRegistryWebApplicationFactory`** — craft/refine success is intended to award **`refine`** XP (**`sourceKind: activity`**, **10** XP) via the same **`SkillProgressionGrantOperations`** stack as NEO-38 ([implementation plan](../../plans/NEO-42-implementation-plan.md)); manual QA **[`NEO-42.md`](../../manual-qa/NEO-42.md)**; **[server README — Craft / refine hook (NEO-42)](../../../server/README.md#craft--refine-hook--skill-xp-neo-42)**. **E3.M2** must invoke **`GrantOnSuccessfulCraftOrRefine`** on real **`CraftResult`** success for end-to-end behavior (no craft route in repo at this slice).
|
||||
|
||||
**Still backlog within this module:** Slice 3 **NEO-43** (and **NEO-44** gig path); **NEO-42** end-to-end wiring awaits **E3.M2** success handler. Keep this paragraph and [documentation tracking](documentation_and_implementation_alignment.md) in sync as Slice 2 stories merge.
|
||||
|
||||
## Purpose
|
||||
|
||||
|
|
|
|||
|
|
@ -9,6 +9,10 @@
|
|||
| **Stage target** | Prototype |
|
||||
| **Status** | Planned (see [dependency register](module_dependency_register.md)) |
|
||||
|
||||
## Implementation snapshot
|
||||
|
||||
**Prep (NEO-42):** Server ships **`RefineActivitySkillXpGrant.GrantOnSuccessfulCraftOrRefine`** + **`RefineSkillXpConstants`** under `server/NeonSprawl.Server/Game/Skills/` — delegates to **`SkillProgressionGrantOperations.TryApplyGrant`** with **`refine`** + **`activity`** (prototype **10** XP), same persistence and **`allowedXpSourceKinds`** rules as **`POST …/skill-progression`**. **No** `CraftRequest` / **`CraftResult`** route yet; when this module adds authoritative craft/refine success, call that helper once on success ([NEO-42 implementation plan](../../plans/NEO-42-implementation-plan.md); [server README — Craft / refine hook (NEO-42)](../../../server/README.md#craft--refine-hook--skill-xp-neo-42)).
|
||||
|
||||
## Purpose
|
||||
|
||||
Implements the processing and crafting pipeline: raw-to-refined flows and recipe execution with server-authoritative inventory mutations. Bridges gathered materials ([E3.M1](E3_M1_ResourceNodeAndGatherLoop.md)) and item definitions ([E3.M3](E3_M3_ItemizationAndInventorySchema.md)) into usable outputs for combat, quests, and economy loops.
|
||||
|
|
|
|||
|
|
@ -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). **Slice 3 integration** — [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), [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), [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). **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 |
|
||||
| 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/` |
|
||||
|
||||
---
|
||||
|
|
|
|||
|
|
@ -35,7 +35,7 @@
|
|||
|
||||
## Acceptance criteria checklist
|
||||
|
||||
- [x] Craft/refine completion **shall** grant skill XP via **NEO-38** path — **`RefineActivitySkillXpGrant`** wraps **`TryApplyGrant`** (same stack as **`POST …/skill-progression`**). **E3.M2** must call **`GrantOnSuccessfulCraftOrRefine`** on success (documented in **`server/README.md`** + manual QA); no craft route in repo yet.
|
||||
- [x] **Preparatory:** **`RefineActivitySkillXpGrant`** invokes the **NEO-38** path (**`TryApplyGrant`**, same stack as **`POST …/skill-progression`**). **Follow-up:** **E3.M2** success handler must call **`GrantOnSuccessfulCraftOrRefine`** (documented in **`server/README.md`** + manual QA); no craft route in repo yet.
|
||||
- [x] Grant channels respect **skill catalog** `allowedXpSourceKinds` (stub registry test: **`refine`** without **`activity`** → no XP).
|
||||
|
||||
## Decisions (implementation)
|
||||
|
|
|
|||
|
|
@ -0,0 +1,66 @@
|
|||
# Code review: NEO-42 (craft/refine → refine skill XP, prep helper)
|
||||
|
||||
**Date:** 2026-05-10
|
||||
|
||||
**Scope:** Branch `NEO-42-craft-refine-skill-xp-activity` (commits `20a141b`, `15bf598`) vs `origin/main`. Linear issue NEO-42. Diff: 7 files, +377 / -0.
|
||||
|
||||
**Base:** `origin/main`
|
||||
|
||||
---
|
||||
|
||||
## Verdict
|
||||
|
||||
**Approve with nits** — Plan-aligned preparatory slice: adds `RefineActivitySkillXpGrant` + `RefineSkillXpConstants` (parallel to NEO-41 gather constants), defensive deny-path test factory, and docs (server README, manual QA, implementation plan). No live E3.M2 call site exists yet (module absent), which is explicitly acknowledged in the plan, README, and manual QA. All `dotnet test NeonSprawl.sln` tests pass (**139** total, **3** new under `RefineActivitySkillXpGrantTests`). Two doc-alignment suggestions and a few nits below.
|
||||
|
||||
## Summary
|
||||
|
||||
This PR adds a thin static helper `RefineActivitySkillXpGrant.GrantOnSuccessfulCraftOrRefine(playerId, registry, xpStore, levelCurve)` that delegates to `SkillProgressionGrantOperations.TryApplyGrant` with `refine` + `activity` + **10** XP, plus a constants holder. There is no recipe/craft execution path in the repo to call it from, so the helper is shipped as a known callsite for a future E3.M2 merge. Three xUnit tests use the existing AAA pattern: a happy-path that asserts the in-memory store gained 10 XP under `refine`, a happy-path that round-trips through `GET …/skill-progression`, and a deny path using a new `RefineActivityDeniedRegistryWebApplicationFactory` (refine catalog row without `activity` in `allowedXpSourceKinds`). Server README and `docs/manual-qa/NEO-42.md` document the contract and verification flow via the existing `POST …/skill-progression` control endpoint. **Risk:** Low; pure additive surface, server authority unchanged, helper is unreferenced until E3.M2 wires it.
|
||||
|
||||
## Documentation checked
|
||||
|
||||
- `docs/plans/NEO-42-implementation-plan.md` — **matches** (kickoff clarifications honored, both acceptance checkboxes correspond to landed artifacts, files added/modified list matches diff, helper-then-E3.M2-call shape preserved).
|
||||
- `docs/decomposition/modules/module_dependency_register.md` — **matches** for E3.M2 row (kept **Planned**, defensible since no `CraftRequest` / `RecipeDef` / `CraftResult` implemented); **matches** for E2.M2 row (already **In Progress**).
|
||||
- `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — **matches** (post-follow-up: **NEO-42 landed (prep)** paragraph + backlog text updated). ~~**partially matches**: the "Implementation snapshot" still asserts "Still backlog within this module: Slice 3 NEO-42, NEO-43"; NEO-42 has landed as a preparatory helper and should get a parallel "NEO-42 landed (prep)" paragraph the way NEO-41 did. See Suggestion 1.~~ **Done.**
|
||||
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches** (E2.M2 tracking row extended with NEO-42 prep + plan link). ~~**partially matches**: the E2.M2 tracking-table row enumerates NEO-37 → NEO-41 with landed notes but does not yet add a NEO-42 fragment. See Suggestion 1.~~ **Done.**
|
||||
- `docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md` — **matches** (**Implementation snapshot** prep paragraph added). ~~**partially matches**: doc still has no Implementation snapshot; adding a single bullet noting the preparatory helper + plan would help E3.M2 readers find the wiring point when they later implement `CraftResult`. See Suggestion 2.~~ **Done.**
|
||||
- `docs/manual-qa/NEO-42.md` — **matches** (`book_or_item` is correctly outside `refine.allowedXpSourceKinds = [activity, mission_reward, trainer]` in `content/skills/prototype_skills.json`; control POST + GET steps reflect the real grant path).
|
||||
- `server/README.md` (diff) — **matches** plan (Craft / refine hook → skill XP (NEO-42) section).
|
||||
- Cross-cutting `client_server_authority.md` / `contracts.md` — **N/A** (no new public surface; helper lives behind future server caller).
|
||||
|
||||
**Register / tracking:** ~~Implementation tracking table E2.M2 row needs a NEO-42 paragraph; register Status fields require no change.~~ E2.M2 row updated; register Status fields unchanged. **Done.**
|
||||
|
||||
## Blocking issues
|
||||
|
||||
_None._
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. ~~**Update E2.M2 implementation-snapshot docs to reflect NEO-42 landing.** Two locations:~~ **Done.**
|
||||
- ~~`docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — replace / supplement the line "Still backlog within this module: Slice 3 NEO-42, NEO-43" with a "**NEO-42 landed (prep):** `RefineActivitySkillXpGrant` + `RefineSkillXpConstants` + deny-path factory; awaiting E3.M2 success handler to invoke" paragraph, mirroring the NEO-41 paragraph.~~
|
||||
- ~~`docs/decomposition/modules/documentation_and_implementation_alignment.md` — extend the E2.M2 tracking row's snapshot column with the equivalent NEO-42 sentence and add the plan link. This keeps the same pattern NEO-41 set in `0a141ba`-style follow-ups.~~
|
||||
|
||||
2. ~~**Add an Implementation snapshot to `E3_M2_RefinementAndRecipeExecution.md`.** Status can remain **Planned** in the register (no `CraftRequest` / `RecipeDef` / `CraftResult` yet), but a one-paragraph snapshot under Summary noting "preparatory helper `RefineActivitySkillXpGrant` ready for the future success handler; plan: NEO-42" would help the next implementer find the existing wiring point. Mirrors how NEO-25 / NEO-26 noted partial work without prematurely flipping module status.~~ **Done.**
|
||||
|
||||
## Nits
|
||||
|
||||
- ~~**Nit (test 3 assertion strength):** `RefineActivitySkillXpGrantTests.GrantOnSuccessfulCraftOrRefine_WhenRefineDisallowsActivity_ShouldLeaveRefineXpZero` currently does:~~ **Done.** (`Assert.False(totals.ContainsKey("refine"));`)
|
||||
|
||||
```csharp
|
||||
var totals = xpStore.GetXpTotals("dev-local-1");
|
||||
_ = totals.TryGetValue("refine", out var refineXp);
|
||||
Assert.Equal(0, refineXp);
|
||||
```
|
||||
|
||||
~~Because `TryApplyGrant` returns before `TryApplyXpDelta` on a deny, the `refine` key should never exist in the store. `Assert.False(totals.ContainsKey("refine"));` (or `Assert.False(totals.TryGetValue("refine", out _));`) expresses that semantic exactly and would catch a regression where the store unexpectedly created a 0-valued entry. Current form passes both states.~~
|
||||
|
||||
- ~~**Nit (helper symmetry with gather call site):** `RefineActivitySkillXpGrant.GrantOnSuccessfulCraftOrRefine` calls `playerId.Trim()` before delegating, but the NEO-41 inline call in `InteractionApi.cs` passes the already-normalized `playerKey` straight through. Harmless either way (store re-normalizes), but picking one direction across both gather and refine hooks keeps callers consistent for whoever wires E3.M2 next.~~ **Done.** (Trim removed; `<remarks>` documents normalized `playerId`.)
|
||||
|
||||
- **Nit (factory duplication):** `RefineActivityDeniedRegistryWebApplicationFactory` is ~90% identical to `SalvageActivityDeniedRegistryWebApplicationFactory` (only the stub-registry composition and namespace differ). Optional follow-up: extract a small `InMemoryFactoryWithSkillRegistryOverride<TRegistry>` base in `NeonSprawl.Server.Tests` to keep future deny-path factories DRY (combat XP, intrusion, etc.). Not worth blocking on for two factories.
|
||||
|
||||
- ~~**Nit (acceptance-criteria phrasing):** Plan checkbox "Craft/refine completion **shall** grant skill XP via NEO-38 path" is ticked, but no craft/refine completion path exists yet. The plan acknowledges this in the same bullet ("E3.M2 must call `GrantOnSuccessfulCraftOrRefine` on success … no craft route in repo yet"). Consider rewording to "**Preparatory** helper invokes NEO-38 grant path; E3.M2 success handler wiring tracked as follow-up" so future readers don't misread the box as a fully implemented end-to-end behavior. Optional.~~ **Done.**
|
||||
|
||||
## Verification
|
||||
|
||||
- `dotnet test NeonSprawl.sln` — **passed** (**139** tests, including the 3 new `RefineActivitySkillXpGrantTests`).
|
||||
- Manual: follow `docs/manual-qa/NEO-42.md` — `POST …/move` → `POST …/skill-progression` with `{skillId: "refine", sourceKind: "activity", amount: 10}` → expect `granted: true`, then re-`POST` with `sourceKind: "book_or_item"` → expect `granted: false`, `reasonCode: "source_kind_not_allowed"`.
|
||||
- After E3.M2 lands: confirm a single call to `RefineActivitySkillXpGrant.GrantOnSuccessfulCraftOrRefine` on the craft-success branch (not on deny / cost-failure) and re-verify via `GET …/skill-progression`.
|
||||
|
|
@ -74,9 +74,8 @@ public sealed class RefineActivitySkillXpGrantTests
|
|||
// Act
|
||||
RefineActivitySkillXpGrant.GrantOnSuccessfulCraftOrRefine("dev-local-1", registry, xpStore, levelCurve);
|
||||
|
||||
// Assert
|
||||
// Assert — deny returns before TryApplyXpDelta; refine row must not appear in stored XP map.
|
||||
var totals = xpStore.GetXpTotals("dev-local-1");
|
||||
_ = totals.TryGetValue("refine", out var refineXp);
|
||||
Assert.Equal(0, refineXp);
|
||||
Assert.False(totals.ContainsKey("refine"));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ namespace NeonSprawl.Server.Game.Skills;
|
|||
public static class RefineActivitySkillXpGrant
|
||||
{
|
||||
/// <summary>Applies one <c>refine</c> + <c>activity</c> grant if the catalog allowlist permits it.</summary>
|
||||
/// <remarks>Pass a normalized <paramref name="playerId"/> (e.g. route-trimmed), matching <c>InteractionApi</c> gather hook callers.</remarks>
|
||||
public static void GrantOnSuccessfulCraftOrRefine(
|
||||
string playerId,
|
||||
ISkillDefinitionRegistry registry,
|
||||
|
|
@ -15,7 +16,7 @@ public static class RefineActivitySkillXpGrant
|
|||
ISkillLevelCurve levelCurve)
|
||||
{
|
||||
_ = SkillProgressionGrantOperations.TryApplyGrant(
|
||||
playerId.Trim(),
|
||||
playerId,
|
||||
RefineSkillXpConstants.RefineSkillId,
|
||||
RefineSkillXpConstants.ActivityXpPerCraftRefineCompletion,
|
||||
RefineSkillXpConstants.ActivitySourceKind,
|
||||
|
|
|
|||
Loading…
Reference in New Issue