don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/plans/NEO-79-implementation-plan.md

128 lines
12 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters!

This file contains ambiguous Unicode characters that may be confused with others in your current locale. If your use case is intentional and legitimate, you can safely ignore this warning. Use the Escape button to highlight these characters.

# 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).
Reference in New Issue View Git Blame Copy Permalink