From 91f74b7d6eb0e64edd463bef2878f6ce07284117 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 31 May 2026 16:37:22 -0400 Subject: [PATCH] NEO-104: Address code review findings for encounter stores. Normalize encounter ids in IsAllRequiredTargetsDefeated, document TryActivate completion gating, lock IsCompleted reads, add operations tests, and update plan and review doc. --- docs/plans/NEO-104-implementation-plan.md | 1 + docs/reviews/2026-05-31-NEO-104.md | 12 ++-- .../EncounterDefinitionRegistryTests.cs | 4 +- .../EncounterProgressOperationsTests.cs | 62 +++++++++++++++++++ .../Encounters/EncounterProgressOperations.cs | 5 +- .../Encounters/IEncounterProgressStore.cs | 2 + .../InMemoryEncounterCompletionStore.cs | 10 ++- 7 files changed, 84 insertions(+), 12 deletions(-) diff --git a/docs/plans/NEO-104-implementation-plan.md b/docs/plans/NEO-104-implementation-plan.md index 2759bf1..f50c538 100644 --- a/docs/plans/NEO-104-implementation-plan.md +++ b/docs/plans/NEO-104-implementation-plan.md @@ -62,6 +62,7 @@ - **DI:** registered in `AddEncounterAndRewardCatalogs`. - **Tests:** `InMemoryEncounterProgressStoreTests`, `InMemoryEncounterCompletionStoreTests`, `EncounterProgressOperationsTests`; host DI resolve in `EncounterDefinitionRegistryTests`. - **Docs:** `server/README.md`; E5.M3 module snapshot + alignment register + backlog updated. +- **Plan file list:** `Program.cs` and `InMemoryWebApplicationFactory.cs` unchanged — stores register in existing `AddEncounterAndRewardCatalogs` (already invoked at startup); always in-memory (no Postgres test-factory swap). ## Technical approach diff --git a/docs/reviews/2026-05-31-NEO-104.md b/docs/reviews/2026-05-31-NEO-104.md index 9cac928..02dcbf4 100644 --- a/docs/reviews/2026-05-31-NEO-104.md +++ b/docs/reviews/2026-05-31-NEO-104.md @@ -30,19 +30,19 @@ None. ## Suggestions -1. **Plan file list vs shipped diff** — The plan lists **`InMemoryWebApplicationFactory.cs`** and **`Program.cs`** as files to modify; neither changed. This is acceptable: stores register in the existing **`AddEncounterAndRewardCatalogs`** extension (already invoked at startup), and encounter stores are always in-memory (no Postgres swap needed in the test factory, unlike position/inventory stores). Consider a one-line note in the plan **Implementation reconciliation** that factory/Program edits were N/A. +1. ~~**Plan file list vs shipped diff** — The plan lists **`InMemoryWebApplicationFactory.cs`** and **`Program.cs`** as files to modify; neither changed. This is acceptable: stores register in the existing **`AddEncounterAndRewardCatalogs`** extension (already invoked at startup), and encounter stores are always in-memory (no Postgres swap needed in the test factory, unlike position/inventory stores). Consider a one-line note in the plan **Implementation reconciliation** that factory/Program edits were N/A.~~ **Done.** Note added to plan **Implementation reconciliation**. -2. **Completion guard on `TryActivate`** — The plan describes **`TryActivate`** returning `false` when the completion store marks the encounter done. Gating lives only in **`EncounterProgressOperations.TryActivateOnFirstEngagement`**; a direct **`IEncounterProgressStore.TryActivate`** call after completion would still create a started row. Acceptable while NEO-106/105 route exclusively through operations; add a brief XML note on **`IEncounterProgressStore.TryActivate`** that completion gating is the caller's responsibility if direct store use is anticipated. +2. ~~**Completion guard on `TryActivate`** — The plan describes **`TryActivate`** returning `false` when the completion store marks the encounter done. Gating lives only in **`EncounterProgressOperations.TryActivateOnFirstEngagement`**; a direct **`IEncounterProgressStore.TryActivate`** call after completion would still create a started row. Acceptable while NEO-106/105 route exclusively through operations; add a brief XML note on **`IEncounterProgressStore.TryActivate`** that completion gating is the caller's responsibility if direct store use is anticipated.~~ **Done.** XML on **`IEncounterProgressStore.TryActivate`**. -3. **Optional operations coverage** — Store and core happy-path operations are well covered. Consider adding AAA tests for: **`TryActivateOnFirstEngagement`** returns `false` when encounter is already completed; resolve/activate returns `false` for an NPC id not in any encounter definition. +3. ~~**Optional operations coverage** — Store and core happy-path operations are well covered. Consider adding AAA tests for: **`TryActivateOnFirstEngagement`** returns `false` when encounter is already completed; resolve/activate returns `false` for an NPC id not in any encounter definition.~~ **Done.** Two new tests in **`EncounterProgressOperationsTests`** plus mixed-case **`IsAllRequiredTargetsDefeated`** coverage. -4. **`IsAllRequiredTargetsDefeated` id normalization** — The method passes **`encounterId`** to case-sensitive **`TryGetDefinition`**. NEO-102 precedent documents that only **`TryNormalizeKnown`** lowercases. Internal callers today use catalog **`row.Id`** (lowercase); NEO-108 HTTP should normalize before calling — optionally harden now with **`TryNormalizeKnown`** or **`EncounterProgressIds.NormalizeEncounterId`** + lookup for defense in depth. +4. ~~**`IsAllRequiredTargetsDefeated` id normalization** — The method passes **`encounterId`** to case-sensitive **`TryGetDefinition`**. NEO-102 precedent documents that only **`TryNormalizeKnown`** lowercases. Internal callers today use catalog **`row.Id`** (lowercase); NEO-108 HTTP should normalize before calling — optionally harden now with **`TryNormalizeKnown`** or **`EncounterProgressIds.NormalizeEncounterId`** + lookup for defense in depth.~~ **Done.** **`TryNormalizeKnown`** before lookup and progress read. ## Nits -- Nit: **`Host_ShouldResolveEncounterProgressStoresFromDi_WhenStartupSucceeds`** — **`Assert.NotNull`** on values from **`GetRequiredService`** is redundant (missing registration throws). Minor AAA nit: DI resolution fits **Arrange**; **Act** could be only **`TryActivate`** / **`TryMarkCompleted`**. +- ~~Nit: **`Host_ShouldResolveEncounterProgressStoresFromDi_WhenStartupSucceeds`** — **`Assert.NotNull`** on values from **`GetRequiredService`** is redundant (missing registration throws). Minor AAA nit: DI resolution fits **Arrange**; **Act** could be only **`TryActivate`** / **`TryMarkCompleted`**.~~ **Done.** -- Nit: **`InMemoryEncounterCompletionStore.IsCompleted`** reads **`ConcurrentDictionary`** without the per-key lock used by **`TryMarkCompleted`** / **`TryGetCompletedAt`**. Benign for prototype bool checks; align with lock if strict read-your-writes under concurrency matters later. +- ~~Nit: **`InMemoryEncounterCompletionStore.IsCompleted`** reads **`ConcurrentDictionary`** without the per-key lock used by **`TryMarkCompleted`** / **`TryGetCompletedAt`**. Benign for prototype bool checks; align with lock if strict read-your-writes under concurrency matters later.~~ **Done.** **`IsCompleted`** uses per-key lock. ## Verification diff --git a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterDefinitionRegistryTests.cs b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterDefinitionRegistryTests.cs index 23a09a3..9af8db9 100644 --- a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterDefinitionRegistryTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterDefinitionRegistryTests.cs @@ -210,17 +210,15 @@ public class EncounterDefinitionRegistryTests await using var factory = new InMemoryWebApplicationFactory(); using var client = factory.CreateClient(); _ = await client.GetAsync("/health"); - // Act var progressStore = factory.Services.GetRequiredService(); var completionStore = factory.Services.GetRequiredService(); + // Act var activated = progressStore.TryActivate("dev-local-1", PrototypeCombatPocket); var completed = completionStore.TryMarkCompleted( "dev-local-1", PrototypeCombatPocket, new DateTimeOffset(2026, 5, 31, 12, 0, 0, TimeSpan.Zero)); // Assert - Assert.NotNull(progressStore); - Assert.NotNull(completionStore); Assert.True(activated); Assert.True(completed); Assert.True(completionStore.IsCompleted("dev-local-1", PrototypeCombatPocket)); diff --git a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressOperationsTests.cs index cd30419..28f535c 100644 --- a/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Encounters/EncounterProgressOperationsTests.cs @@ -118,6 +118,68 @@ public sealed class EncounterProgressOperationsTests Assert.False(marked); } + [Fact] + public async Task TryActivateOnFirstEngagement_ShouldReturnFalse_WhenEncounterAlreadyCompleted() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var registry = factory.Services.GetRequiredService(); + var progress = factory.Services.GetRequiredService(); + var completion = factory.Services.GetRequiredService(); + _ = completion.TryMarkCompleted(PlayerId, EncounterId, DateTimeOffset.UtcNow); + // Act + var activated = EncounterProgressOperations.TryActivateOnFirstEngagement( + PlayerId, + MeleeNpc, + registry, + progress, + completion); + // Assert + Assert.False(activated); + Assert.False(progress.TryGetProgress(PlayerId, EncounterId, out _)); + } + + [Fact] + public async Task TryActivateOnFirstEngagement_ShouldReturnFalse_WhenNpcNotInAnyEncounter() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var registry = factory.Services.GetRequiredService(); + var progress = factory.Services.GetRequiredService(); + var completion = factory.Services.GetRequiredService(); + // Act + var activated = EncounterProgressOperations.TryActivateOnFirstEngagement( + PlayerId, + "prototype_npc_unknown", + registry, + progress, + completion); + // Assert + Assert.False(activated); + } + + [Fact] + public async Task IsAllRequiredTargetsDefeated_ShouldReturnTrue_WhenEncounterIdHasMixedCase() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var registry = factory.Services.GetRequiredService(); + var progress = factory.Services.GetRequiredService(); + var completion = factory.Services.GetRequiredService(); + _ = EncounterProgressOperations.TryActivateOnFirstEngagement(PlayerId, MeleeNpc, registry, progress, completion); + _ = EncounterProgressOperations.TryMarkTargetDefeated(PlayerId, MeleeNpc, registry, progress, completion); + _ = EncounterProgressOperations.TryMarkTargetDefeated(PlayerId, RangedNpc, registry, progress, completion); + _ = EncounterProgressOperations.TryMarkTargetDefeated(PlayerId, EliteNpc, registry, progress, completion); + // Act + var allDefeated = EncounterProgressOperations.IsAllRequiredTargetsDefeated( + PlayerId, + " Prototype_Combat_Pocket ", + registry, + progress); + // Assert + Assert.True(allDefeated); + } + [Fact] public async Task TryMarkCompleted_ShouldReturnFalseOnSecondAttempt_WhenUsingCompletionStoreDirectly() { diff --git a/server/NeonSprawl.Server/Game/Encounters/EncounterProgressOperations.cs b/server/NeonSprawl.Server/Game/Encounters/EncounterProgressOperations.cs index 291ae26..9aba447 100644 --- a/server/NeonSprawl.Server/Game/Encounters/EncounterProgressOperations.cs +++ b/server/NeonSprawl.Server/Game/Encounters/EncounterProgressOperations.cs @@ -116,12 +116,13 @@ public static class EncounterProgressOperations IEncounterDefinitionRegistry encounterRegistry, IEncounterProgressStore progressStore) { - if (!encounterRegistry.TryGetDefinition(encounterId, out var definition)) + if (!encounterRegistry.TryNormalizeKnown(encounterId, out var normalizedEncounterId) || + !encounterRegistry.TryGetDefinition(normalizedEncounterId, out var definition)) { return false; } - if (!progressStore.TryGetProgress(playerId, encounterId, out var progress) || !progress.Started) + if (!progressStore.TryGetProgress(playerId, normalizedEncounterId, out var progress) || !progress.Started) { return false; } diff --git a/server/NeonSprawl.Server/Game/Encounters/IEncounterProgressStore.cs b/server/NeonSprawl.Server/Game/Encounters/IEncounterProgressStore.cs index fc15de9..1d1ebc3 100644 --- a/server/NeonSprawl.Server/Game/Encounters/IEncounterProgressStore.cs +++ b/server/NeonSprawl.Server/Game/Encounters/IEncounterProgressStore.cs @@ -9,6 +9,8 @@ public interface IEncounterProgressStore /// /// Creates a started row with an empty defeated set. /// Returns true on first activation; false when already started. + /// Does not consult — callers that must block activation after completion + /// (e.g. ) must gate before calling this method. /// bool TryActivate(string playerId, string encounterId); diff --git a/server/NeonSprawl.Server/Game/Encounters/InMemoryEncounterCompletionStore.cs b/server/NeonSprawl.Server/Game/Encounters/InMemoryEncounterCompletionStore.cs index ee124da..656f674 100644 --- a/server/NeonSprawl.Server/Game/Encounters/InMemoryEncounterCompletionStore.cs +++ b/server/NeonSprawl.Server/Game/Encounters/InMemoryEncounterCompletionStore.cs @@ -13,7 +13,15 @@ public sealed class InMemoryEncounterCompletionStore : IEncounterCompletionStore public bool IsCompleted(string playerId, string encounterId) { var key = EncounterProgressIds.MakeProgressKey(playerId, encounterId); - return key.Length > 0 && completedAtByKey.ContainsKey(key); + if (key.Length == 0) + { + return false; + } + + lock (keyLocks.GetOrAdd(key, _ => new object())) + { + return completedAtByKey.ContainsKey(key); + } } ///