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

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. Done. Persistence test now uses separate Arrange / Act (write) / Act (read) / Assert phases.

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. Done. [Theory] with four empty/whitespace player+quest id cases added to InMemoryPlayerQuestStateStoreTests.

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. Done. Added GatherIntroQuestId, GatherIntroFirstObjectiveId, and ChainFirstObjectiveId to PrototypeE7M1QuestCatalogRules; tests updated.

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. Done. Bruno health smoke noted in plan reconciliation and server/README.md.

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. Done.

• 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. Done.

• 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. Done. Uses GetFieldValue<string>(2).

• 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. Done. README links bruno/neon-sprawl-server/quest-progress/ (health smoke until E7M1-08).

Verification

# 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.