From 8bafec9d79af40634b0a8ef0d1fa70115821b054 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 22:03:12 -0400 Subject: [PATCH] NEO-79: Add kickoff implementation plan for ability registry + DI. --- docs/plans/NEO-79-implementation-plan.md | 120 +++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 docs/plans/NEO-79-implementation-plan.md diff --git a/docs/plans/NEO-79-implementation-plan.md b/docs/plans/NEO-79-implementation-plan.md new file mode 100644 index 0000000..ddde48e --- /dev/null +++ b/docs/plans/NEO-79-implementation-plan.md @@ -0,0 +1,120 @@ +# 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 + +- [ ] Unknown ability id fails hotbar/cast validation same as today (`unknown_ability`, no behavior regression). +- [ ] All four prototype ids resolve with expected **`baseDamage`** / **`cooldownSeconds`** via `TryGetDefinition`. +- [ ] DI resolves `IAbilityDefinitionRegistry` at host startup (test via `InMemoryWebApplicationFactory`). +- [ ] 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 Bruno or manual QA for this story (server-only DI + internal migration; HTTP proof is NEO-78). + +## 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** |