From f51617f8b57b8e9685f6b56f5e945e4f985b1ded Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 31 May 2026 18:27:28 -0400 Subject: [PATCH] NEO-107: Address code review findings for event store persistence. Update E5.M3 module summary and NEO-105 snapshot, fix store test AAA layout, add empty encounter id fail-closed test, strike through review items. --- .../modules/E5_M3_EncounterAndRewardTables.md | 4 ++-- docs/reviews/2026-05-31-NEO-107.md | 10 ++++----- ...nMemoryEncounterCompleteEventStoreTests.cs | 21 ++++++++++++++----- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md b/docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md index fae7d9a..87c5dc1 100644 --- a/docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md +++ b/docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md @@ -7,7 +7,7 @@ | **Module ID** | E5.M3 | | **Epic** | [Epic 5 — PvE Combat](../epics/epic_05_pve_combat.md) | | **Stage target** | Prototype | -| **Status** | In Progress — Slice 3 backlog [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md): **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) catalog + CI landed · **E5M3-02** [NEO-101](https://linear.app/neon-sprawl/issue/NEO-101) server load landed · **E5M3-03** [NEO-102](https://linear.app/neon-sprawl/issue/NEO-102) registries landed · **E5M3-04** [NEO-103](https://linear.app/neon-sprawl/issue/NEO-103) HTTP read landed · **E5M3-05** [NEO-104](https://linear.app/neon-sprawl/issue/NEO-104) progress + completion stores landed · **E5M3-06** [NEO-105](https://linear.app/neon-sprawl/issue/NEO-105) completion grants landed · **E5M3-07** [NEO-106](https://linear.app/neon-sprawl/issue/NEO-106) combat wiring landed · **E5M3-08** [NEO-108](https://linear.app/neon-sprawl/issue/NEO-108) → **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) | +| **Status** | In Progress — Slice 3 backlog [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md): **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) catalog + CI landed · **E5M3-02** [NEO-101](https://linear.app/neon-sprawl/issue/NEO-101) server load landed · **E5M3-03** [NEO-102](https://linear.app/neon-sprawl/issue/NEO-102) registries landed · **E5M3-04** [NEO-103](https://linear.app/neon-sprawl/issue/NEO-103) HTTP read landed · **E5M3-05** [NEO-104](https://linear.app/neon-sprawl/issue/NEO-104) progress + completion stores landed · **E5M3-06** [NEO-105](https://linear.app/neon-sprawl/issue/NEO-105) completion grants landed · **E5M3-07** [NEO-106](https://linear.app/neon-sprawl/issue/NEO-106) combat wiring landed · **E5M3-09** [NEO-107](https://linear.app/neon-sprawl/issue/NEO-107) event record landed · **E5M3-08** [NEO-108](https://linear.app/neon-sprawl/issue/NEO-108) → **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) | | **Linear** | Label **`E5.M3`** · [E5M3-prototype-backlog.md](../../plans/E5M3-prototype-backlog.md) **E5M3-01** [NEO-100](https://linear.app/neon-sprawl/issue/NEO-100) · **E5M3-02** [NEO-101](https://linear.app/neon-sprawl/issue/NEO-101) · **E5M3-03** [NEO-102](https://linear.app/neon-sprawl/issue/NEO-102) · **E5M3-04** [NEO-103](https://linear.app/neon-sprawl/issue/NEO-103) · **E5M3-05** [NEO-104](https://linear.app/neon-sprawl/issue/NEO-104) · **E5M3-06** [NEO-105](https://linear.app/neon-sprawl/issue/NEO-105) · **E5M3-07** [NEO-106](https://linear.app/neon-sprawl/issue/NEO-106) · **E5M3-08** [NEO-108](https://linear.app/neon-sprawl/issue/NEO-108) · **E5M3-09** [NEO-107](https://linear.app/neon-sprawl/issue/NEO-107) · **E5M3-10** [NEO-109](https://linear.app/neon-sprawl/issue/NEO-109) · **E5M3-11** [NEO-110](https://linear.app/neon-sprawl/issue/NEO-110) · **E5M3-12** [NEO-111](https://linear.app/neon-sprawl/issue/NEO-111) | ## Purpose @@ -88,7 +88,7 @@ The **first shipped encounter + reward spine** is **frozen** for prototype tunin **Progress + completion stores landed ([NEO-104](https://linear.app/neon-sprawl/issue/NEO-104)):** **`IEncounterProgressStore`** + **`IEncounterCompletionStore`** (in-memory) + **`EncounterProgressOperations`** — activation, order-independent defeat tracking, idempotent completion flag; combat/HTTP wiring deferred to NEO-106/108. Plan: [NEO-104 implementation plan](../../plans/NEO-104-implementation-plan.md); [server README — Encounter progress](../../../server/README.md#encounter-progress--completion-stores-neo-104). -**Completion + inventory grants landed ([NEO-105](https://linear.app/neon-sprawl/issue/NEO-105)):** **`EncounterCompletionOperations.TryCompleteAndGrant`** + **`EncounterCompleteEvent`** result payload — reward-table fixed grants via **`PlayerInventoryOperations`**, fail-closed inventory deny, idempotent completion mark; event persistence deferred to NEO-107. Plan: [NEO-105 implementation plan](../../plans/NEO-105-implementation-plan.md); [server README — Encounter completion](../../../server/README.md#encounter-completion--inventory-grants-neo-105). +**Completion + inventory grants landed ([NEO-105](https://linear.app/neon-sprawl/issue/NEO-105)):** **`EncounterCompletionOperations.TryCompleteAndGrant`** + **`EncounterCompleteEvent`** result payload — reward-table fixed grants via **`PlayerInventoryOperations`**, fail-closed inventory deny, idempotent completion mark; event record persisted via **`IEncounterCompleteEventStore`** ([NEO-107](https://linear.app/neon-sprawl/issue/NEO-107)). Plan: [NEO-105 implementation plan](../../plans/NEO-105-implementation-plan.md); [server README — Encounter completion](../../../server/README.md#encounter-completion--inventory-grants-neo-105). **Combat wiring landed ([NEO-106](https://linear.app/neon-sprawl/issue/NEO-106)):** **`EncounterCombatWiring.TryProcessCastOutcome`** on **`AbilityCastApi`** — activate on first damaging hit, mark defeats, invoke completion grants when all required NPCs defeated; NEO-44 gig XP preserved. Per-player progress HTTP deferred to NEO-108. Plan: [NEO-106 implementation plan](../../plans/NEO-106-implementation-plan.md); [server README — Encounter combat wiring](../../../server/README.md#encounter-combat-wiring-neo-106). diff --git a/docs/reviews/2026-05-31-NEO-107.md b/docs/reviews/2026-05-31-NEO-107.md index 5f08e3a..74d86e9 100644 --- a/docs/reviews/2026-05-31-NEO-107.md +++ b/docs/reviews/2026-05-31-NEO-107.md @@ -18,7 +18,7 @@ NEO-107 adds **`IEncounterCompleteEventStore`** and **`InMemoryEncounterComplete |------|--------| | `docs/plans/NEO-107-implementation-plan.md` | **Matches** — kickoff decisions adopted; acceptance checklist checked; reconciliation section accurate; frozen completion→event flow table reflected in code and tests. | | `docs/plans/E5M3-prototype-backlog.md` (E5M3-09) | **Matches** — acceptance criteria checked; landed note cites plan. | -| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Partially matches** — implementation snapshot bullet for NEO-107 added; summary **Status** row (line 10) still omits **E5M3-09 / NEO-107 landed**; NEO-105 bullet still says “event persistence deferred to NEO-107” (now landed). | +| `docs/decomposition/modules/E5_M3_EncounterAndRewardTables.md` | **Matches** — implementation snapshot bullet for NEO-107 added; summary **Status** row includes **E5M3-09 / NEO-107 landed**; NEO-105 bullet cross-links NEO-107 persistence (review follow-up). | | `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-107 producer hook anchor added with correct consumer contract vocabulary. | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M3 row notes **NEO-107** event record. | | `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E5.M3 note cites **NEO-107** event store landed. | @@ -31,15 +31,15 @@ None. ## Suggestions -1. **E5.M3 summary Status row** — Update the module summary table (line 10) to include **E5M3-09 / NEO-107** alongside the other landed slices so the header matches the implementation snapshot bullets below. +1. ~~**E5.M3 summary Status row** — Update the module summary table (line 10) to include **E5M3-09 / NEO-107** alongside the other landed slices so the header matches the implementation snapshot bullets below.~~ **Done.** Status row updated in `E5_M3_EncounterAndRewardTables.md`. -2. **Stale NEO-105 deferral line** — In `E5_M3_EncounterAndRewardTables.md`, the NEO-105 bullet still reads “event persistence deferred to NEO-107.” Reword to point at the NEO-107 snapshot (or drop the deferral phrase) now that persistence is landed. +2. ~~**Stale NEO-105 deferral line** — In `E5_M3_EncounterAndRewardTables.md`, the NEO-105 bullet still reads “event persistence deferred to NEO-107.” Reword to point at the NEO-107 snapshot (or drop the deferral phrase) now that persistence is landed.~~ **Done.** NEO-105 bullet now cross-links **`IEncounterCompleteEventStore`** (NEO-107). -3. **Empty encounter id fail-closed test** — Plan test table lists “empty key / invalid ids fail closed.” `InMemoryEncounterCompleteEventStoreTests` covers whitespace **`PlayerId`** only; a symmetric case with empty **`EncounterId`** would close the gap (store layer only). +3. ~~**Empty encounter id fail-closed test** — Plan test table lists “empty key / invalid ids fail closed.” `InMemoryEncounterCompleteEventStoreTests` covers whitespace **`PlayerId`** only; a symmetric case with empty **`EncounterId`** would close the gap (store layer only).~~ **Done.** Added `TryRecord_ShouldFailClosed_WhenEncounterIdIsEmpty`; split empty **`PlayerId`** into its own test. ## Nits -- Nit: **`InMemoryEncounterCompleteEventStoreTests`** — both test methods call **`TryGet`** under **Act**; move verification reads to **Assert** for full AAA per `csharp-style.md`. +- ~~Nit: **`InMemoryEncounterCompleteEventStoreTests`** — both test methods call **`TryGet`** under **Act**; move verification reads to **Assert** for full AAA per `csharp-style.md`.~~ **Done.** **`TryGet`** moved to **Assert** in all store tests. - Nit: **`EncounterDefinitionRegistryTests`** resolves **`IEncounterCompleteEventStore`** in **Arrange** but only asserts **`IsType`** — sufficient for DI smoke; no functional **`TryRecord`** assertion needed unless you want parity with progress/completion store tests. diff --git a/server/NeonSprawl.Server.Tests/Game/Encounters/InMemoryEncounterCompleteEventStoreTests.cs b/server/NeonSprawl.Server.Tests/Game/Encounters/InMemoryEncounterCompleteEventStoreTests.cs index 476e480..8db7ede 100644 --- a/server/NeonSprawl.Server.Tests/Game/Encounters/InMemoryEncounterCompleteEventStoreTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Encounters/InMemoryEncounterCompleteEventStoreTests.cs @@ -18,28 +18,39 @@ public sealed class InMemoryEncounterCompleteEventStoreTests // Act var first = store.TryRecord(completeEvent); var second = store.TryRecord(completeEvent with { CompletedAt = CompletedAt.AddHours(1) }); - var found = store.TryGet(PlayerId, EncounterId, out var stored); // Assert Assert.True(first); Assert.False(second); - Assert.True(found); + Assert.True(store.TryGet(PlayerId, EncounterId, out var stored)); Assert.Equal(completeEvent.IdempotencyKey, stored.IdempotencyKey); Assert.Equal(CompletedAt, stored.CompletedAt); Assert.Equal(2, stored.GrantedItems.Count); } [Fact] - public void TryRecord_ShouldFailClosed_WhenPlayerOrEncounterIdIsEmpty() + public void TryRecord_ShouldFailClosed_WhenPlayerIdIsEmpty() { // Arrange var store = new InMemoryEncounterCompleteEventStore(); var invalidEvent = CreatePrototypeEvent() with { PlayerId = " " }; // Act var recorded = store.TryRecord(invalidEvent); - var found = store.TryGet(" ", EncounterId, out _); // Assert Assert.False(recorded); - Assert.False(found); + Assert.False(store.TryGet(" ", EncounterId, out _)); + } + + [Fact] + public void TryRecord_ShouldFailClosed_WhenEncounterIdIsEmpty() + { + // Arrange + var store = new InMemoryEncounterCompleteEventStore(); + var invalidEvent = CreatePrototypeEvent() with { EncounterId = " " }; + // Act + var recorded = store.TryRecord(invalidEvent); + // Assert + Assert.False(recorded); + Assert.False(store.TryGet(PlayerId, " ", out _)); } private static EncounterCompleteEvent CreatePrototypeEvent() =>