don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

NEO-51: Address code review — tests, docs, review strikethroughs. 

Add Slice 1 role/stackMax and missing-path loader tests; E3.M3 server
load bullet; register footnote; host test temp-dir cleanup.
pull/86/head
VinPropane 2026-05-17 22:04:03 -04:00
parent 1c9c93513e
commit 38f0c56711
4 changed files with 106 additions and 16 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md
View File

@ -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

2
docs/decomposition/modules/module_dependency_register.md
View File

@ -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

10
docs/reviews/2026-05-17-NEO-51.md
View File

@ -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.

90
server/NeonSprawl.Server.Tests/Game/Items/ItemDefinitionCatalogLoaderTests.cs
View File

@ -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<string>() == 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<InvalidOperationException>(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<InvalidOperationException>(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<InvalidOperationException>(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<InvalidOperationException>(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,6 +335,8 @@ public class ItemDefinitionCatalogLoaderTests
// Arrange
var badDir = Path.Combine(Path.GetTempPath(), "neon-sprawl-empty-items-" + Guid.NewGuid().ToString("n"));
Directory.CreateDirectory(badDir);
try
{
// Act
var ex = Record.Exception(() =>
{
@ -268,4 +348,10 @@ public class ItemDefinitionCatalogLoaderTests
Assert.NotNull(ex);
Assert.Contains("Item catalog validation failed", ex.ToString(), StringComparison.Ordinal);
}
finally
{
if (Directory.Exists(badDir))
Directory.Delete(badDir);
}
}
}