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.pull/156/head
parent
025c3250d2
commit
540257d12d
|
|
@ -30,6 +30,12 @@ NEO-117 adds **`QuestStateOperations`** — static accept, step advance, and com
|
||||||
|
|
||||||
(none)
|
(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
|
## 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`.
|
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`.
|
||||||
|
|
|
||||||
|
|
@ -1,4 +1,5 @@
|
||||||
using Microsoft.Extensions.DependencyInjection;
|
using Microsoft.Extensions.DependencyInjection;
|
||||||
|
using NeonSprawl.Server.Game.PositionState;
|
||||||
using NeonSprawl.Server.Game.Quests;
|
using NeonSprawl.Server.Game.Quests;
|
||||||
using NeonSprawl.Server.Tests;
|
using NeonSprawl.Server.Tests;
|
||||||
using Xunit;
|
using Xunit;
|
||||||
|
|
@ -154,6 +155,25 @@ public sealed class QuestStateOperationsTests
|
||||||
Assert.Equal(QuestProgressStatus.Completed, result.Snapshot!.Status);
|
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]
|
[Fact]
|
||||||
public async Task TryAccept_ShouldDenyUnknownPlayer_WhenPlayerNotInStore()
|
public async Task TryAccept_ShouldDenyUnknownPlayer_WhenPlayerNotInStore()
|
||||||
{
|
{
|
||||||
|
|
@ -392,6 +412,30 @@ public sealed class QuestStateOperationsTests
|
||||||
Assert.Equal(QuestProgressStatus.Completed, complete.Snapshot!.Status);
|
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(
|
private static void AcceptAndComplete(
|
||||||
(IQuestDefinitionRegistry QuestRegistry, IPlayerQuestStateStore ProgressStore) deps,
|
(IQuestDefinitionRegistry QuestRegistry, IPlayerQuestStateStore ProgressStore) deps,
|
||||||
string questId,
|
string questId,
|
||||||
|
|
|
||||||
|
|
@ -6,6 +6,9 @@ namespace NeonSprawl.Server.Game.Quests;
|
||||||
/// </summary>
|
/// </summary>
|
||||||
public interface IPlayerQuestStateStore
|
public interface IPlayerQuestStateStore
|
||||||
{
|
{
|
||||||
|
/// <summary>True when mutations for <paramref name="playerId"/> are allowed (dev bucket or Postgres <c>player_position</c> row).</summary>
|
||||||
|
bool CanWritePlayer(string playerId);
|
||||||
|
|
||||||
/// <summary>Missing row ⇒ <c>false</c> (callers treat as <c>not_started</c>).</summary>
|
/// <summary>Missing row ⇒ <c>false</c> (callers treat as <c>not_started</c>).</summary>
|
||||||
bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot);
|
bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -66,6 +66,13 @@ public sealed class InMemoryPlayerQuestStateStore(IOptions<GamePositionOptions>
|
||||||
return new HashSet<string>(StringComparer.OrdinalIgnoreCase) { id };
|
return new HashSet<string>(StringComparer.OrdinalIgnoreCase) { id };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <inheritdoc />
|
||||||
|
public bool CanWritePlayer(string playerId)
|
||||||
|
{
|
||||||
|
var player = QuestProgressIds.NormalizePlayerId(playerId);
|
||||||
|
return player.Length > 0 && knownPlayers.Contains(player);
|
||||||
|
}
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <inheritdoc />
|
||||||
public bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot)
|
public bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot)
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -10,6 +10,20 @@ public sealed class PostgresPlayerQuestStateStore(Npgsql.NpgsqlDataSource dataSo
|
||||||
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
|
PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
|
||||||
};
|
};
|
||||||
|
|
||||||
|
/// <inheritdoc />
|
||||||
|
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);
|
||||||
|
}
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <inheritdoc />
|
||||||
public bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot)
|
public bool TryGetProgress(string playerId, string questId, out QuestStepState snapshot)
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -21,6 +21,11 @@ public static class QuestStateOperations
|
||||||
return Deny(QuestStateReasonCodes.UnknownQuest);
|
return Deny(QuestStateReasonCodes.UnknownQuest);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (!progressStore.CanWritePlayer(playerId))
|
||||||
|
{
|
||||||
|
return Deny(QuestStateReasonCodes.UnknownPlayer);
|
||||||
|
}
|
||||||
|
|
||||||
if (progressStore.TryGetProgress(playerId, normalizedQuestId, out var existing))
|
if (progressStore.TryGetProgress(playerId, normalizedQuestId, out var existing))
|
||||||
{
|
{
|
||||||
if (existing.Status == QuestProgressStatus.Completed)
|
if (existing.Status == QuestProgressStatus.Completed)
|
||||||
|
|
@ -45,7 +50,7 @@ public static class QuestStateOperations
|
||||||
return Success(snapshot);
|
return Success(snapshot);
|
||||||
}
|
}
|
||||||
|
|
||||||
return Deny(QuestStateReasonCodes.UnknownPlayer);
|
return DenyActivateFailure(progressStore, playerId, normalizedQuestId, snapshot);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
|
|
@ -89,7 +94,7 @@ public static class QuestStateOperations
|
||||||
return Success(snapshot);
|
return Success(snapshot);
|
||||||
}
|
}
|
||||||
|
|
||||||
return Deny(QuestStateReasonCodes.UnknownPlayer, existing);
|
return DenyAdvanceFailure(progressStore, playerId, normalizedQuestId, snapshot, existing);
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
|
|
@ -128,13 +133,69 @@ public static class QuestStateOperations
|
||||||
return Success(snapshot);
|
return Success(snapshot);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (snapshot?.Status == QuestProgressStatus.Completed)
|
||||||
|
{
|
||||||
|
return Success(snapshot);
|
||||||
|
}
|
||||||
|
|
||||||
if (progressStore.TryGetProgress(playerId, normalizedQuestId, out var afterAttempt) &&
|
if (progressStore.TryGetProgress(playerId, normalizedQuestId, out var afterAttempt) &&
|
||||||
afterAttempt.Status == QuestProgressStatus.Completed)
|
afterAttempt.Status == QuestProgressStatus.Completed)
|
||||||
{
|
{
|
||||||
return Success(afterAttempt);
|
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) =>
|
private static QuestStateOperationResult Success(QuestStepState snapshot) =>
|
||||||
|
|
|
||||||
|
|
@ -162,6 +162,7 @@ Per-player quest runtime state lives in **`IPlayerQuestStateStore`**, keyed by *
|
||||||
|
|
||||||
**Store interface methods:**
|
**Store interface methods:**
|
||||||
|
|
||||||
|
- **`CanWritePlayer`** — whether mutations are allowed for the player (dev bucket / Postgres `player_position`).
|
||||||
- **`TryGetProgress`** — read one row; false when not started.
|
- **`TryGetProgress`** — read one row; false when not started.
|
||||||
- **`TryActivate`** — first accept (`not_started` → `active`).
|
- **`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).
|
- **`TryAdvanceStep`** — bump step index and clear counters for the new step (denies when completed or index does not increase).
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue