From 065872405ce96faa538421d529faaff5091bf83a Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 3 Jun 2026 23:01:59 -0400 Subject: [PATCH] NEO-116: Add kickoff implementation plan for player quest state store. Documents store contract, Postgres V008, tests, and kickoff decisions (in-memory + Postgres when configured) before E7M1-06 operations land. --- docs/plans/NEO-116-implementation-plan.md | 151 ++++++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 docs/plans/NEO-116-implementation-plan.md diff --git a/docs/plans/NEO-116-implementation-plan.md b/docs/plans/NEO-116-implementation-plan.md new file mode 100644 index 0000000..fc114f3 --- /dev/null +++ b/docs/plans/NEO-116-implementation-plan.md @@ -0,0 +1,151 @@ +# NEO-116 — Implementation plan + +## Story reference + +| Field | Value | +|--------|--------| +| **Key** | NEO-116 | +| **Title** | E7M1-05: Player quest state store + QuestStepState persistence | +| **Linear** | https://linear.app/neon-sprawl/issue/NEO-116/e7m1-05-player-quest-state-store-queststepstate-persistence | +| **Module** | [E7.M1 — QuestStateMachine](../decomposition/modules/E7_M1_QuestStateMachine.md) · Epic 7 Slice 1 · backlog **E7M1-05** | +| **Branch** | `NEO-116-player-quest-state-store` | +| **Precursor** | [NEO-114](https://linear.app/neon-sprawl/issue/NEO-114) — quest definition registry (**Done**); [NEO-115](https://linear.app/neon-sprawl/issue/NEO-115) — GET `/game/world/quest-definitions` (**landed on `main`**) | +| **Pattern** | [NEO-104](NEO-104-implementation-plan.md) — keyed in-memory store + atomic mutations; [NEO-44](NEO-44-implementation-plan.md) — Postgres when configured + persistence integration test | +| **Blocks** | [NEO-117](https://linear.app/neon-sprawl/issue/NEO-117) — `QuestStateOperations` (accept, step advance, complete); E7M1-07+ objective wiring | +| **Client counterpart** | None — server-only store; player-visible progress starts at [NEO-122](https://linear.app/neon-sprawl/issue/NEO-122) / [NEO-123](https://linear.app/neon-sprawl/issue/NEO-123) | + +## Kickoff clarifications + +| Topic | Question | Agent recommendation | Answer | +|--------|----------|----------------------|--------| +| **Postgres persistence** | Include `V008` migration + `PostgresPlayerQuestStateStore` in NEO-116 vs defer? | **Include when configured** — E7M1 backlog default is in-memory **+ optional Postgres**; mirror `AddGigProgressionStore` / `PostgresPlayerGigProgressionStore`. | **Adopted** — user chose in-memory + Postgres when configured. | +| **Store API shape** | Mutation methods on store vs minimal `TryGet` + `TryReplace` only? | **Mutation methods on store** — `TryActivate`, `TryAdvanceStep`, `TryUpdateObjectiveCounter`, `TryMarkComplete`; E7M1-06 `QuestStateOperations` adds prerequisite/registry validation on top (NEO-104 encounter store precedent). | **Adopted** (agent default; no objection). | +| **`not_started` representation** | Missing row vs explicit row with `not_started` status? | **Implicit missing row** — encounter progress pattern; E7M1-08 HTTP merges registry + store. | **Adopted** (agent default). | +| **DI registration** | Extend `AddQuestDefinitionCatalog` vs separate extension? | **Separate `AddPlayerQuestStateStore`** — persistence separate from catalog load (NEO-113/114), like `AddGigProgressionStore`. | **Adopted** (agent default). | +| **Objective counters scope** | Counters for current step only vs full quest history? | **Current step only** — reset counters on `TryAdvanceStep`; sufficient for E7M1-07 quantity objectives. | **Adopted** (agent default). | + +## Goal, scope, and out-of-scope + +**Goal:** Durable per-player quest progress rows keyed by **`(playerId, questId)`**: status (`not_started` implicit / `active` / `completed`), current step index, per-objective counters for the active step, and completion timestamp. Provide **`IPlayerQuestStateStore`** with in-memory and Postgres implementations registered in DI. + +**In scope (from Linear + [E7M1-05](E7M1-prototype-backlog.md#e7m1-05--player-quest-state-store--queststepstate-persistence)):** + +- **`QuestStepState`** snapshot type and **`PlayerQuestProgressRow`** (or equivalent immutable read model). +- **`QuestProgressStatus`** enum: `Active`, `Completed` (missing row ⇒ `not_started` at call sites). +- **`IPlayerQuestStateStore`** + **`InMemoryPlayerQuestStateStore`** (dev player bucket seeded; thread-safe). +- **`PostgresPlayerQuestStateStore`** + **`PostgresPlayerQuestProgressBootstrap`** + **`V008__player_quest_progress.sql`** when `ConnectionStrings:NeonSprawl` is set. +- **`QuestProgressIds`** — player/quest id normalization + composite key helper (mirror `EncounterProgressIds`). +- **`QuestStateServiceCollectionExtensions.AddPlayerQuestStateStore`** wired from **`Program.cs`**. +- Unit tests (AAA): accept (`not_started` → `active`), step advance + counter updates, idempotent complete, completed row cannot regress to active. +- Postgres persistence integration test (`RequirePostgresFact`): write via store on first factory, read on second factory. +- `server/README.md` quest progress store section (brief). +- Update alignment register E7.M1 row when complete. + +**Out of scope (from Linear + backlog):** + +- **`QuestStateOperations`** prerequisite enforcement and reason codes ([NEO-117](https://linear.app/neon-sprawl/issue/NEO-117) / E7M1-06). +- Gather/craft/encounter objective wiring (E7M1-07). +- HTTP **`GET /game/players/{id}/quest-progress`** (E7M1-08) and accept POST (E7M1-09). +- Bruno, Godot, dev fixture reset API for quest rows (may add in NEO-108-style story if needed). +- Reward grants / E7.M2 bundles. + +## Acceptance criteria checklist + +- [ ] Store supports read/update per `(playerId, questId)`. +- [ ] Completed quest cannot regress to active without explicit reset API (none in prototype — store denies `TryActivate` / `TryAdvanceStep` / counter updates when completed). +- [ ] Idempotent complete: second `TryMarkComplete` returns `false` without changing `CompletedAt` (Linear AC). + +## Technical approach + +1. **Types (`Game/Quests/`)** + - **`QuestProgressStatus`**: `Active`, `Completed`. + - **`QuestStepState`**: immutable snapshot for one quest row — `PlayerId`, `QuestId`, `Status`, `CurrentStepIndex` (0-based), `IReadOnlyDictionary ObjectiveCounters` (objective id → accumulated count for **current step only**), optional `CompletedAt`. + - **`QuestProgressIds`**: `NormalizePlayerId`, `NormalizeQuestId`, `MakeProgressKey` (trim + lowercase; empty guard). + +2. **`IPlayerQuestStateStore`** + - `bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot)` — missing row ⇒ `false` (callers treat as `not_started`). + - `bool TryActivate(string playerId, string questId, out QuestStepState snapshot)` — creates row at step 0, empty counters; `true` on first activation; `false` if already active or completed or unknown player (in-memory: no dev bucket). + - `bool TryAdvanceStep(string playerId, string questId, int newStepIndex, out QuestStepState snapshot)` — requires active row; sets index, **clears** objective counters; denies when completed or `newStepIndex` ≤ current. + - `bool TryUpdateObjectiveCounter(string playerId, string questId, string objectiveId, int newCount, out QuestStepState snapshot)` — requires active row; sets counter for objective id (non-negative); denies when completed. + - `bool TryMarkComplete(string playerId, string questId, DateTimeOffset completedAt, out QuestStepState snapshot)` — first mark `true`; replay `false`; preserves first `CompletedAt`. + - XML remarks: game code should inject this interface; **`QuestStateOperations`** (NEO-117) and HTTP (E7M1-08) consume it — not the catalog. + +3. **`InMemoryPlayerQuestStateStore`** + - `ConcurrentDictionary` keyed by `MakeProgressKey`; per-key locks. + - Seed configured dev player bucket on construction (`GamePositionOptions.DevPlayerId`) — empty inner map (all quests implicit `not_started`). + - `CanWritePlayer` semantics: returns false when player has no bucket (mirror gig/inventory stores). + +4. **Postgres (`V008__player_quest_progress.sql`)** + - Table **`player_quest_progress`**: `player_id` FK → `player_position`, `quest_id`, `status` (`active` / `completed`), `current_step_index`, `objective_counters` JSONB (objective id → int map), `completed_at` TIMESTAMPTZ NULL, `updated_at`, PK `(player_id, quest_id)`. + - **`PostgresPlayerQuestProgressBootstrap.EnsureSchema`** — idempotent DDL from migration file (NEO-44 pattern). + - **`PostgresPlayerQuestStateStore`** — same interface; deny writes when player missing from `player_position`; use transactions + `FOR UPDATE` on mutations. + +5. **DI** + - **`QuestStateServiceCollectionExtensions.AddPlayerQuestStateStore(configuration)`**: + - When connection string set → `PostgresPlayerQuestStateStore`. + - Else → `InMemoryPlayerQuestStateStore`. + - Call from **`Program.cs`** after quest catalog registration. + - Extend **`InMemoryWebApplicationFactory`** — strip Postgres quest store + register in-memory implementation (same list as gig progression). + +6. **Tests** + - **`InMemoryPlayerQuestStateStoreTests`**: AAA coverage for activate, advance (counter reset), counter update, idempotent complete, completed-row denial on activate/advance/counter, unknown player false, host DI resolve. + - **`PlayerQuestProgressPersistenceIntegrationTests`**: direct store mutation on `PostgresWebApplicationFactory`, second factory reads same row (no HTTP — store-only story). + +7. **Docs** + - `server/README.md` — quest progress store section (interface methods, implicit `not_started`, Postgres table, note HTTP deferred to E7M1-08). + - `documentation_and_implementation_alignment.md` E7.M1 row — note NEO-116 store landed; register stays **Planned** until E7M1-06+ runtime. + +### Prototype store behavior (frozen for tests) + +| Step | Expected | +|------|----------| +| Missing row | `TryGetProgress` false; `TryActivate` → active, step 0, empty counters | +| Re-activate | `TryActivate` false (already active) | +| Counter | `TryUpdateObjectiveCounter(..., "gather_scrap", 3)` → counter map updated | +| Advance | `TryAdvanceStep(..., 1)` → step 1, counters cleared | +| Complete | `TryMarkComplete` once → true, status completed; second → false | +| Regression guard | After complete, `TryActivate` / `TryAdvanceStep` / `TryUpdateObjectiveCounter` all false | + +Use frozen quest id **`prototype_quest_gather_intro`** and objective id from catalog in tests (load from `PrototypeE7M1QuestCatalogRules` / first step objective where practical). + +## Files to add + +| Path | Purpose | +|------|---------| +| `server/NeonSprawl.Server/Game/Quests/QuestProgressStatus.cs` | Status enum for active/completed rows. | +| `server/NeonSprawl.Server/Game/Quests/QuestStepState.cs` | Immutable progress snapshot (module contract). | +| `server/NeonSprawl.Server/Game/Quests/QuestProgressIds.cs` | Player/quest id normalization and composite key. | +| `server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs` | Store contract. | +| `server/NeonSprawl.Server/Game/Quests/InMemoryPlayerQuestStateStore.cs` | Thread-safe in-memory implementation. | +| `server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestProgressBootstrap.cs` | Applies V008 DDL once per process. | +| `server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs` | PostgreSQL-backed implementation. | +| `server/NeonSprawl.Server/Game/Quests/QuestStateServiceCollectionExtensions.cs` | DI: Postgres when configured, else in-memory. | +| `server/db/migrations/V008__player_quest_progress.sql` | Quest progress table DDL. | +| `server/NeonSprawl.Server.Tests/Game/Quests/InMemoryPlayerQuestStateStoreTests.cs` | AAA unit + host DI tests. | +| `server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs` | Postgres cross-factory persistence test. | +| `docs/plans/NEO-116-implementation-plan.md` | This plan. | + +## Files to modify + +| Path | Rationale | +|------|-----------| +| `server/NeonSprawl.Server/Program.cs` | Register `AddPlayerQuestStateStore` after quest catalog. | +| `server/NeonSprawl.Server.Tests/InMemoryWebApplicationFactory.cs` | Force in-memory quest store in unit/integration tests (strip Postgres registration). | +| `server/README.md` | Document quest progress store, V008 table, implicit `not_started`, deferred HTTP. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | E7.M1 row — note NEO-116 store when complete. | + +## Tests + +| Test file | What it covers | +|-----------|----------------| +| `InMemoryPlayerQuestStateStoreTests.cs` | **Unit:** `TryActivate` creates active row at step 0; second activate false. **Unit:** `TryUpdateObjectiveCounter` updates map; denied when completed. **Unit:** `TryAdvanceStep` bumps index and clears counters; denied when completed or non-increasing index. **Unit:** `TryMarkComplete` idempotent (second false, `CompletedAt` unchanged). **Unit:** completed row denies activate/advance/counter. **Unit:** unknown player / empty ids false. **Host:** `InMemoryWebApplicationFactory` resolves `IPlayerQuestStateStore`; smoke activate + get on dev player + frozen quest id. | +| `PlayerQuestProgressPersistenceIntegrationTests.cs` | **Postgres:** `TryActivate` + `TryUpdateObjectiveCounter` + `TryMarkComplete` on first factory; second `PostgresWebApplicationFactory` `TryGetProgress` returns completed row with same counters-at-complete semantics and `CompletedAt`. | + +## Open questions / risks + +| Question / risk | Agent recommendation | Status | +|-----------------|----------------------|--------| +| **JSONB vs normalized objective counter table** | Single JSONB column on `player_quest_progress` — flexible for variable objectives per step; prototype four quests only. | **adopted** | +| **Quest id normalization** | Lowercase + trim via `QuestProgressIds`; store keys use normalized quest id; align with `IQuestDefinitionRegistry.TryNormalizeKnown` at operation layer (NEO-117). | **adopted** | +| **Dev fixture quest reset** | Omit this story; Bruno quest flows may need clear hook in E7M1-08/09 or follow NEO-108 encounter reset pattern. | **deferred** | +| **Constants for tests** | Reuse `PrototypeE7M1QuestCatalogRules.ExpectedQuestIds` / gather intro id; avoid duplicating quest id strings. | **adopted** |