NEO-116: Address code review findings for quest state store.

Fix persistence test AAA layout, empty-id denial theory, frozen id constants,
Postgres rollback/JSONB read, XML doc cref, README Bruno link, and review doc.

docs/plans/NEO-116-implementation-plan.md

@@ -60,7 +60,8 @@
 - **Types:** `QuestProgressStatus`, `QuestStepState`, `QuestProgressIds`.
 - **Store:** `IPlayerQuestStateStore`; `InMemoryPlayerQuestStateStore` (dev player seeded); `PostgresPlayerQuestStateStore` + `V008__player_quest_progress.sql` when Postgres configured.
 - **DI:** `AddPlayerQuestStateStore` in `Program.cs`; in-memory override in `InMemoryWebApplicationFactory`.
-- **Tests:** `InMemoryPlayerQuestStateStoreTests` (10 AAA cases + host DI); `PlayerQuestProgressPersistenceIntegrationTests` (`RequirePostgresFact`).
+- **Tests:** `InMemoryPlayerQuestStateStoreTests` (AAA cases + host DI + empty-id denial); `PlayerQuestProgressPersistenceIntegrationTests` (`RequirePostgresFact`).
+- **Bruno:** `bruno/neon-sprawl-server/quest-progress/Health after quest state store load.bru` — health smoke only (pre-commit hook when `Program.cs` changes; no quest HTTP this story).
 - **Docs:** `server/README.md` quest progress store section; alignment register E7.M1 row updated.
 
 ## Technical approach

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

@@ -32,23 +32,23 @@ NEO-116 adds **`IPlayerQuestStateStore`** with thread-safe **`InMemoryPlayerQues
 
 ## 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.
+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`.
+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.
+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.
+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.
+- ~~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.
+- ~~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.
+- ~~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.
+- ~~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
 

server/NeonSprawl.Server.Tests/Game/Quests/InMemoryPlayerQuestStateStoreTests.cs

@@ -10,9 +10,9 @@ public sealed class InMemoryPlayerQuestStateStoreTests
 {
     private const string PlayerId = "dev-local-1";
     private const string UnknownPlayerId = "unknown-player-xyz";
-    private const string GatherQuestId = "prototype_quest_gather_intro";
+    private const string GatherQuestId = PrototypeE7M1QuestCatalogRules.GatherIntroQuestId;
     private const string ChainQuestId = PrototypeE7M1QuestCatalogRules.ChainQuestId;
-    private const string GatherObjectiveId = "gather_intro_obj_scrap";
+    private const string GatherObjectiveId = PrototypeE7M1QuestCatalogRules.GatherIntroFirstObjectiveId;
     private static readonly DateTimeOffset CompletedAt = new(2026, 6, 3, 12, 0, 0, TimeSpan.Zero);
 
     [Fact]
@@ -73,7 +73,7 @@ public sealed class InMemoryPlayerQuestStateStoreTests
         // Arrange
         var store = CreateStore();
         Assert.True(store.TryActivate(PlayerId, ChainQuestId, out _));
-        Assert.True(store.TryUpdateObjectiveCounter(PlayerId, ChainQuestId, "chain_obj_gather", 5, out _));
+        Assert.True(store.TryUpdateObjectiveCounter(PlayerId, ChainQuestId, PrototypeE7M1QuestCatalogRules.ChainFirstObjectiveId, 5, out _));
         // Act
         var advanced = store.TryAdvanceStep(PlayerId, ChainQuestId, 1, out var snapshot);
         // Assert
@@ -141,6 +141,25 @@ public sealed class InMemoryPlayerQuestStateStoreTests
         Assert.False(activated);
     }
 
+    [Theory]
+    [InlineData("", GatherQuestId)]
+    [InlineData("   ", GatherQuestId)]
+    [InlineData(PlayerId, "")]
+    [InlineData(PlayerId, "   ")]
+    public void TryGetProgressAndActivate_ShouldReturnFalse_WhenPlayerOrQuestIdEmpty(
+        string playerId,
+        string questId)
+    {
+        // Arrange
+        var store = CreateStore();
+        // Act
+        var getProgress = store.TryGetProgress(playerId, questId, out _);
+        var activate = store.TryActivate(playerId, questId, out _);
+        // Assert
+        Assert.False(getProgress);
+        Assert.False(activate);
+    }
+
     [Fact]
     public async Task Host_ShouldResolveStore_AndActivateGatherIntro()
     {

server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs

@@ -14,8 +14,6 @@ public sealed class PlayerQuestProgressPersistenceIntegrationTests(PostgresInteg
     private PostgresWebApplicationFactory Factory => harness.Factory;
 
     private const string PlayerId = "dev-local-1";
-    private const string GatherQuestId = "prototype_quest_gather_intro";
-    private const string GatherObjectiveId = "gather_intro_obj_scrap";
     private static readonly DateTimeOffset CompletedAt = new(2026, 6, 3, 14, 30, 0, TimeSpan.Zero);
 
     [RequirePostgresFact]
@@ -23,28 +21,44 @@ public sealed class PlayerQuestProgressPersistenceIntegrationTests(PostgresInteg
     {
         // Arrange
         await ResetQuestProgressTableAsync();
-        using (var scope = Factory.Services.CreateScope())
+
+        // Act — write through first host
+        using (var firstScope = Factory.Services.CreateScope())
         {
-            var store = scope.ServiceProvider.GetRequiredService<IPlayerQuestStateStore>();
+            var store = firstScope.ServiceProvider.GetRequiredService<IPlayerQuestStateStore>();
-            // Act — write on first host
+            Assert.True(store.TryActivate(PlayerId, PrototypeE7M1QuestCatalogRules.GatherIntroQuestId, out _));
-            Assert.True(store.TryActivate(PlayerId, GatherQuestId, out _));
+            Assert.True(store.TryUpdateObjectiveCounter(
-            Assert.True(store.TryUpdateObjectiveCounter(PlayerId, GatherQuestId, GatherObjectiveId, 3, out _));
+                PlayerId,
-            Assert.True(store.TryMarkComplete(PlayerId, GatherQuestId, CompletedAt, out _));
+                PrototypeE7M1QuestCatalogRules.GatherIntroQuestId,
+                PrototypeE7M1QuestCatalogRules.GatherIntroFirstObjectiveId,
+                3,
+                out _));
+            Assert.True(store.TryMarkComplete(
+                PlayerId,
+                PrototypeE7M1QuestCatalogRules.GatherIntroQuestId,
+                CompletedAt,
+                out _));
         }
 
+        // Act — read back through a fresh host
         QuestStepState readBack;
         await using (var secondFactory = new PostgresWebApplicationFactory())
         {
-            using var scope = secondFactory.Services.CreateScope();
+            using var secondScope = secondFactory.Services.CreateScope();
-            var store = scope.ServiceProvider.GetRequiredService<IPlayerQuestStateStore>();
+            var store = secondScope.ServiceProvider.GetRequiredService<IPlayerQuestStateStore>();
-            readBack = store.TryGetProgress(PlayerId, GatherQuestId, out var snapshot) ? snapshot : null!;
+            readBack = store.TryGetProgress(
+                PlayerId,
+                PrototypeE7M1QuestCatalogRules.GatherIntroQuestId,
+                out var snapshot)
+                ? snapshot
+                : null!;
         }
 
         // Assert
         Assert.NotNull(readBack);
         Assert.Equal(QuestProgressStatus.Completed, readBack.Status);
         Assert.Equal(0, readBack.CurrentStepIndex);
-        Assert.Equal(3, readBack.ObjectiveCounters[GatherObjectiveId]);
+        Assert.Equal(3, readBack.ObjectiveCounters[PrototypeE7M1QuestCatalogRules.GatherIntroFirstObjectiveId]);
         Assert.Equal(CompletedAt, readBack.CompletedAt);
     }
 

server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs

@@ -2,7 +2,7 @@ namespace NeonSprawl.Server.Game.Quests;
 
 /// <summary>
 /// Persisted per-player quest progress keyed by <c>(playerId, questId)</c> (NEO-116).
-/// Missing row ⇒ <c>not_started</c>. <see cref="QuestStateOperations"/> (NEO-117) and HTTP (E7M1-08) consume this interface.
+/// Missing row ⇒ <c>not_started</c>. <c>QuestStateOperations</c> (NEO-117) and HTTP (E7M1-08) consume this interface.
 /// </summary>
 public interface IPlayerQuestStateStore
 {

server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs

@@ -85,6 +85,7 @@ public sealed class PostgresPlayerQuestStateStore(Npgsql.NpgsqlDataSource dataSo
             if (reader.Read())
             {
                 snapshot = ReadSnapshot(reader, player, quest);
+                tx.Rollback();
                 return false;
             }
         }
@@ -304,7 +305,7 @@ public sealed class PostgresPlayerQuestStateStore(Npgsql.NpgsqlDataSource dataSo
     {
         var status = ParseStatus(reader.GetString(0));
         var stepIndex = reader.GetInt32(1);
-        var countersJson = reader.GetString(2);
+        var countersJson = reader.GetFieldValue<string>(2);
         var completedAt = reader.IsDBNull(3) ? (DateTimeOffset?)null : reader.GetFieldValue<DateTimeOffset>(3);
         var counters = DeserializeCounters(countersJson);
         return new QuestStepState(player, quest, status, stepIndex, counters, completedAt);

server/NeonSprawl.Server/Game/Quests/PrototypeE7M1QuestCatalogRules.cs

@@ -23,6 +23,15 @@ public static class PrototypeE7M1QuestCatalogRules
     /// <summary>Keep in sync with <c>scripts/validate_content.py</c> <c>PROTOTYPE_E7M1_CHAIN_QUEST_ID</c>.</summary>
     public const string ChainQuestId = "prototype_quest_operator_chain";
 
+    /// <summary>First frozen onboarding quest id (gather intro).</summary>
+    public const string GatherIntroQuestId = "prototype_quest_gather_intro";
+
+    /// <summary>First objective id on gather intro step (frozen catalog).</summary>
+    public const string GatherIntroFirstObjectiveId = "gather_intro_obj_scrap";
+
+    /// <summary>First objective id on operator chain step 0 (frozen catalog).</summary>
+    public const string ChainFirstObjectiveId = "chain_obj_gather";
+
     /// <summary>Keep in sync with <c>scripts/validate_content.py</c> <c>PROTOTYPE_E7M1_CHAIN_TERMINAL_ITEM_ID</c>.</summary>
     public const string ChainTerminalItemId = "contract_handoff_token";
 

server/README.md

@@ -170,7 +170,7 @@ Per-player quest runtime state lives in **`IPlayerQuestStateStore`**, keyed by *
 
 Completed rows cannot regress to active without a reset API (none in prototype). Player ids are normalized (trim + lowercase). Quest ids should be validated via **`IQuestDefinitionRegistry.TryNormalizeKnown`** at operation/HTTP layers (NEO-117+).
 
-**Storage:** in-memory singleton when **`ConnectionStrings:NeonSprawl`** is unset (seeds configured dev player only). When Postgres is configured, **`PostgresPlayerQuestStateStore`** persists to **`player_quest_progress`** ([`V008__player_quest_progress.sql`](../db/migrations/V008__player_quest_progress.sql)) with **`objective_counters`** JSONB for the current step. Plan: [NEO-116 implementation plan](../../docs/plans/NEO-116-implementation-plan.md).
+**Storage:** in-memory singleton when **`ConnectionStrings:NeonSprawl`** is unset (seeds configured dev player only). When Postgres is configured, **`PostgresPlayerQuestStateStore`** persists to **`player_quest_progress`** ([`V008__player_quest_progress.sql`](../db/migrations/V008__player_quest_progress.sql)) with **`objective_counters`** JSONB for the current step. Plan: [NEO-116 implementation plan](../../docs/plans/NEO-116-implementation-plan.md). Bruno startup smoke: `bruno/neon-sprawl-server/quest-progress/` (health only until E7M1-08 HTTP).
 
 ## Encounter definitions (NEO-103)