From 540257d12d97c331a7dc5bb9fdd787cffb285133 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 6 Jun 2026 19:36:41 -0400 Subject: [PATCH] NEO-117: Fix Bugbot deny-code misclassification on quest accept. Add CanWritePlayer to IPlayerQuestStateStore and check before prerequisite gate; map TryActivate/TryAdvanceStep store failures from progress snapshots instead of always unknown_player. --- docs/reviews/2026-06-06-NEO-117.md | 6 ++ .../Game/Quests/QuestStateOperationsTests.cs | 44 ++++++++++++ .../Game/Quests/IPlayerQuestStateStore.cs | 3 + .../Quests/InMemoryPlayerQuestStateStore.cs | 7 ++ .../Quests/PostgresPlayerQuestStateStore.cs | 14 ++++ .../Game/Quests/QuestStateOperations.cs | 67 ++++++++++++++++++- server/README.md | 1 + 7 files changed, 139 insertions(+), 3 deletions(-) diff --git a/docs/reviews/2026-06-06-NEO-117.md b/docs/reviews/2026-06-06-NEO-117.md index 04d6c6a..62d6fe9 100644 --- a/docs/reviews/2026-06-06-NEO-117.md +++ b/docs/reviews/2026-06-06-NEO-117.md @@ -30,6 +30,12 @@ NEO-117 adds **`QuestStateOperations`** — static accept, step advance, and com (none) +## Bugbot follow-up (2026-06-06) + +1. ~~**TryActivate failure misclassified** — Map store mutation failures from returned snapshots / re-read progress instead of always `unknown_player`.~~ **Done.** `DenyActivateFailure` / `DenyAdvanceFailure` helpers; store replay on `TryMarkComplete` returns idempotent success. + +2. ~~**Unknown player wrong deny code** — Check `CanWritePlayer` before prerequisite gate so unknown players get `unknown_player`, not `prerequisite_incomplete`.~~ **Done.** Added `IPlayerQuestStateStore.CanWritePlayer` (in-memory + Postgres); `TryAccept` calls it before prerequisites. + ## Suggestions 1. ~~**Test `ResolveDependencies` pattern** — `QuestStateOperationsTests.ResolveDependencies` creates an `IServiceScope` that is never disposed. **`EncounterCompletionOperationsTests`** resolves from `factory.Services` root (singleton stores). Align to avoid undisposed scopes and match the established encounter test pattern.~~ **Done.** `ResolveDependencies` and host smoke test resolve from `factory.Services`. diff --git a/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs index 2ee2d1e..d4e66a9 100644 --- a/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Quests/QuestStateOperationsTests.cs @@ -1,4 +1,5 @@ using Microsoft.Extensions.DependencyInjection; +using NeonSprawl.Server.Game.PositionState; using NeonSprawl.Server.Game.Quests; using NeonSprawl.Server.Tests; using Xunit; @@ -154,6 +155,25 @@ public sealed class QuestStateOperationsTests Assert.Equal(QuestProgressStatus.Completed, result.Snapshot!.Status); } + [Fact] + public async Task TryAccept_ShouldDenyUnknownPlayer_WhenPrerequisiteQuestNotWritable() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + + // Act + var result = QuestStateOperations.TryAccept( + UnknownPlayerId, + RefineQuestId, + deps.QuestRegistry, + deps.ProgressStore); + + // Assert + Assert.False(result.Success); + Assert.Equal(QuestStateReasonCodes.UnknownPlayer, result.ReasonCode); + } + [Fact] public async Task TryAccept_ShouldDenyUnknownPlayer_WhenPlayerNotInStore() { @@ -392,6 +412,30 @@ public sealed class QuestStateOperationsTests Assert.Equal(QuestProgressStatus.Completed, complete.Snapshot!.Status); } + [Fact] + public void CanWritePlayer_ShouldReturnFalse_ForUnknownPlayer() + { + // Arrange + var store = new InMemoryPlayerQuestStateStore( + Microsoft.Extensions.Options.Options.Create(new GamePositionOptions { DevPlayerId = PlayerId })); + // Act + var canWrite = store.CanWritePlayer(UnknownPlayerId); + // Assert + Assert.False(canWrite); + } + + [Fact] + public void CanWritePlayer_ShouldReturnTrue_ForDevPlayer() + { + // Arrange + var store = new InMemoryPlayerQuestStateStore( + Microsoft.Extensions.Options.Options.Create(new GamePositionOptions { DevPlayerId = PlayerId })); + // Act + var canWrite = store.CanWritePlayer(PlayerId); + // Assert + Assert.True(canWrite); + } + private static void AcceptAndComplete( (IQuestDefinitionRegistry QuestRegistry, IPlayerQuestStateStore ProgressStore) deps, string questId, diff --git a/server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs b/server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs index 53342d4..1b48cd7 100644 --- a/server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs +++ b/server/NeonSprawl.Server/Game/Quests/IPlayerQuestStateStore.cs @@ -6,6 +6,9 @@ namespace NeonSprawl.Server.Game.Quests; /// public interface IPlayerQuestStateStore { + /// True when mutations for are allowed (dev bucket or Postgres player_position row). + bool CanWritePlayer(string playerId); + /// Missing row ⇒ false (callers treat as not_started). bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot); diff --git a/server/NeonSprawl.Server/Game/Quests/InMemoryPlayerQuestStateStore.cs b/server/NeonSprawl.Server/Game/Quests/InMemoryPlayerQuestStateStore.cs index c32e320..487b2e8 100644 --- a/server/NeonSprawl.Server/Game/Quests/InMemoryPlayerQuestStateStore.cs +++ b/server/NeonSprawl.Server/Game/Quests/InMemoryPlayerQuestStateStore.cs @@ -66,6 +66,13 @@ public sealed class InMemoryPlayerQuestStateStore(IOptions return new HashSet(StringComparer.OrdinalIgnoreCase) { id }; } + /// + public bool CanWritePlayer(string playerId) + { + var player = QuestProgressIds.NormalizePlayerId(playerId); + return player.Length > 0 && knownPlayers.Contains(player); + } + /// public bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot) { diff --git a/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs b/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs index 2a60b37..b8f4631 100644 --- a/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs +++ b/server/NeonSprawl.Server/Game/Quests/PostgresPlayerQuestStateStore.cs @@ -10,6 +10,20 @@ public sealed class PostgresPlayerQuestStateStore(Npgsql.NpgsqlDataSource dataSo PropertyNamingPolicy = JsonNamingPolicy.CamelCase, }; + /// + public bool CanWritePlayer(string playerId) + { + var player = QuestProgressIds.NormalizePlayerId(playerId); + if (player.Length == 0) + { + return false; + } + + PostgresPlayerQuestProgressBootstrap.EnsureSchema(dataSource); + using var conn = dataSource.OpenConnection(); + return PlayerExists(conn, player); + } + /// public bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot) { diff --git a/server/NeonSprawl.Server/Game/Quests/QuestStateOperations.cs b/server/NeonSprawl.Server/Game/Quests/QuestStateOperations.cs index 1ca6717..4fd4bf3 100644 --- a/server/NeonSprawl.Server/Game/Quests/QuestStateOperations.cs +++ b/server/NeonSprawl.Server/Game/Quests/QuestStateOperations.cs @@ -21,6 +21,11 @@ public static class QuestStateOperations return Deny(QuestStateReasonCodes.UnknownQuest); } + if (!progressStore.CanWritePlayer(playerId)) + { + return Deny(QuestStateReasonCodes.UnknownPlayer); + } + if (progressStore.TryGetProgress(playerId, normalizedQuestId, out var existing)) { if (existing.Status == QuestProgressStatus.Completed) @@ -45,7 +50,7 @@ public static class QuestStateOperations return Success(snapshot); } - return Deny(QuestStateReasonCodes.UnknownPlayer); + return DenyActivateFailure(progressStore, playerId, normalizedQuestId, snapshot); } /// @@ -89,7 +94,7 @@ public static class QuestStateOperations return Success(snapshot); } - return Deny(QuestStateReasonCodes.UnknownPlayer, existing); + return DenyAdvanceFailure(progressStore, playerId, normalizedQuestId, snapshot, existing); } /// @@ -128,13 +133,69 @@ public static class QuestStateOperations return Success(snapshot); } + if (snapshot?.Status == QuestProgressStatus.Completed) + { + return Success(snapshot); + } + if (progressStore.TryGetProgress(playerId, normalizedQuestId, out var afterAttempt) && afterAttempt.Status == QuestProgressStatus.Completed) { return Success(afterAttempt); } - return Deny(QuestStateReasonCodes.UnknownPlayer); + if (!progressStore.CanWritePlayer(playerId)) + { + return Deny(QuestStateReasonCodes.UnknownPlayer); + } + + return DenyFromProgressSnapshot(snapshot, QuestStateReasonCodes.NotActive) + ?? Deny(QuestStateReasonCodes.UnknownPlayer, progress); + } + + private static QuestStateOperationResult DenyActivateFailure( + IPlayerQuestStateStore progressStore, + string playerId, + string questId, + QuestStepState attemptSnapshot) => + DenyFromProgressSnapshot(attemptSnapshot, QuestStateReasonCodes.AlreadyActive) + ?? (progressStore.TryGetProgress(playerId, questId, out var reread) + ? DenyFromProgressSnapshot(reread, QuestStateReasonCodes.AlreadyActive) + : null) + ?? Deny(QuestStateReasonCodes.UnknownPlayer); + + private static QuestStateOperationResult DenyAdvanceFailure( + IPlayerQuestStateStore progressStore, + string playerId, + string questId, + QuestStepState attemptSnapshot, + QuestStepState priorRead) => + DenyFromProgressSnapshot(attemptSnapshot, QuestStateReasonCodes.InvalidStepIndex) + ?? (progressStore.TryGetProgress(playerId, questId, out var reread) + ? DenyFromProgressSnapshot(reread, QuestStateReasonCodes.InvalidStepIndex) + : null) + ?? Deny(QuestStateReasonCodes.UnknownPlayer, priorRead); + + private static QuestStateOperationResult? DenyFromProgressSnapshot( + QuestStepState? snapshot, + string activeOrIntermediateReasonCode) + { + if (snapshot is null) + { + return null; + } + + if (snapshot.Status == QuestProgressStatus.Completed) + { + return Deny(QuestStateReasonCodes.AlreadyCompleted, snapshot); + } + + if (snapshot.Status == QuestProgressStatus.Active) + { + return Deny(activeOrIntermediateReasonCode, snapshot); + } + + return null; } private static QuestStateOperationResult Success(QuestStepState snapshot) => diff --git a/server/README.md b/server/README.md index c45c63c..b658051 100644 --- a/server/README.md +++ b/server/README.md @@ -162,6 +162,7 @@ Per-player quest runtime state lives in **`IPlayerQuestStateStore`**, keyed by * **Store interface methods:** +- **`CanWritePlayer`** — whether mutations are allowed for the player (dev bucket / Postgres `player_position`). - **`TryGetProgress`** — read one row; false when not started. - **`TryActivate`** — first accept (`not_started` → `active`). - **`TryAdvanceStep`** — bump step index and clear counters for the new step (denies when completed or index does not increase).