From 1e9f1e9a3a68afb964454190c05a7852fd68b8cb Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 23 May 2026 18:09:02 -0400 Subject: [PATCH] NEO-52: add implementation plan for item definition registry + DI Kickoff plan mirrors NEO-35 skill registry pattern on NEO-51 catalog; includes GetDefinitionsInIdOrder and TryGetDefinition per kickoff answers. --- docs/plans/NEO-52-implementation-plan.md | 92 ++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 docs/plans/NEO-52-implementation-plan.md diff --git a/docs/plans/NEO-52-implementation-plan.md b/docs/plans/NEO-52-implementation-plan.md new file mode 100644 index 0000000..47b561a --- /dev/null +++ b/docs/plans/NEO-52-implementation-plan.md @@ -0,0 +1,92 @@ +# NEO-52 — Implementation plan + +## Story reference + +| Field | Value | +|--------|--------| +| **Key** | NEO-52 | +| **Title** | E3.M3: Item definition registry + DI | +| **Linear** | https://linear.app/neon-sprawl/issue/NEO-52/e3m3-item-definition-registry-di | +| **Module** | [E3.M3 — ItemizationAndInventorySchema](../decomposition/modules/E3_M3_ItemizationAndInventorySchema.md) · Epic 3 Slice 1 (E3M3-03) | +| **Branch** | `NEO-52-item-definition-registry-di` | +| **Precursor** | [NEO-51](https://linear.app/neon-sprawl/issue/NEO-51) — fail-fast `ItemDefinitionCatalog` load (**Done** on `main`) | + +## Kickoff clarifications + +| Topic | Question | Agent recommendation | Answer | +|--------|----------|----------------------|--------| +| **Registry API surface** | Include `GetDefinitionsInIdOrder()` now or TryGet-only? | **Include enumeration** — mirror [NEO-35](NEO-35-implementation-plan.md) / `ISkillDefinitionRegistry`; [NEO-53](https://linear.app/neon-sprawl/issue/NEO-53) `GET /game/world/item-definitions` will need ordered defs without reaching into `ItemDefinitionCatalog`. | **User:** include `GetDefinitionsInIdOrder`. | +| **Lookup method name** | `TryGetDefinition` vs `TryGetItem`? | **`TryGetDefinition(string? itemId, …)`** — same naming as `ISkillDefinitionRegistry.TryGetDefinition`; catalog keeps `TryGetItem` for direct catalog access. | **User:** `TryGetDefinition`. | +| **NEO-51 vs NEO-52 split** | Combine registry with catalog load? | **Strict split** (already decided on NEO-51 kickoff) — NEO-51 ships loader + catalog; this story adds injectable registry only. | **Adopted** — NEO-51 plan + user answer on NEO-51 kickoff. | +| **Program.cs eager resolve** | Eager-resolve `IItemDefinitionRegistry` at boot? | **Omit** — `ItemDefinitionCatalog` is already eager-resolved in `Program.cs` (NEO-51); registry is a thin adapter (NEO-35 default). | **Adopted** | +| **Runtime validation** | Re-validate items in registry? | **No** — catalog load is fail-fast (NEO-51); registry delegates to loaded `ItemDefRow` only. | **Adopted** | + +## Goal, scope, and out-of-scope + +**Goal:** Provide **`IItemDefinitionRegistry`** backed by the startup-loaded **`ItemDefinitionCatalog`**: resolve by stable **`itemId`**, enumerate definitions in id order, expose prototype metadata (`displayName`, `prototypeRole`, `stackMax`, `inventorySlotKind`). Register in DI so **NEO-53+** (HTTP), **NEO-54** (inventory engine), and future craft/gather callers depend on the interface instead of the catalog type. + +**In scope (from Linear + E3M3-03):** + +- `ItemDefinitionRegistry` thin adapter over `ItemDefinitionCatalog`. +- DI registration alongside existing catalog singleton. +- Unit tests (AAA): known prototype id lookup + metadata; unknown `itemId` returns false without throwing; enumeration order; host resolves registry from DI. + +**Out of scope (from Linear):** + +- HTTP ([NEO-53](https://linear.app/neon-sprawl/issue/NEO-53)). +- Persistence, per-player inventory, stack mutation ([NEO-54+](E3M3-prototype-backlog.md)). +- Changing loader, Slice 1 gate, or catalog load semantics (NEO-51). + +## Acceptance criteria checklist + +- [ ] DI resolves `IItemDefinitionRegistry` at host startup (test via `InMemoryWebApplicationFactory`). +- [ ] Unit tests (AAA): lookup for known prototype `itemId` (e.g. `scrap_metal_bulk`, `prototype_armor_shell`) with expected metadata. +- [ ] Unit tests (AAA): unknown `itemId` returns false / absent without throwing. +- [ ] `GetDefinitionsInIdOrder` returns all loaded rows ordered by `id` (ordinal). + +## Technical approach + +1. **`IItemDefinitionRegistry`** — mirror [`ISkillDefinitionRegistry`](../../server/NeonSprawl.Server/Game/Skills/ISkillDefinitionRegistry.cs): + - `TryGetDefinition(string? itemId, [NotNullWhen(true)] out ItemDefRow? definition)` — null and unknown ids return false without throwing. + - `GetDefinitionsInIdOrder()` — all rows ordered by `ItemDefRow.Id` (ordinal). + - XML remarks: inventory/craft/gather callers (NEO-54+) should use this interface; HTTP read model (NEO-53) should not reach into `ItemDefinitionCatalog`. + +2. **`ItemDefinitionRegistry`** — primary-constructor adapter over `ItemDefinitionCatalog` (same pattern as [`SkillDefinitionRegistry`](../../server/NeonSprawl.Server/Game/Skills/SkillDefinitionRegistry.cs)). + +3. **DI** — extend [`AddItemDefinitionCatalog`](../../server/NeonSprawl.Server/Game/Items/ItemCatalogServiceCollectionExtensions.cs) to register `IItemDefinitionRegistry` → `ItemDefinitionRegistry` after catalog registration. No change to `Program.cs` eager-resolve (catalog only). + +4. **Comments** — update `ItemDefinitionCatalog` summary if needed to point at `IItemDefinitionRegistry`; update [server README](../../server/README.md) item-catalog section (currently says “until NEO-52”). + +5. **Tests** — new `ItemDefinitionRegistryTests.cs` mirroring [`SkillDefinitionRegistryTests`](../../server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionRegistryTests.cs): in-memory catalog helper, prototype fixture via loader + `ItemCatalogTestPaths`, host DI test. + +## Files to add + +| Path | Purpose | +|------|---------| +| `server/NeonSprawl.Server/Game/Items/IItemDefinitionRegistry.cs` | Public contract: lookup, enumerate, remarks for NEO-53/NEO-54 callers. | +| `server/NeonSprawl.Server/Game/Items/ItemDefinitionRegistry.cs` | Singleton adapter over `ItemDefinitionCatalog`. | +| `server/NeonSprawl.Server.Tests/Game/Items/ItemDefinitionRegistryTests.cs` | AAA unit + host DI tests (mirror `SkillDefinitionRegistryTests`). | + +## Files to modify + +| Path | Rationale | +|------|-----------| +| `server/NeonSprawl.Server/Game/Items/ItemCatalogServiceCollectionExtensions.cs` | Register `IItemDefinitionRegistry` → `ItemDefinitionRegistry` after catalog singleton. | +| `server/NeonSprawl.Server/Game/Items/ItemDefinitionCatalog.cs` | Tighten summary comment to reference `IItemDefinitionRegistry` (replace “NEO-52” placeholder). | +| `server/README.md` | Item catalog section: document `IItemDefinitionRegistry` as preferred lookup surface. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | E3.M3 row — note NEO-52 registry when landed. | + +## Tests + +| Test file | What it covers | +|-----------|----------------| +| `server/NeonSprawl.Server.Tests/Game/Items/ItemDefinitionRegistryTests.cs` | **Unit:** `TryGetDefinition` for `scrap_metal_bulk` (or equip stub) returns true with `StackMax`, `PrototypeRole`, `InventorySlotKind`; null and unknown id return false without throw; `GetDefinitionsInIdOrder` count and ordinal order for multi-row fixture; loader-backed prototype fixture matches catalog. **Host:** `InMemoryWebApplicationFactory` resolves `IItemDefinitionRegistry` and finds `scrap_metal_bulk`. | + +## Open questions / risks + +| Question / risk | Agent recommendation | Status | +|-----------------|----------------------|--------| +| **Catalog vs registry for game code** | New callers inject `IItemDefinitionRegistry`; leave `ItemDefinitionCatalog` eager-resolve in `Program.cs` for fail-fast only. | `adopted` | +| **Duplicate API on catalog** | Keep `ItemDefinitionCatalog.TryGetItem` for loader/tests; registry is the game-facing surface (NEO-35 precedent with catalog `ById`). | `adopted` | + +None blocking beyond the above.