From 0cc5bb892590498bb8935b106f5e101912116b26 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 15:15:25 -0400 Subject: [PATCH] NEO-61: address code review follow-ups. Fix AAA layout in depletion tests, add ResourceNodeInstanceIds.Normalize, throw on failed lazy-init after catalog hit, and update dependency register footnote plus review doc strikethroughs. --- .../modules/module_dependency_register.md | 2 +- docs/reviews/2026-05-24-NEO-61.md | 18 +++++++++--------- .../ResourceNodeInstanceOperationsTests.cs | 18 +++++++++++------- ...eNodeInstancePersistenceIntegrationTests.cs | 13 +++++++------ .../InMemoryResourceNodeInstanceStore.cs | 17 +++-------------- .../PostgresResourceNodeInstanceStore.cs | 6 +++--- .../Game/Gathering/ResourceNodeInstanceIds.cs | 17 +++++++++++++++++ .../ResourceNodeInstanceOperations.cs | 8 +++----- 8 files changed, 54 insertions(+), 45 deletions(-) create mode 100644 server/NeonSprawl.Server/Game/Gathering/ResourceNodeInstanceIds.cs diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index 34a38fc..a72eeb2 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -48,7 +48,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen | E3.M4 | SinkAndDurabilityLifecycle | E3.M3, E8.M3 | DurabilityState, ItemSinkEvent, RepairCostRule | Pre-production | Planned | | E3.M5 | EconomyBalancePolicy | E3.M4, E9.M2 | EconomyPolicy, PriceBandRule, FaucetSinkRatio | Pre-production | Planned | -**E3.M1 note:** Epic 3 **Slice 2** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-57](https://linear.app/neon-sprawl/issue/NEO-57) → [NEO-64](https://linear.app/neon-sprawl/issue/NEO-64); label **`E3.M1`**. See [E3M1-prototype-backlog.md](../../plans/E3M1-prototype-backlog.md), [E3_M1_ResourceNodeAndGatherLoop.md](E3_M1_ResourceNodeAndGatherLoop.md). **NEO-57** (content + CI), **NEO-58** (server fail-fast load), **NEO-59** (`IResourceNodeDefinitionRegistry` + DI), **NEO-60** (`GET /game/world/resource-node-definitions`) — E3M1-01–**E3M1-04** complete; register row **In Progress** until depletion / gather engine slices land. **NEO-41** (prototype gather XP on `resource_node` interact) remains until **E3M1-07** migrates into the gather engine. Remaining Slice 2 work adds **`GatherResult`**, depletion store, inventory grants, and telemetry hooks **`resource_gathered`** / **`gather_node_depleted`**. +**E3.M1 note:** Epic 3 **Slice 2** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-57](https://linear.app/neon-sprawl/issue/NEO-57) → [NEO-64](https://linear.app/neon-sprawl/issue/NEO-64); label **`E3.M1`**. See [E3M1-prototype-backlog.md](../../plans/E3M1-prototype-backlog.md), [E3_M1_ResourceNodeAndGatherLoop.md](E3_M1_ResourceNodeAndGatherLoop.md). **NEO-57** (content + CI), **NEO-58** (server fail-fast load), **NEO-59** (`IResourceNodeDefinitionRegistry` + DI), **NEO-60** (`GET /game/world/resource-node-definitions`), **NEO-61** (`IResourceNodeInstanceStore` + depletion operations, `V006`) — E3M1-01–**E3M1-05** complete; register row **In Progress** until gather engine / interact wiring slices land. **NEO-41** (prototype gather XP on `resource_node` interact) remains until **E3M1-07** migrates into the gather engine. Remaining Slice 2 work adds **`GatherResult`**, inventory grants, and telemetry hooks **`resource_gathered`** / **`gather_node_depleted`**. **E3.M3 note:** Epic 3 **Slice 1** backlog in Linear ([Epic 3 — Crafting, Gathering, and Itemization Economy](https://linear.app/neon-sprawl/project/epic-3-crafting-gathering-and-itemization-economy-65785ed05bc2)): [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) → [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56); label **`E3.M3`**. See [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md), [E3_M3_ItemizationAndInventorySchema.md](E3_M3_ItemizationAndInventorySchema.md). **NEO-50** (content + CI), **NEO-51** (server fail-fast load), **NEO-52** (`IItemDefinitionRegistry` + DI), **NEO-53** (`GET /game/world/item-definitions`), **NEO-54** (inventory store + stack/slot rules engine), **NEO-55** (`GET`/`POST /game/players/{id}/inventory`), and **NEO-56** (comment-only `item_created` / `inventory_transfer_denied` telemetry hook sites in `PlayerInventoryOperations`) — Epic 3 Slice 1 backlog **E3M3-01**–**E3M3-07** complete; register row **In Progress** until Slice 2+; later slices update the alignment table as they land. diff --git a/docs/reviews/2026-05-24-NEO-61.md b/docs/reviews/2026-05-24-NEO-61.md index d8906e1..565edbe 100644 --- a/docs/reviews/2026-05-24-NEO-61.md +++ b/docs/reviews/2026-05-24-NEO-61.md @@ -20,11 +20,11 @@ NEO-61 adds server-authoritative **remaining gathers** per world node instance: | [`docs/plans/E3M1-prototype-backlog.md`](../plans/E3M1-prototype-backlog.md) | **Matches** — E3M1-05 acceptance items flipped with NEO-61 landed note. | | [`docs/decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md`](../decomposition/modules/E3_M1_ResourceNodeAndGatherLoop.md) | **Matches** — NEO-61 depletion-store bullet under **Related implementation slices**; per-instance capacity, world-wide keying, **`node_depleted`** align with **Purpose** / **Responsibilities**. | | [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E3.M1 row notes NEO-61 landed with README link; **Still planned** correctly lists gather engine / interact wiring. | -| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M1 row **In Progress** is correct, but **E3.M1 note** footnote still lists NEO-57–NEO-60 only and says “until depletion … slices land” without NEO-61 (alignment table already updated). | +| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E3.M1 row **In Progress** is correct; **E3.M1 note** footnote updated for NEO-61 (E3M1-05). | | [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **Matches** — server-authoritative capacity; no client-trusted depletion state. | | [`server/README.md`](../../server/README.md) | **Matches** — Resource node instance depletion subsection: lazy init, reason-code table, persistence split, NEO-63 deferral. | -Register/tracking: alignment table update is done; **E3.M1 note** footnote should mention NEO-61 when touched (same pattern as NEO-60 register suggestion). +Register/tracking: alignment table and **E3.M1 note** footnote updated for NEO-61. ## Blocking issues @@ -32,20 +32,20 @@ None. ## Suggestions -1. **AAA layout in stateful tests** — Two new test methods mix **Assert** into **Act**: - - `ResourceNodeInstanceOperationsTests.TryCommitSuccessfulGatherDecrement_RepeatedUntilCapacity_ShouldReturnNodeDepletedWithoutGoingNegative` — assertions inside the decrement loop. - - `ResourceNodeInstancePersistenceIntegrationTests.TryCommitSuccessfulGatherDecrement_WhenDepleted_ShouldPersistZeroRow` — assertions inside the final `using` Act block. - Move loop invariants / final expectations to a dedicated **Assert** section (collect last applied outcome during Act, assert after) per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert). +1. ~~**AAA layout in stateful tests** — Two new test methods mix **Assert** into **Act**:~~ Done. + - ~~`ResourceNodeInstanceOperationsTests.TryCommitSuccessfulGatherDecrement_RepeatedUntilCapacity_ShouldReturnNodeDepletedWithoutGoingNegative` — assertions inside the decrement loop.~~ + - ~~`ResourceNodeInstancePersistenceIntegrationTests.TryCommitSuccessfulGatherDecrement_WhenDepleted_ShouldPersistZeroRow` — assertions inside the final `using` Act block.~~ + ~~Move loop invariants / final expectations to a dedicated **Assert** section (collect last applied outcome during Act, assert after) per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert).~~ -2. **Dependency register footnote (optional)** — Extend **E3.M1 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to include **NEO-61** (E3M1-05 depletion store) so the register matches the alignment table — same follow-up as NEO-60 for NEO-57–NEO-60. +2. ~~**Dependency register footnote (optional)** — Extend **E3.M1 note** in [`module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) to include **NEO-61** (E3M1-05 depletion store) so the register matches the alignment table — same follow-up as NEO-60 for NEO-57–NEO-60.~~ Done. 3. **Concurrency test gap (optional)** — Plan §8 listed in-memory get/ensure/decrement **concurrency hygiene** tests; implementation uses per-id locks but has no parallel test. NEO-54 inventory store also omits concurrent tests, so non-blocking; add a lightweight `Parallel.For` smoke test if you want plan parity. ## Nits -- Nit: **`NormalizeInteractableId`** lives as `internal static` on **`InMemoryResourceNodeInstanceStore`** but is called from **`PostgresResourceNodeInstanceStore`** and **`ResourceNodeInstanceOperations`** — works, but a small shared helper (e.g. `ResourceNodeInstanceIds.Normalize`) would reduce cross-type coupling if this grows. +- ~~Nit: **`NormalizeInteractableId`** lives as `internal static` on **`InMemoryResourceNodeInstanceStore`** but is called from **`PostgresResourceNodeInstanceStore`** and **`ResourceNodeInstanceOperations`** — works, but a small shared helper (e.g. `ResourceNodeInstanceIds.Normalize`) would reduce cross-type coupling if this grows.~~ Done. -- Nit: **`TryEnsureInitialized` failure → `unknown_node`** in `ResourceNodeInstanceOperations` (lines 26–31) is unreachable after a registry hit with non-negative **`maxGathers`**; a distinct internal reason or `Debug.Assert` would clarify intent vs catalog miss. +- ~~Nit: **`TryEnsureInitialized` failure → `unknown_node`** in `ResourceNodeInstanceOperations` (lines 26–31) is unreachable after a registry hit with non-negative **`maxGathers`**; a distinct internal reason or `Debug.Assert` would clarify intent vs catalog miss.~~ Done — throws **`InvalidOperationException`** (programmer error after validated catalog **`maxGathers`**). - Nit: Postgres **`TryDecrementRemainingGathers`** opens a second connection via **`TryGetRemainingGathers`** on decrement miss to distinguish depleted-at-zero — correct behavior, slightly extra round-trip on deny path only. diff --git a/server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeInstanceOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeInstanceOperationsTests.cs index 7ea881a..d8f741e 100644 --- a/server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeInstanceOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeInstanceOperationsTests.cs @@ -37,24 +37,28 @@ public sealed class ResourceNodeInstanceOperationsTests var registry = factory.Services.GetRequiredService(); var store = factory.Services.GetRequiredService(); - // Act — ten successful gathers (10 → 0) - ResourceNodeInstanceMutationOutcome lastApplied = default; - for (var i = 0; i < 10; i++) + // Act — ten successful gathers (10 → 0), then one depletion attempt + var appliedOutcomes = new ResourceNodeInstanceMutationOutcome[10]; + for (var i = 0; i < appliedOutcomes.Length; i++) { - lastApplied = ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement(AlphaId, registry, store); - Assert.Equal(ResourceNodeInstanceMutationKind.Applied, lastApplied.Kind); - Assert.True(lastApplied.Snapshot!.Value.RemainingGathers >= 0); + appliedOutcomes[i] = ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement(AlphaId, registry, store); } var depleted = ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement(AlphaId, registry, store); + store.TryGetRemainingGathers(AlphaId, out var persisted); // Assert + Assert.All(appliedOutcomes, static o => + { + Assert.Equal(ResourceNodeInstanceMutationKind.Applied, o.Kind); + Assert.True(o.Snapshot!.Value.RemainingGathers >= 0); + }); + var lastApplied = appliedOutcomes[^1]; Assert.Equal(0, lastApplied.Snapshot!.Value.RemainingGathers); Assert.Equal(ResourceNodeInstanceMutationKind.Denied, depleted.Kind); Assert.Equal(ResourceNodeInstanceReasonCodes.NodeDepleted, depleted.ReasonCode); Assert.NotNull(depleted.Snapshot); Assert.Equal(0, depleted.Snapshot!.Value.RemainingGathers); - store.TryGetRemainingGathers(AlphaId, out var persisted); Assert.Equal(0, persisted.RemainingGathers); } diff --git a/server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeInstancePersistenceIntegrationTests.cs b/server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeInstancePersistenceIntegrationTests.cs index 133e266..eaa7c38 100644 --- a/server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeInstancePersistenceIntegrationTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Gathering/ResourceNodeInstancePersistenceIntegrationTests.cs @@ -58,19 +58,20 @@ public sealed class ResourceNodeInstancePersistenceIntegrationTests(PostgresInte // Act ResourceNodeInstanceMutationOutcome denyOutcome; + ResourceNodeInstanceSnapshot persisted; await using var secondFactory = new PostgresWebApplicationFactory(); using (var scope = secondFactory.Services.CreateScope()) { var registry = scope.ServiceProvider.GetRequiredService(); var store = scope.ServiceProvider.GetRequiredService(); denyOutcome = ResourceNodeInstanceOperations.TryCommitSuccessfulGatherDecrement(AlphaId, registry, store); - store.TryGetRemainingGathers(AlphaId, out var snapshot); - - // Assert - Assert.Equal(ResourceNodeInstanceMutationKind.Denied, denyOutcome.Kind); - Assert.Equal(ResourceNodeInstanceReasonCodes.NodeDepleted, denyOutcome.ReasonCode); - Assert.Equal(0, snapshot.RemainingGathers); + store.TryGetRemainingGathers(AlphaId, out persisted); } + + // Assert + Assert.Equal(ResourceNodeInstanceMutationKind.Denied, denyOutcome.Kind); + Assert.Equal(ResourceNodeInstanceReasonCodes.NodeDepleted, denyOutcome.ReasonCode); + Assert.Equal(0, persisted.RemainingGathers); } private async Task ResetInstanceTableAsync() diff --git a/server/NeonSprawl.Server/Game/Gathering/InMemoryResourceNodeInstanceStore.cs b/server/NeonSprawl.Server/Game/Gathering/InMemoryResourceNodeInstanceStore.cs index 96abe62..c061272 100644 --- a/server/NeonSprawl.Server/Game/Gathering/InMemoryResourceNodeInstanceStore.cs +++ b/server/NeonSprawl.Server/Game/Gathering/InMemoryResourceNodeInstanceStore.cs @@ -12,7 +12,7 @@ public sealed class InMemoryResourceNodeInstanceStore : IResourceNodeInstanceSto /// public bool TryGetRemainingGathers(string interactableId, out ResourceNodeInstanceSnapshot snapshot) { - var key = NormalizeInteractableId(interactableId); + var key = ResourceNodeInstanceIds.Normalize(interactableId); if (key.Length == 0) { snapshot = default; @@ -35,7 +35,7 @@ public sealed class InMemoryResourceNodeInstanceStore : IResourceNodeInstanceSto /// public bool TryEnsureInitialized(string interactableId, int initialRemainingGathers) { - var key = NormalizeInteractableId(interactableId); + var key = ResourceNodeInstanceIds.Normalize(interactableId); if (key.Length == 0 || initialRemainingGathers < 0) { return false; @@ -61,7 +61,7 @@ public sealed class InMemoryResourceNodeInstanceStore : IResourceNodeInstanceSto { previousRemaining = 0; remainingGathers = 0; - var key = NormalizeInteractableId(interactableId); + var key = ResourceNodeInstanceIds.Normalize(interactableId); if (key.Length == 0) { return false; @@ -85,15 +85,4 @@ public sealed class InMemoryResourceNodeInstanceStore : IResourceNodeInstanceSto return true; } } - - internal static string NormalizeInteractableId(string? interactableId) - { - var trimmed = interactableId?.Trim(); - if (string.IsNullOrEmpty(trimmed)) - { - return string.Empty; - } - - return trimmed.ToLowerInvariant(); - } } diff --git a/server/NeonSprawl.Server/Game/Gathering/PostgresResourceNodeInstanceStore.cs b/server/NeonSprawl.Server/Game/Gathering/PostgresResourceNodeInstanceStore.cs index f3d636b..a00f8aa 100644 --- a/server/NeonSprawl.Server/Game/Gathering/PostgresResourceNodeInstanceStore.cs +++ b/server/NeonSprawl.Server/Game/Gathering/PostgresResourceNodeInstanceStore.cs @@ -6,7 +6,7 @@ public sealed class PostgresResourceNodeInstanceStore(Npgsql.NpgsqlDataSource da /// public bool TryGetRemainingGathers(string interactableId, out ResourceNodeInstanceSnapshot snapshot) { - var key = InMemoryResourceNodeInstanceStore.NormalizeInteractableId(interactableId); + var key = ResourceNodeInstanceIds.Normalize(interactableId); if (key.Length == 0) { snapshot = default; @@ -34,7 +34,7 @@ public sealed class PostgresResourceNodeInstanceStore(Npgsql.NpgsqlDataSource da /// public bool TryEnsureInitialized(string interactableId, int initialRemainingGathers) { - var key = InMemoryResourceNodeInstanceStore.NormalizeInteractableId(interactableId); + var key = ResourceNodeInstanceIds.Normalize(interactableId); if (key.Length == 0 || initialRemainingGathers < 0) { return false; @@ -63,7 +63,7 @@ public sealed class PostgresResourceNodeInstanceStore(Npgsql.NpgsqlDataSource da { previousRemaining = 0; remainingGathers = 0; - var key = InMemoryResourceNodeInstanceStore.NormalizeInteractableId(interactableId); + var key = ResourceNodeInstanceIds.Normalize(interactableId); if (key.Length == 0) { return false; diff --git a/server/NeonSprawl.Server/Game/Gathering/ResourceNodeInstanceIds.cs b/server/NeonSprawl.Server/Game/Gathering/ResourceNodeInstanceIds.cs new file mode 100644 index 0000000..63731f1 --- /dev/null +++ b/server/NeonSprawl.Server/Game/Gathering/ResourceNodeInstanceIds.cs @@ -0,0 +1,17 @@ +namespace NeonSprawl.Server.Game.Gathering; + +/// Canonical interactable id normalization for resource-node instance stores (NEO-61). +public static class ResourceNodeInstanceIds +{ + /// Trim + lowercase; empty when input is null/whitespace. + public static string Normalize(string? interactableId) + { + var trimmed = interactableId?.Trim(); + if (string.IsNullOrEmpty(trimmed)) + { + return string.Empty; + } + + return trimmed.ToLowerInvariant(); + } +} diff --git a/server/NeonSprawl.Server/Game/Gathering/ResourceNodeInstanceOperations.cs b/server/NeonSprawl.Server/Game/Gathering/ResourceNodeInstanceOperations.cs index 8c16594..2186d45 100644 --- a/server/NeonSprawl.Server/Game/Gathering/ResourceNodeInstanceOperations.cs +++ b/server/NeonSprawl.Server/Game/Gathering/ResourceNodeInstanceOperations.cs @@ -12,7 +12,7 @@ public static class ResourceNodeInstanceOperations IResourceNodeDefinitionRegistry registry, IResourceNodeInstanceStore store) { - var key = InMemoryResourceNodeInstanceStore.NormalizeInteractableId(interactableId); + var key = ResourceNodeInstanceIds.Normalize(interactableId); if (key.Length == 0 || !registry.TryGetDefinition(key, out var definition)) { return new ResourceNodeInstanceMutationOutcome( @@ -25,10 +25,8 @@ public static class ResourceNodeInstanceOperations { if (!store.TryEnsureInitialized(key, definition.MaxGathers)) { - return new ResourceNodeInstanceMutationOutcome( - ResourceNodeInstanceMutationKind.Denied, - ResourceNodeInstanceReasonCodes.UnknownNode, - null); + throw new InvalidOperationException( + $"Failed to initialize resource node instance '{key}' with maxGathers {definition.MaxGathers}."); } }