From de611ac2943d61a1b2c8457168e5acc959502f07 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Wed, 3 Jun 2026 23:19:52 -0400 Subject: [PATCH] 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 | 3 +- docs/reviews/2026-06-03-NEO-116.md | 16 ++++---- .../InMemoryPlayerQuestStateStoreTests.cs | 25 ++++++++++-- ...uestProgressPersistenceIntegrationTests.cs | 38 +++++++++++++------ .../Game/Quests/IPlayerQuestStateStore.cs | 2 +- .../Quests/PostgresPlayerQuestStateStore.cs | 3 +- .../Quests/PrototypeE7M1QuestCatalogRules.cs | 9 +++++ server/README.md | 2 +- 8 files changed, 71 insertions(+), 27 deletions(-) diff --git a/docs/plans/NEO-116-implementation-plan.md b/docs/plans/NEO-116-implementation-plan.md index 105a0bf..b992196 100644 --- a/docs/plans/NEO-116-implementation-plan.md +++ b/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 diff --git a/docs/reviews/2026-06-03-NEO-116.md b/docs/reviews/2026-06-03-NEO-116.md index 4bf5890..36e89a8 100644 --- a/docs/reviews/2026-06-03-NEO-116.md +++ b/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 `` references a type not yet in the repo (NEO-117). Harmless for compile; consider `QuestStateOperations` until NEO-117 lands if doc warnings appear. +- ~~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.~~ **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`** 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`** or typed JSON read — same pattern worth watching in future JSONB stores.~~ **Done.** Uses **`GetFieldValue(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 diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/InMemoryPlayerQuestStateStoreTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/InMemoryPlayerQuestStateStoreTests.cs index 12d0645..a741451 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/InMemoryPlayerQuestStateStoreTests.cs +++ b/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() { diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs index 8b21b7d..d405783 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/PlayerQuestProgressPersistenceIntegrationTests.cs +++ b/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(); - // Act — write on first host - Assert.True(store.TryActivate(PlayerId, GatherQuestId, out _)); - Assert.True(store.TryUpdateObjectiveCounter(PlayerId, GatherQuestId, GatherObjectiveId, 3, out _)); - Assert.True(store.TryMarkComplete(PlayerId, GatherQuestId, CompletedAt, out _)); + var store = firstScope.ServiceProvider.GetRequiredService(); + Assert.True(store.TryActivate(PlayerId, PrototypeE7M1QuestCatalogRules.GatherIntroQuestId, out _)); + Assert.True(store.TryUpdateObjectiveCounter( + PlayerId, + 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(); - var store = scope.ServiceProvider.GetRequiredService(); - readBack = store.TryGetProgress(PlayerId, GatherQuestId, out var snapshot) ? snapshot : null!; + using var secondScope = secondFactory.Services.CreateScope(); + var store = secondScope.ServiceProvider.GetRequiredService(); + 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); } diff --git a/server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs b/server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs index b1276f6..53342d4 100644 --- a/server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs +++ b/server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs @@ -2,7 +2,7 @@ namespace NeonSprawl.Server.Game.Quests; /// /// Persisted per-player quest progress keyed by (playerId, questId) (NEO-116). -/// Missing row ⇒ not_started. (NEO-117) and HTTP (E7M1-08) consume this interface. +/// Missing row ⇒ not_started. QuestStateOperations (NEO-117) and HTTP (E7M1-08) consume this interface. /// public interface IPlayerQuestStateStore { diff --git a/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs b/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs index 22e0ef7..135c828 100644 --- a/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs +++ b/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(2); var completedAt = reader.IsDBNull(3) ? (DateTimeOffset?)null : reader.GetFieldValue(3); var counters = DeserializeCounters(countersJson); return new QuestStepState(player, quest, status, stepIndex, counters, completedAt); diff --git a/server/NeonSprawl.Server/Game/Quests/PrototypeE7M1QuestCatalogRules.cs b/server/NeonSprawl.Server/Game/Quests/PrototypeE7M1QuestCatalogRules.cs index d76ac3c..1de299f 100644 --- a/server/NeonSprawl.Server/Game/Quests/PrototypeE7M1QuestCatalogRules.cs +++ b/server/NeonSprawl.Server/Game/Quests/PrototypeE7M1QuestCatalogRules.cs @@ -23,6 +23,15 @@ public static class PrototypeE7M1QuestCatalogRules /// Keep in sync with scripts/validate_content.py PROTOTYPE_E7M1_CHAIN_QUEST_ID. public const string ChainQuestId = "prototype_quest_operator_chain"; + /// First frozen onboarding quest id (gather intro). + public const string GatherIntroQuestId = "prototype_quest_gather_intro"; + + /// First objective id on gather intro step (frozen catalog). + public const string GatherIntroFirstObjectiveId = "gather_intro_obj_scrap"; + + /// First objective id on operator chain step 0 (frozen catalog). + public const string ChainFirstObjectiveId = "chain_obj_gather"; + /// Keep in sync with scripts/validate_content.py PROTOTYPE_E7M1_CHAIN_TERMINAL_ITEM_ID. public const string ChainTerminalItemId = "contract_handoff_token"; diff --git a/server/README.md b/server/README.md index 222d8bb..a4e4b07 100644 --- a/server/README.md +++ b/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)