NEO-38: add implementation plan for skill XP grant and level-up
parent
9e8914708f
commit
58b9016d40
|
|
@ -0,0 +1,107 @@
|
|||
# NEO-38 — Implementation plan
|
||||
|
||||
## Story reference
|
||||
|
||||
| Field | Value |
|
||||
|--------|--------|
|
||||
| **Key** | NEO-38 |
|
||||
| **Title** | Apply skill XP grant + allowlist + level-up |
|
||||
| **Linear** | https://linear.app/neon-sprawl/issue/NEO-38/apply-skill-xp-grant-allowlist-level-up |
|
||||
| **Module** | [E2.M2 — XpAwardAndLevelEngine](../decomposition/modules/E2_M2_XpAwardAndLevelEngine.md) |
|
||||
| **Blocked by** | [NEO-37](https://linear.app/neon-sprawl/issue/NEO-37) (read snapshot) — implemented on `main`; this branch builds on that contract. |
|
||||
|
||||
## Kickoff clarifications
|
||||
|
||||
| Topic | Question / note | Resolution |
|
||||
|--------|-----------------|------------|
|
||||
| **Persistence** | NEO-38 AC: follow NEO-29 pattern vs in-memory-only + follow-up | **User:** mirror **Hotbar / NEO-29** — abstract store, **`InMemory*`** fallback, **`Postgres*`** when `ConnectionStrings__NeonSprawl` is set, with schema bootstrap analogous to **`PostgresHotbarLoadoutBootstrap`**. |
|
||||
| **Successful POST body** | Surface level-up in response vs minimal + GET only | User asked for recommendation → **Agent:** **HTTP 200** with **versioned JSON** that includes **full progression snapshot** (aligned with **`GET …/skill-progression`**) plus an explicit **`levelUps`** (or equivalent) array for thresholds crossed on this grant. **User:** confirmed snapshot + level-ups. |
|
||||
|
||||
## Goal, scope, and out-of-scope
|
||||
|
||||
**Goal:** Server-authoritative **apply XP grant** path: validate **`skillId`** (`ISkillDefinitionRegistry.TryGetDefinition`) and **`sourceKind`** against **`SkillDefRow.AllowedXpSourceKinds`**; persist per-player XP; recompute level from an **inline placeholder curve** (acceptable until [NEO-39](https://linear.app/neon-sprawl/issue/NEO-39)); return **structured denies** (stable reason codes, hotbar-style **200 + `granted: false`** where that pattern already fits the repo) and **level-up** data on success.
|
||||
|
||||
**In scope (from Linear):**
|
||||
|
||||
- Reject unknown skill id and disallowed `sourceKind` with **stable reason codes**.
|
||||
- Valid grant updates persistence; **level** increases when thresholds crossed (**`LevelUpEvent`** semantics surfaced in JSON for this story).
|
||||
- Versioned **POST** + automated tests; **Bruno** requests; **`server/README.md`**.
|
||||
- Persistence: **in-memory + Postgres when configured**, per kickoff decision.
|
||||
|
||||
**Out of scope (from Linear):**
|
||||
|
||||
- Data-driven level curve file + CI (**NEO-39**).
|
||||
- Telemetry catalog (**NEO-40**).
|
||||
- Slice 3 activity wiring (**NEO-41**–**43**).
|
||||
|
||||
## Acceptance criteria checklist
|
||||
|
||||
- [ ] Unknown `skillId` rejected; `sourceKind` not in allowlist rejected (**stable reason codes**).
|
||||
- [ ] Valid grant updates stored progression; level increases when thresholds crossed (**level-up** payload on success).
|
||||
- [ ] Versioned **POST** + tests; Bruno; README.
|
||||
- [ ] Persistence: **NEO-29-style** dual backend (in-memory + Postgres when configured).
|
||||
|
||||
## Technical approach
|
||||
|
||||
1. **Route:** **`POST /game/players/{id}/skill-progression`** (same resource as NEO-37 **GET**, mirroring **`GET` + `POST` …/hotbar-loadout**). **Reject** missing/invalid **`schemaVersion`** with **400** (same idea as hotbar update).
|
||||
|
||||
2. **Player gate:** **`404`** when **`!IPositionStateStore.TryGetPosition`**, unchanged from NEO-37.
|
||||
|
||||
3. **Request body (versioned):** e.g. **`schemaVersion`**, **`skillId`**, **`amount`** (positive int), **`sourceKind`** (string matching catalog values: `activity`, `mission_reward`, `trainer`, `book_or_item`). Single grant per request for a small, testable surface; batch grants can be a later issue if needed.
|
||||
|
||||
4. **Validation:** Use **`ISkillDefinitionRegistry`**. If unknown skill → deny with **`unknown_skill`** (exact string TBD but stable and documented). If known skill but **`sourceKind`** not in **`AllowedXpSourceKinds`** (case/whitespace normalization consistent with catalog) → **`source_kind_not_allowed`**. Non-positive **`amount`** → **`invalid_amount`** (or reuse a generic **`bad_grant`** only if we want fewer codes — prefer specific codes per AC).
|
||||
|
||||
5. **Persistence:** Introduce **`IPlayerSkillProgressionStore`** (name may match existing naming conventions) with operations sufficient to **read XP per skill** and **apply a delta** atomically per request, returning new XP and whether level changed. **In-memory** implementation keyed by normalized player id; **Postgres** implementation with **`EnsureSchema`**, **`player_id` + `skill_id`** uniqueness, XP column (level **derived** from XP + curve helper to avoid drift). **No row** for a skill ⇒ treat as **XP 0** / **level 1** at read time (GET and POST deny paths that return current snapshot).
|
||||
|
||||
6. **GET snapshot (NEO-37 evolution):** Replace the hard-coded **0 / 1** builder in **`SkillProgressionSnapshotApi.BuildSnapshot`** with **registry + store merge** so persisted XP appears on **`GET /game/players/{id}/skill-progression`**. Preserve **`schemaVersion` 1** if the JSON shape is unchanged (only values differ). Keep **ordinal sort** for stable serialization; **unordered-by-contract** rule unchanged.
|
||||
|
||||
7. **Level curve (placeholder):** Inline C# thresholds or formula (documented in code comments, **subsumed by NEO-39**). Example pattern: simple per-level XP thresholds up to a small cap so tests can cross **at least one** level-up with realistic amounts.
|
||||
|
||||
8. **Success response:** **HTTP 200**, versioned envelope: e.g. **`granted: true`**, **`progression`** (same shape as **`SkillProgressionSnapshotResponse`** or a shared type), **`levelUps`**: array of **`{ skillId, previousLevel, newLevel }`** (field names camelCase JSON). Empty **`levelUps`** when no threshold crossed.
|
||||
|
||||
9. **Deny response:** **HTTP 200** with **`granted: false`**, **`reasonCode`**, and current **`progression`** snapshot (hotbar **`Updated: false`** pattern) so clients always receive authoritative state for UI sync — unless a case clearly fits **400** (malformed body / wrong schema version).
|
||||
|
||||
10. **Wire-up:** **`AddSkillProgressionStore`** (or similar) in **`Program.cs`** next to **`AddHotbarLoadoutStore`**. **`InMemoryWebApplicationFactory`** forces the in-memory implementation for isolated tests (same replacement pattern as hotbar/position).
|
||||
|
||||
11. **Bruno / docs:** Add **`Post … skill progression grant.bru`** (exact name follows collection style) under **`bruno/neon-sprawl-server/skill-progression/`**; extend **`server/README.md`**. Add **`docs/manual-qa/NEO-38.md`** during implementation per project convention.
|
||||
|
||||
## Files to add
|
||||
|
||||
| Path | Purpose |
|
||||
|------|---------|
|
||||
| `server/NeonSprawl.Server/Game/Skills/IPlayerSkillProgressionStore.cs` | Abstraction: read XP map / apply grant for one player+skill. |
|
||||
| `server/NeonSprawl.Server/Game/Skills/InMemoryPlayerSkillProgressionStore.cs` | In-memory persistence; normalized player keys. |
|
||||
| `server/NeonSprawl.Server/Game/Skills/PostgresPlayerSkillProgressionStore.cs` | Postgres implementation when connection string present. |
|
||||
| `server/NeonSprawl.Server/Game/Skills/PostgresSkillProgressionBootstrap.cs` | Schema ensure (mirror hotbar bootstrap style). |
|
||||
| `server/NeonSprawl.Server/Game/Skills/SkillProgressionServiceCollectionExtensions.cs` | Chooses Postgres vs in-memory from configuration. |
|
||||
| `server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantDtos.cs` | Versioned POST request + success/deny response DTOs (`levelUps`, `reasonCode`, etc.). |
|
||||
| `server/NeonSprawl.Server/Game/Skills/SkillLevelCurvePlaceholder.cs` | Inline threshold/floor helpers until NEO-39 (pure functions, unit-testable). |
|
||||
| `server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionGrantApiTests.cs` | In-memory host: unknown skill, bad source kind, success, level-up, GET reflects POST, **404** player gate, bad schema → **400**. |
|
||||
| `server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionGrantPersistenceIntegrationTests.cs` | Postgres harness: grant survives new host/process (mirror **`HotbarLoadoutPersistenceIntegrationTests`**). |
|
||||
| `bruno/neon-sprawl-server/skill-progression/Post skill progression grant.bru` | Manual smoke for happy + one deny path. |
|
||||
| `docs/manual-qa/NEO-38.md` | Curl/Bruno checklist for grant + GET verification. |
|
||||
|
||||
## Files to modify
|
||||
|
||||
| Path | Rationale |
|
||||
|------|-----------|
|
||||
| `server/NeonSprawl.Server/Game/Skills/SkillProgressionSnapshotApi.cs` | Register **POST**; inject store; change **GET** builder to merge registry + stored XP via placeholder level curve. |
|
||||
| `server/NeonSprawl.Server/Program.cs` | Register **`AddSkillProgressionStore`** and ensure catalog/store order remains valid. |
|
||||
| `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | Swap/replace **`IPlayerSkillProgressionStore`** with in-memory implementation for tests. |
|
||||
| `server/README.md` | Document **POST** grant, reason codes, persistence note, Bruno path, links to plan + manual QA. |
|
||||
|
||||
## Tests
|
||||
|
||||
| Test file | What it covers |
|
||||
|-----------|----------------|
|
||||
| `SkillProgressionGrantApiTests.cs` | **AAA** integration tests: denies (**unknown_skill**, **source_kind_not_allowed**); success increases XP; **levelUps** when threshold crossed; **GET** matches after grant; **404** without position; malformed / wrong **`schemaVersion`** → **400**. |
|
||||
| `SkillProgressionGrantPersistenceIntegrationTests.cs` | Postgres: write grant, new factory/client, **GET** (or POST read-back) shows persisted XP. |
|
||||
| `SkillProgressionSnapshotApiTests.cs` (existing) | Update expectations if **GET** implementation changes from hard-coded zeros to store-backed merge (same defaults when store empty). |
|
||||
|
||||
Bruno + **`docs/manual-qa/NEO-38.md`** supplement automated coverage.
|
||||
|
||||
## Open questions / risks
|
||||
|
||||
- **Reason code naming:** Final strings should be listed once in README and reused in tests/Bruno to avoid drift (prefer **`snake_case`** consistent with **`HotbarLoadoutApi`** constants).
|
||||
- **Placeholder curve vs. content:** Keep thresholds boring and documented; **NEO-39** replaces the helper without changing persistence schema if XP remains the stored value.
|
||||
- **Concurrency:** Postgres store should use a transaction per grant; in-memory uses concurrent structures or per-player locks if needed for tests under parallel load (prototype: acceptable to document single-threaded assumption if minimal).
|
||||
Loading…
Reference in New Issue