NEO-116: Add code review for player quest state store.

docs/reviews/2026-06-03-NEO-116.md

@@ -0,0 +1,71 @@
+# Code review — NEO-116 (E7M1-05)
+
+**Date:** 2026-06-03  
+**Scope:** Branch `NEO-116-player-quest-state-store` vs `origin/main` — commits `0658724` … `395b70c`  
+**Base:** `origin/main`
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+NEO-116 adds **`IPlayerQuestStateStore`** with thread-safe **`InMemoryPlayerQuestStateStore`** (dev-player bucket) and **`PostgresPlayerQuestStateStore`** (`V008__player_quest_progress.sql`, JSONB objective counters, `FOR UPDATE` mutations). **`QuestStepState`**, **`QuestProgressStatus`**, and **`QuestProgressIds`** mirror the NEO-104 encounter store and NEO-44 Postgres patterns. DI via **`AddPlayerQuestStateStore`** is wired in **`Program.cs`**; tests force in-memory registration in **`InMemoryWebApplicationFactory`**. Eleven AAA unit/host tests cover activate, counters, step advance with counter reset, idempotent complete, completed-row denial, and unknown player; a Postgres cross-factory integration test validates persistence. Documentation (implementation plan, backlog AC, E7.M1 module page, alignment register, dependency register, `server/README.md`) is updated. Server-only infrastructure — correct scope. Risk is low; main follow-ups are test-layout consistency and minor plan/doc reconciliation for the Bruno health smoke.
+
+## Documentation checked
+
+| Path | Result |
+|------|--------|
+| `docs/plans/NEO-116-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation section accurate. |
+| `docs/plans/E7M1-prototype-backlog.md` (E7M1-05) | **Matches** — AC checked; in/out of scope aligned except Bruno note below. |
+| `docs/decomposition/modules/E7_M1_QuestStateMachine.md` | **Matches** — store bullet documents interface, V008, deferred HTTP/operations. |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M1 row notes NEO-116 store; register stays **Planned** until E7M1-06+ runtime. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E7.M1 note appended for E7M1-05 / NEO-116. |
+| `server/README.md` | **Matches** — quest progress store section with methods, implicit `not_started`, V008, NEO-117 deferral. |
+| Full-stack epic decomposition | **N/A** — server-only; client counterpart explicitly deferred (NEO-122 / NEO-123). |
+
+**Register / tracking:** No register **Status** promotion required — E7.M1 correctly remains **Planned** until **`QuestStateOperations`** (NEO-117) lands.
+
+## Blocking issues
+
+(none)
+
+## Suggestions
+
+1. **Persistence integration test AAA layout** — `PlayerQuestProgressPersistenceIntegrationTests.ActivateCounterComplete_ShouldPersistAcrossNewFactory` labels store writes as `// Act` inside the `// Arrange` block. Match **`PlayerInventoryPersistenceIntegrationTests`**: reset in Arrange; first `using` scope = Act (write); second factory scope = Act (read); Assert last.
+
+2. **Empty-id denial tests** — Implementation plan test table lists “unknown player / empty ids false”; only unknown player is covered. Add a `[Fact]` (or theory) asserting `TryActivate` / `TryGetProgress` return false for empty/whitespace `playerId` or `questId`.
+
+3. **Frozen quest id constants in tests** — Plan adopted reusing **`PrototypeE7M1QuestCatalogRules`**; tests use **`ChainQuestId`** but hardcode **`prototype_quest_gather_intro`** and objective ids. A **`GatherIntroQuestId`** constant (parallel to **`ChainQuestId`**) would avoid drift; optional helper for first-step objective ids if reused in NEO-117.
+
+4. **Plan reconciliation for Bruno** — Kickoff/plan **out of scope** lists Bruno; branch adds `bruno/neon-sprawl-server/quest-progress/Health after quest state store load.bru` (health smoke only). Harmless, but add a one-line note under **Implementation reconciliation** so the plan matches shipped artifacts.
+
+## Nits
+
+- Nit: **`IPlayerQuestStateStore`** XML `<see cref="QuestStateOperations"/>` references a type not yet in the repo (NEO-117). Harmless for compile; consider `<c>QuestStateOperations</c>` until NEO-117 lands if doc warnings appear.
+
+- Nit: **`PostgresPlayerQuestStateStore.TryActivate`** returns false on duplicate row without explicit **`tx.Rollback()`** (relies on dispose). **`PostgresPlayerGigProgressionStore`** rolls back explicitly on failure paths — aligning would improve consistency.
+
+- Nit: **`ReadSnapshot`** uses **`reader.GetString(2)`** for JSONB counters; works if Npgsql maps JSONB as string (integration test validates when Postgres CI runs). If driver behavior changes, prefer **`GetFieldValue<string>`** or typed JSON read — same pattern worth watching in future JSONB stores.
+
+- Nit: Bruno health folder is useful startup smoke but not linked from `server/README.md` (unlike NEO-115 quest-definitions Bruno link). Optional README mention when HTTP lands in E7M1-08.
+
+## Verification
+
+```bash
+# Quest progress store (primary signal for this story)
+cd server
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~InMemoryPlayerQuestStateStoreTests|FullyQualifiedName~PlayerQuestProgressPersistence"
+
+# Quest registry / world API regression (precursors)
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj \
+  --filter "FullyQualifiedName~QuestDefinitionRegistryTests|FullyQualifiedName~QuestDefinitionsWorldApiTests"
+
+# Full server suite (CI)
+dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
+```
+
+**Reviewer note:** `InMemoryPlayerQuestStateStoreTests` + host DI test (11 tests) passed locally during review. Postgres integration test requires **`ConnectionStrings__NeonSprawl`** (`RequirePostgresFact`).
+
+**Manual:** Optional Bruno `bruno/neon-sprawl-server/quest-progress/Health after quest state store load.bru` against dev server on port 5253 to confirm DI wiring after startup.