# NEO-35 — Implementation plan

## Story reference

| Field | Value |
|--------|--------|
| **Key** | NEO-35 |
| **Title** | E2.M1: SkillDefinitionRegistry service + lookup tests |
| **Linear** | https://linear.app/neon-sprawl/issue/NEO-35/e2m1-skilldefinitionregistry-service-lookup-tests |

## Kickoff clarifications

| Topic | Question | Answer |
|--------|------------|--------|
| Scope / sequencing | Any open decisions before planning? | **None.** Linear description, [E2.M1 — SkillDefinitionRegistry](../decomposition/modules/E2_M1_SkillDefinitionRegistry.md), and shipped **NEO-34** (`SkillDefinitionCatalog`, `SkillDefRow`, loader, DI) fix scope, acceptance, and out-of-scope items. **NEO-34** is **Done**, so the dependency chain is satisfied for implementation. |
| Kickoff continuation | User asked to continue kickoff after board/relationship clarification | Proceed with this plan on branch `NEO-35-e2m1-skilldefinition-registry`; no additional product questions pending. |

## Goal, scope, and out-of-scope

**Goal:** Provide a small **`ISkillDefinitionRegistry`** (name as in Linear; implementation class name may be `SkillDefinitionRegistry`) **backed by** the startup-loaded **`SkillDefinitionCatalog`**: resolve by stable **`id`**, enumerate definitions, read **`category`**, **`displayName`**, and **`allowedXpSourceKinds`**. Surface is **registered in DI** so **NEO-36** (HTTP/read model) and **E2.M2** callers can depend on the interface. Document on the interface (and briefly in code where helpful) how **E2.M2** will use this for **grant validation** (`allowedXpSourceKinds` vs `XpGrantEvent.sourceKind`).

**In scope (from Linear):**

- C# types aligned with [`content/schemas/skill-def.schema.json`](../../content/schemas/skill-def.schema.json) for **prototype fields** (reuse or thinly wrap existing **`SkillDefRow`** from NEO-34 — avoid duplicating divergent models).
- Unit tests: known prototype ids resolve; **unknown id** returns absent / false **without throwing**; assert at least one **`allowedXpSourceKinds`** expectation on a known skill.
- XML / remarks on the public contract for **E2.M2** and future callers.

**Out of scope (from Linear):**

- **`UnlockRequirement`** gating logic (future slice; schema has no such field today).
- Wiring into the XP engine (**E2.M2**).

## Acceptance criteria checklist

- [x] Tests: **happy path** — known prototype `id` (e.g. from existing test fixtures / `salvage` trio) resolves with expected metadata.
- [x] Tests: **unknown id** — lookup indicates absence / `Try*` returns false **without throwing**.
- [x] Tests: **`allowedXpSourceKinds`** — at least one assertion on a known skill’s list (per E2.M1 contract).
- [x] **`ISkillDefinitionRegistry`** (or chosen public interface name) is **registered in DI** alongside the catalog so **NEO-36** can inject it without reaching for `SkillDefinitionCatalog` internals.

## Technical approach

1. **Interface:** `ISkillDefinitionRegistry` with:
   - **`TryGet`** (or equivalent) for **id → definition** with non-throwing miss semantics.
   - **Enumeration** of all definitions (e.g. `IEnumerable<SkillDefRow>` or read-only collection) for projections.
   - Document that **E2.M2** must reject XP grants when `sourceKind` ∉ `AllowedXpSourceKinds` for the target skill (behavior deferred to E2.M2; this story only exposes data).

2. **Implementation:** `SkillDefinitionRegistry` as a **thin adapter** over **`SkillDefinitionCatalog`** (constructor-injected singleton dependency on `SkillDefinitionCatalog`).

3. **DI:** Extend **`AddSkillDefinitionCatalog`** (or adjacent extension in the same file) to register **`ISkillDefinitionRegistry`** as singleton after / with the catalog registration. No requirement to eagerly-resolve the registry in **`Program.cs`** if **`SkillDefinitionCatalog`** is already resolved (NEO-34); optional `_ = app.Services.GetRequiredService<ISkillDefinitionRegistry>()` only if we want symmetry — default **omit** unless a test or ordering reason appears.

4. **Models:** Prefer exposing **`SkillDefRow`** on the interface (already matches schema fields). If a separate public DTO is needed for naming only, keep it a **record with identical fields** and map in one line — avoid a second parallel shape long-term.

## Files to add

| Path | Purpose |
|------|---------|
| `server/NeonSprawl.Server/Game/Skills/ISkillDefinitionRegistry.cs` | Public contract: lookup, enumerate, XML for E2.M2 / NEO-36. |
| `server/NeonSprawl.Server/Game/Skills/SkillDefinitionRegistry.cs` | Singleton adapter over `SkillDefinitionCatalog`. |

## Files to modify

| Path | Rationale |
|------|-----------|
| `server/NeonSprawl.Server/Game/Skills/SkillCatalogServiceCollectionExtensions.cs` | Register `ISkillDefinitionRegistry` → `SkillDefinitionRegistry` after catalog registration. |
| `server/NeonSprawl.Server/Game/Skills/SkillDefinitionCatalog.cs` | Optional: tighten summary comment if it still says “NEO-35 may wrap” once registry exists. |

## Tests

| Test file | What it covers |
|-----------|------------------|
| `server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionRegistryTests.cs` | **Unit:** build a **`SkillDefinitionCatalog`** in-memory (same constructor as production) with a minimal `ById` dictionary **or** load via **`SkillDefinitionCatalogLoader`** using the existing temp-layout helper pattern from **`SkillDefinitionCatalogLoaderTests`**; assert **`TryGet`** for `salvage` (or trio) returns true and **`AllowedXpSourceKinds`** contains expected values; assert unknown id returns false and does not throw; assert enumeration count matches catalog. **AAA** labels per [csharp-style](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert). |

Optional (only if valuable for regression): one assertion in **`SkillDefinitionCatalogLoaderTests`** host test that **`ISkillDefinitionRegistry`** resolves from **`InMemoryWebApplicationFactory`** — not strictly required if unit tests + extension registration are sufficient.

## Open questions / risks

**None.** If **`SkillDefRow`** is later replaced by enums for category / source kinds, the registry interface can evolve in a follow-up issue without blocking NEO-36 string-based JSON projection.
