From 38f0c567111baa6aa3986dd42f4bc474258fb484 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 17 May 2026 22:04:03 -0400 Subject: [PATCH] =?UTF-8?q?NEO-51:=20Address=20code=20review=20=E2=80=94?= =?UTF-8?q?=20tests,=20docs,=20review=20strikethroughs.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add Slice 1 role/stackMax and missing-path loader tests; E3.M3 server load bullet; register footnote; host test temp-dir cleanup. --- .../E3_M3_ItemizationAndInventorySchema.md | 2 + .../modules/module_dependency_register.md | 2 +- docs/reviews/2026-05-17-NEO-51.md | 10 +- .../Items/ItemDefinitionCatalogLoaderTests.cs | 108 ++++++++++++++++-- 4 files changed, 106 insertions(+), 16 deletions(-) diff --git a/docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md b/docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md index cb68857..f502ff6 100644 --- a/docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md +++ b/docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md @@ -62,6 +62,8 @@ The **first shipped six-item spine** under `content/items/*.json` is **frozen** Epic 3 **Slice 1** — MVP inventory; `item_created`, transfer failures. +**Server load (NEO-51):** On host startup, `server/NeonSprawl.Server/Game/Items/` loads `content/items/*_items.json` with the same validation gates as CI (`scripts/validate_content.py`) and **refuses to listen** when the catalog is invalid. Config and discovery: [server README — Item catalog](../../../server/README.md#item-catalog-contentitems-neo-51). Plan: [NEO-51 implementation plan](../../plans/NEO-51-implementation-plan.md). + **Linear backlog (decomposed):** [E3M3-prototype-backlog.md](../../plans/E3M3-prototype-backlog.md) — **E3M3-01** [NEO-50](https://linear.app/neon-sprawl/issue/NEO-50) (content + CI) through **E3M3-07** [NEO-56](https://linear.app/neon-sprawl/issue/NEO-56) (telemetry hooks). ## Source anchors diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index 10796c5..6138a68 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.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+** moves the register row to **In Progress** when slices land. +**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) and **NEO-51** (server fail-fast load) moved the register row to **In Progress**; later slices update the alignment table as they land. ### Epic 4 — World Topology diff --git a/docs/reviews/2026-05-17-NEO-51.md b/docs/reviews/2026-05-17-NEO-51.md index fe9b4a7..c33af7e 100644 --- a/docs/reviews/2026-05-17-NEO-51.md +++ b/docs/reviews/2026-05-17-NEO-51.md @@ -8,6 +8,8 @@ **Approve with nits** +**Follow-up:** Suggestions 1–3 and the temp-dir nit below are **done** (strikethrough + **Done.**). + ## Summary NEO-51 adds server-side fail-fast loading of `content/items/*_items.json` at startup, mirroring the NEO-34 skill-catalog pattern: JsonSchema.Net row validation, duplicate-`id` rejection, `schemaVersion` 1 enforcement, and a C# prototype Slice 1 gate aligned with `scripts/validate_content.py`. `ItemDefinitionCatalog` is registered as a singleton and eagerly resolved in `Program.cs` before the host listens; tests cover loader happy/failure paths and host boot success/failure. The change is localized, well-tested for the main failure classes, and documentation/plan alignment is strong. Residual risk is low: constants drift vs Python (mitigated by comments) and two plan-listed negative tests not yet written. @@ -28,15 +30,15 @@ None. ## Suggestions -1. **Test gaps vs plan** — [`NEO-51-implementation-plan.md`](../plans/NEO-51-implementation-plan.md) lists unit tests for **wrong `prototypeRole` set** and **`equip_stub` with `stackMax` ≠ 1**. Coverage today includes incomplete ids and bag slot on equip_stub, but not those two cases. Add focused loader tests (AAA) so a regression in `PrototypeSlice1ItemCatalogRules` cannot slip through on role multiset or stack max alone. +1. ~~**Test gaps vs plan** — [`NEO-51-implementation-plan.md`](../plans/NEO-51-implementation-plan.md) lists unit tests for **wrong `prototypeRole` set** and **`equip_stub` with `stackMax` ≠ 1**. Coverage today includes incomplete ids and bag slot on equip_stub, but not those two cases. Add focused loader tests (AAA) so a regression in `PrototypeSlice1ItemCatalogRules` cannot slip through on role multiset or stack max alone.~~ **Done.** — `Load_ShouldThrow_WhenSlice1PrototypeRolesDoNotMatch`, `Load_ShouldThrow_WhenEquipStubStackMaxIsNotOne` in `ItemDefinitionCatalogLoaderTests.cs`. -2. **E3.M3 module page** — Add a short “Server load (NEO-51)” bullet under related implementation (or refresh the summary **Status**) pointing at `Game/Items/` and fail-fast boot, parallel to E2.M1’s NEO-34 note in the alignment table. Keeps module doc and alignment doc in sync for readers who skip the register. +2. ~~**E3.M3 module page** — Add a short “Server load (NEO-51)” bullet under related implementation (or refresh the summary **Status**) pointing at `Game/Items/` and fail-fast boot, parallel to E2.M1’s NEO-34 note in the alignment table. Keeps module doc and alignment doc in sync for readers who skip the register.~~ **Done.** — [E3_M3_ItemizationAndInventorySchema.md](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) **Server load (NEO-51)** bullet; register footnote names NEO-51. -3. **Early-path loader tests (optional)** — Consider tests for missing items directory and missing schema file to lock the first `ThrowIfAny` messages; low priority because host boot test covers empty catalog dir. +3. ~~**Early-path loader tests (optional)** — Consider tests for missing items directory and missing schema file to lock the first `ThrowIfAny` messages; low priority because host boot test covers empty catalog dir.~~ **Done.** — `Load_ShouldThrow_WhenItemsDirectoryMissing`, `Load_ShouldThrow_WhenSchemaFileMissing`. ## Nits -- Nit: `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid` creates a temp directory and does not delete it (minor test hygiene). +- ~~Nit: `Host_ShouldFailStartup_WhenCatalogDirectoryInvalid` creates a temp directory and does not delete it (minor test hygiene).~~ **Done.** — `finally` deletes `badDir`. - Nit: `ItemDefinitionCatalog` copies `byId` into a new `Dictionary` then wraps `ReadOnlyDictionary` — fine for startup immutability; only worth revisiting if catalogs grow large. - Nit: Slice 1 id error strings use single-quoted lists; Python CI uses `repr()` — behavior equivalent, messages differ cosmetically in logs. diff --git a/server/NeonSprawl.Server.Tests/Game/Items/ItemDefinitionCatalogLoaderTests.cs b/server/NeonSprawl.Server.Tests/Game/Items/ItemDefinitionCatalogLoaderTests.cs index 3208b0e..a2ac9a8 100644 --- a/server/NeonSprawl.Server.Tests/Game/Items/ItemDefinitionCatalogLoaderTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Items/ItemDefinitionCatalogLoaderTests.cs @@ -1,5 +1,6 @@ using System.Net; using System.Text; +using System.Text.Json.Nodes; using Microsoft.AspNetCore.Mvc.Testing; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging.Abstractions; @@ -74,6 +75,22 @@ public class ItemDefinitionCatalogLoaderTests return (root.FullName, itemsDir, schemaPath); } + private static void WriteCatalog(string itemsDir, string catalogJson) => + File.WriteAllText(Path.Combine(itemsDir, "prototype_items.json"), catalogJson, Encoding.UTF8); + + private static JsonObject GetItemRow(JsonObject catalogRoot, string itemId) + { + var items = catalogRoot["items"] as JsonArray + ?? throw new InvalidOperationException("expected items array"); + foreach (var node in items) + { + if (node is JsonObject row && row["id"]?.GetValue() == itemId) + return row; + } + + throw new InvalidOperationException($"item id not found: {itemId}"); + } + [Fact] public void Load_ShouldSucceed_WhenCatalogMatchesPrototypeContract() { @@ -157,7 +174,7 @@ public class ItemDefinitionCatalogLoaderTests { // Arrange var (_, itemsDir, schemaPath) = CreateTempContentLayout(); - var singleItem = """ + const string singleItem = """ { "schemaVersion": 1, "items": [ @@ -188,7 +205,7 @@ public class ItemDefinitionCatalogLoaderTests { // Arrange var (_, itemsDir, schemaPath) = CreateTempContentLayout(); - var twoOnly = """ + const string twoOnly = """ { "schemaVersion": 1, "items": [ @@ -235,6 +252,67 @@ public class ItemDefinitionCatalogLoaderTests Assert.Contains("inventorySlotKind 'equipment'", ioe.Message, StringComparison.Ordinal); } + [Fact] + public void Load_ShouldThrow_WhenSlice1PrototypeRolesDoNotMatch() + { + // Arrange + var (_, itemsDir, schemaPath) = CreateTempContentLayout(); + var root = JsonNode.Parse(ValidPrototypeCatalogJson) as JsonObject + ?? throw new InvalidOperationException("expected object root"); + GetItemRow(root, "refined_plate_stock")["prototypeRole"] = "material"; + WriteCatalog(itemsDir, root.ToJsonString()); + // Act + var ex = Record.Exception(() => ItemDefinitionCatalogLoader.Load(itemsDir, schemaPath, NullLogger.Instance)); + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("requires exactly one row per prototypeRole", ioe.Message, StringComparison.Ordinal); + } + + [Fact] + public void Load_ShouldThrow_WhenEquipStubStackMaxIsNotOne() + { + // Arrange + var (_, itemsDir, schemaPath) = CreateTempContentLayout(); + var root = JsonNode.Parse(ValidPrototypeCatalogJson) as JsonObject + ?? throw new InvalidOperationException("expected object root"); + GetItemRow(root, "prototype_armor_shell")["stackMax"] = 5; + WriteCatalog(itemsDir, root.ToJsonString()); + // Act + var ex = Record.Exception(() => ItemDefinitionCatalogLoader.Load(itemsDir, schemaPath, NullLogger.Instance)); + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("prototype_armor_shell", ioe.Message, StringComparison.Ordinal); + Assert.Contains("stackMax 1", ioe.Message, StringComparison.Ordinal); + } + + [Fact] + public void Load_ShouldThrow_WhenItemsDirectoryMissing() + { + // Arrange + var missingDir = Path.Combine(Path.GetTempPath(), "neon-sprawl-no-items-" + Guid.NewGuid().ToString("n")); + var schemaPath = ItemCatalogTestPaths.DiscoverRepoItemDefSchemaPath(); + // Act + var ex = Record.Exception(() => ItemDefinitionCatalogLoader.Load(missingDir, schemaPath, NullLogger.Instance)); + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("missing directory", ioe.Message, StringComparison.Ordinal); + Assert.Contains(missingDir, ioe.Message, StringComparison.Ordinal); + } + + [Fact] + public void Load_ShouldThrow_WhenSchemaFileMissing() + { + // Arrange + var (_, itemsDir, _) = CreateTempContentLayout(); + var missingSchema = Path.Combine(itemsDir, "missing-item-def.schema.json"); + // Act + var ex = Record.Exception(() => ItemDefinitionCatalogLoader.Load(itemsDir, missingSchema, NullLogger.Instance)); + // Assert + var ioe = Assert.IsType(ex); + Assert.Contains("missing schema file", ioe.Message, StringComparison.Ordinal); + Assert.Contains(missingSchema, ioe.Message, StringComparison.Ordinal); + } + [Fact] public async Task Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds() { @@ -257,15 +335,23 @@ public class ItemDefinitionCatalogLoaderTests // Arrange var badDir = Path.Combine(Path.GetTempPath(), "neon-sprawl-empty-items-" + Guid.NewGuid().ToString("n")); Directory.CreateDirectory(badDir); - // Act - var ex = Record.Exception(() => + try { - using var factory = new WebApplicationFactory().WithWebHostBuilder(b => - b.UseSetting("Content:ItemsDirectory", badDir)); - factory.CreateClient(); - }); - // Assert - Assert.NotNull(ex); - Assert.Contains("Item catalog validation failed", ex.ToString(), StringComparison.Ordinal); + // Act + var ex = Record.Exception(() => + { + using var factory = new WebApplicationFactory().WithWebHostBuilder(b => + b.UseSetting("Content:ItemsDirectory", badDir)); + factory.CreateClient(); + }); + // Assert + Assert.NotNull(ex); + Assert.Contains("Item catalog validation failed", ex.ToString(), StringComparison.Ordinal); + } + finally + { + if (Directory.Exists(badDir)) + Directory.Delete(badDir); + } } }