# NEO-79 — Implementation plan

## Story reference

| Field | Value |
|--------|--------|
| **Key** | NEO-79 |
| **Title** | E5M1-03: `IAbilityDefinitionRegistry` + DI |
| **Linear** | https://linear.app/neon-sprawl/issue/NEO-79/e5m1-03-iabilitydefinitionregistry-di |
| **Module** | [E5.M1 — CombatRulesEngine](../decomposition/modules/E5_M1_CombatRulesEngine.md) · Epic 5 Slice 1 · backlog **E5M1-03** |
| **Branch** | `NEO-79-iabilitydefinitionregistry-di` |
| **Precursor** | [NEO-77](https://linear.app/neon-sprawl/issue/NEO-77) — fail-fast `AbilityDefinitionCatalog` load (**Done** on `main`) |
| **Pattern** | [NEO-67](https://linear.app/neon-sprawl/issue/NEO-67) / [NEO-52](https://linear.app/neon-sprawl/issue/NEO-52) — thin registry adapter over startup catalog + DI; strict split after NEO-77 loader |
| **Blocks** | [NEO-78](https://linear.app/neon-sprawl/issue/NEO-78) — GET world ability-definitions; [NEO-80+](E5M1-prototype-backlog.md) — combat engine consumers |
| **Client counterpart** | None (server-only); optional client enrichment in [NEO-85](https://linear.app/neon-sprawl/issue/NEO-85) via NEO-78 HTTP |

## Kickoff clarifications

| Topic | Question | Agent recommendation | Answer |
|--------|----------|----------------------|--------|
| **Enumeration API** | Include `GetDefinitionsInIdOrder()` now? | **Yes** — mirror `IRecipeDefinitionRegistry`; [NEO-78](https://linear.app/neon-sprawl/issue/NEO-78) `GET /game/world/ability-definitions` needs ordered defs without reaching into `AbilityDefinitionCatalog`. | **User:** include enumeration. |
| **Lookup naming** | `TryGetDefinition` vs `TryGetAbility`? | **`TryGetDefinition(string? abilityId, …)`** — same naming as item/skill/recipe registries; catalog keeps `TryGetAbility` for direct catalog access. | **User:** `TryGetDefinition`. |
| **Hotbar/cast migration** | How replace `PrototypeAbilityRegistry.TryNormalizeKnown`? | **`TryNormalizeKnown` on `IAbilityDefinitionRegistry`** — trim + lowercase + catalog lookup; inject into hotbar/cast APIs. Preserves today’s deny behavior (`unknown_ability`) without duplicating normalize in two endpoints. | **User:** registry `TryNormalizeKnown`. |
| **`PrototypeAbilityRegistry`** | Keep static allowlist or slim down? | **Keep id constant strings only** — remove `HashSet` allowlist and `TryNormalizeKnown`; tests/fixtures keep `PrototypePulse` etc. | **User:** constants only. |
| **Program.cs eager resolve** | Eager-resolve `IAbilityDefinitionRegistry` at boot? | **Omit** — `AbilityDefinitionCatalog` is already eager-resolved in `Program.cs` (NEO-77); registry is a thin adapter (NEO-67 default). | **Adopted** — NEO-67 precedent; no separate question. |
| **Runtime validation** | Re-validate abilities in registry? | **No** — catalog load is fail-fast (NEO-77); registry delegates to loaded rows only. | **Adopted** — NEO-67 precedent. |

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

**Goal:** Provide **`IAbilityDefinitionRegistry`** backed by the startup-loaded **`AbilityDefinitionCatalog`**: resolve by stable **`abilityId`**, enumerate definitions in id order, and expose **`TryNormalizeKnown`** for hotbar/cast allowlist validation. Register in DI and migrate **`HotbarLoadoutApi`** / **`AbilityCastApi`** off the static allowlist so unknown ids deny exactly as today while known ids resolve **`baseDamage`** / **`cooldownSeconds`** from content.

**In scope (from Linear + [E5M1-03](E5M1-prototype-backlog.md#e5m1-03--iabilitydefinitionregistry--di)):**

- `IAbilityDefinitionRegistry` + `AbilityDefinitionRegistry` thin adapter over `AbilityDefinitionCatalog`.
- DI registration in `AddAbilityDefinitionCatalog`.
- Unit tests (AAA): known-id lookup with damage + cooldown; unknown-id miss; `TryNormalizeKnown` trim/case behavior; enumeration order; host resolves registry from DI.
- Migrate hotbar/cast call sites from `PrototypeAbilityRegistry.TryNormalizeKnown` to injected registry.
- Slim `PrototypeAbilityRegistry` to id constants only.

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

- HTTP GET route ([NEO-78](https://linear.app/neon-sprawl/issue/NEO-78)).
- Damage application, combat engine, cooldown commit from catalog ([NEO-80+](E5M1-prototype-backlog.md)).
- Changing loader, E5M1 gate, or catalog load semantics (NEO-77).
- Godot / client changes.

## Acceptance criteria checklist

- [x] Unknown ability id fails hotbar/cast validation same as today (`unknown_ability`, no behavior regression).
- [x] All four prototype ids resolve with expected **`baseDamage`** / **`cooldownSeconds`** via `TryGetDefinition`.
- [x] DI resolves `IAbilityDefinitionRegistry` at host startup (test via `InMemoryWebApplicationFactory`).
- [x] Unit tests (AAA): lookup for known prototype ids; unknown id returns false without throwing; `TryNormalizeKnown` accepts trimmed/case-variant input for catalog ids; `GetDefinitionsInIdOrder` returns four rows ordered by `id`.

## Technical approach

1. **`IAbilityDefinitionRegistry`** — mirror [`IRecipeDefinitionRegistry`](../../server/NeonSprawl.Server/Game/Crafting/IRecipeDefinitionRegistry.cs):
   - `TryGetDefinition(string? abilityId, [NotNullWhen(true)] out AbilityDefRow? definition)` — null and unknown ids return false without throwing; exact ordinal match on catalog keys.
   - `TryNormalizeKnown(string rawAbilityId, [NotNullWhen(true)] out string normalized)` — trim, lowercase, empty/whitespace → false; success when normalized id exists in catalog (replaces static allowlist semantics).
   - `GetDefinitionsInIdOrder()` — all rows ordered by `AbilityDefRow.Id` (ordinal).
   - XML remarks: combat engine ([NEO-81+](../../docs/plans/E5M1-prototype-backlog.md)) and HTTP read model ([NEO-78](https://linear.app/neon-sprawl/issue/NEO-78)) should depend on this interface, not `AbilityDefinitionCatalog`.

2. **`AbilityDefinitionRegistry`** — primary-constructor adapter over `AbilityDefinitionCatalog` (same pattern as [`RecipeDefinitionRegistry`](../../server/NeonSprawl.Server/Game/Crafting/RecipeDefinitionRegistry.cs)); precompute ordered list at construction.

3. **DI** — extend [`AddAbilityDefinitionCatalog`](../../server/NeonSprawl.Server/Game/Combat/AbilityCatalogServiceCollectionExtensions.cs) to register `IAbilityDefinitionRegistry` → `AbilityDefinitionRegistry` after catalog singleton. No change to `Program.cs` eager-resolve (catalog only).

4. **Hotbar/cast migration** — inject `IAbilityDefinitionRegistry` into:
   - [`HotbarLoadoutApi`](../../server/NeonSprawl.Server/Game/AbilityInput/HotbarLoadoutApi.cs) POST handler — replace `PrototypeAbilityRegistry.TryNormalizeKnown` with `registry.TryNormalizeKnown`.
   - [`AbilityCastApi`](../../server/NeonSprawl.Server/Game/AbilityInput/AbilityCastApi.cs) POST handler — same replacement for request `abilityId` validation.

5. **`PrototypeAbilityRegistry`** — retain only public const id strings (`PrototypePulse`, etc.); remove static `HashSet` and `TryNormalizeKnown`. Update class summary to point at `IAbilityDefinitionRegistry`.

6. **Docs** — update [`server/README.md`](../../server/README.md) ability-catalog section (registry is preferred lookup; hotbar/cast migrated). Optional one-line E5M1 backlog note when implementation lands.

7. **Tests** — new `AbilityDefinitionRegistryTests.cs` mirroring `RecipeDefinitionRegistryTests`: in-memory catalog helper, loader-backed prototype fixture via `AbilityCatalogTestPaths` + repo `prototype_abilities.json`, host DI test asserting four prototype rows with expected damage/cooldown from [E5.M1 freeze box](../decomposition/modules/E5_M1_CombatRulesEngine.md). Existing hotbar/cast API tests should remain green without changing expected deny/accept behavior.

### Expected prototype lookup values (from content)

| `id` | `baseDamage` | `cooldownSeconds` |
|------|--------------|-------------------|
| `prototype_pulse` | 25 | 3.0 |
| `prototype_guard` | 0 | 6.0 |
| `prototype_dash` | 0 | 4.0 |
| `prototype_burst` | 40 | 5.0 |

## Files to add

| Path | Purpose |
|------|---------|
| `server/NeonSprawl.Server/Game/Combat/IAbilityDefinitionRegistry.cs` | Public contract: lookup, normalize, enumerate; remarks for NEO-78/81+ callers. |
| `server/NeonSprawl.Server/Game/Combat/AbilityDefinitionRegistry.cs` | Singleton adapter over `AbilityDefinitionCatalog`. |
| `server/NeonSprawl.Server.Tests/Game/Combat/AbilityDefinitionRegistryTests.cs` | AAA unit + host DI tests (mirror `RecipeDefinitionRegistryTests`). |
| `docs/plans/NEO-79-implementation-plan.md` | This plan. |

## Files to modify

| Path | Rationale |
|------|-----------|
| `server/NeonSprawl.Server/Game/Combat/AbilityCatalogServiceCollectionExtensions.cs` | Register `IAbilityDefinitionRegistry` → `AbilityDefinitionRegistry` after catalog singleton. |
| `server/NeonSprawl.Server/Game/Combat/AbilityDefinitionCatalog.cs` | Tighten summary to reference `IAbilityDefinitionRegistry` (replace NEO-79 placeholder). |
| `server/NeonSprawl.Server/Game/AbilityInput/HotbarLoadoutApi.cs` | Inject registry; migrate allowlist validation to `TryNormalizeKnown`. |
| `server/NeonSprawl.Server/Game/AbilityInput/AbilityCastApi.cs` | Inject registry; migrate request ability validation to `TryNormalizeKnown`. |
| `server/NeonSprawl.Server/Game/AbilityInput/PrototypeAbilityRegistry.cs` | Remove static allowlist/`TryNormalizeKnown`; keep id const strings for tests. |
| `server/README.md` | Document `IAbilityDefinitionRegistry` as preferred lookup; note hotbar/cast migration. |

## Tests

| File | Coverage |
|------|----------|
| `server/NeonSprawl.Server.Tests/Game/Combat/AbilityDefinitionRegistryTests.cs` | **Unit:** `TryGetDefinition` for `prototype_pulse` / `prototype_burst` with expected `BaseDamage` + `CooldownSeconds`; unknown id + null → false; `TryNormalizeKnown` with leading/trailing space and mixed case; empty/whitespace → false; `GetDefinitionsInIdOrder` count 4 + ordinal id order. **Loader fixture:** copy repo `prototype_abilities.json` via `AbilityCatalogTestPaths`, load catalog, assert all four freeze-box values. **Host:** `InMemoryWebApplicationFactory` resolves `IAbilityDefinitionRegistry` from DI; lookup + enumeration smoke. |
| Existing `HotbarLoadoutApiTests`, `AbilityCastApiTests`, `HotbarLoadoutPersistenceIntegrationTests`, `CooldownSnapshotApiTests` | **Regression:** no test changes expected if behavior preserved; run full AbilityInput test suite after migration. |

No manual QA for this story (server-only DI + internal migration; HTTP proof is NEO-78). **Optional Bruno deny smokes added:** `hotbar-loadout/Post loadout unknown ability deny.bru`, `ability-cast/Post cast unknown ability deny.bru` — HTTP regression for `unknown_ability` after catalog-backed validation.

## Open questions / risks

| Question / risk | Agent recommendation | Status |
|-----------------|---------------------|--------|
| **Case-only mismatch** — client sends id not in catalog after normalize | **Deny `unknown_ability`** — same as today; catalog ids are lowercase frozen strings. | **adopted** |
| **Dual lookup surfaces** — `TryGetDefinition` vs `TryNormalizeKnown` | **Keep both** — hotbar/cast need normalize-only; combat/HTTP need row lookup without re-implementing trim rules. | **adopted** |
| **Test const indirection** | **Keep `PrototypeAbilityRegistry` const strings** in existing tests — no mass string-literal churn. | **adopted** |

## Reconciliation (implementation)

- **`IAbilityDefinitionRegistry`** + **`AbilityDefinitionRegistry`** registered in **`AddAbilityDefinitionCatalog`**; catalog-only eager resolve unchanged in **`Program.cs`**.
- **`HotbarLoadoutApi`** / **`AbilityCastApi`** inject registry and call **`TryNormalizeKnown`**; **`PrototypeAbilityRegistry`** reduced to id constants.
- **10** new AAA tests in **`AbilityDefinitionRegistryTests`**; **31** existing AbilityInput tests unchanged and green (41 total in filtered run).
- **Bruno:** optional `unknown_ability` deny smokes under `hotbar-loadout/` and `ability-cast/` (see Tests section).