From af87b5d12c3677f8cfbde742c1cd977a0ed3190c Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 3 Jun 2026 23:16:48 -0400 Subject: [PATCH] NEO-116: Add code review for player quest state store. --- docs/reviews/2026-06-03-NEO-116.md | 71 ++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) create mode 100644 docs/reviews/2026-06-03-NEO-116.md diff --git a/docs/reviews/2026-06-03-NEO-116.md b/docs/reviews/2026-06-03-NEO-116.md new file mode 100644 index 0000000..4bf5890 --- /dev/null +++ b/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 `` references a type not yet in the repo (NEO-117). Harmless for compile; consider `QuestStateOperations` 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`** 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.