67 lines
5.3 KiB
Markdown
67 lines
5.3 KiB
Markdown
# Code review — NEO-77 server ability catalog load (fail-fast)
|
||
|
||
**Date:** 2026-05-24
|
||
**Scope:** Branch `NEO-77-e5m1-server-ability-catalog-load` · commits `07444e4`–`7c2697e` vs `origin/main`
|
||
**Base:** `origin/main`
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
NEO-77 delivers **E5M1-02**: fail-fast startup load of `content/abilities/*_abilities.json` under `server/NeonSprawl.Server/Game/Combat/`, mirroring the NEO-51 / NEO-66 catalog pattern. The loader validates Draft 2020-12 row schema, `schemaVersion`, duplicate `id`, and the frozen E5M1 four-id gate (`PrototypeE5M1AbilityCatalogRules`), registers `AbilityDefinitionCatalog` in DI, and eagerly resolves it in `Program.cs`. Fourteen AAA loader/host tests pass; full suite (354 tests) is green. Docs (plan, E5M1 backlog, E5.M1 module page, alignment register, `server/README.md`) are updated. `PrototypeAbilityRegistry` is correctly unchanged (NEO-79). Bruno adds a health smoke request consistent with other catalog stories. No client or ability HTTP API — appropriately deferred. Risk is low: infrastructure-only, CI already gates content.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| [`docs/plans/NEO-77-implementation-plan.md`](../plans/NEO-77-implementation-plan.md) | **Matches** — kickoff decisions, loader/catalog/DI scope, acceptance checklist, reconciliation section; `IAbilityDefinitionRegistry` correctly out of scope. |
|
||
| [`docs/plans/E5M1-prototype-backlog.md`](../plans/E5M1-prototype-backlog.md) · **E5M1-02** | **Matches** — AC checkboxes + landed note. |
|
||
| [`docs/decomposition/modules/E5_M1_CombatRulesEngine.md`](../decomposition/modules/E5_M1_CombatRulesEngine.md) | **Matches** — Status line and freeze rules reference NEO-77 server load + NEO-79 registry deferral. |
|
||
| [`docs/decomposition/modules/documentation_and_implementation_alignment.md`](../decomposition/modules/documentation_and_implementation_alignment.md) | **Matches** — E5.M1 row updated with NEO-77 landed note; next backlog item NEO-79. |
|
||
| [`docs/decomposition/modules/module_dependency_register.md`](../decomposition/modules/module_dependency_register.md) | **Matches** — E5.M1 table **Status** **In Progress** is correct; **E5.M1 note** cites NEO-76 + NEO-77 server load. |
|
||
| [`docs/decomposition/modules/client_server_authority.md`](../decomposition/modules/client_server_authority.md) | **N/A** — server startup catalog only; no client mutation or combat resolution. |
|
||
| [`scripts/validate_content.py`](../../scripts/validate_content.py) | **Matches** — C# loader mirrors `_validate_ability_catalogs` + `_prototype_e5m1_ability_gate`; sync comment on `PrototypeE5M1AbilityCatalogRules.ExpectedAbilityIds`. |
|
||
| [`server/README.md`](../../server/README.md) | **Matches** — Ability catalog section (config keys, discovery, fail-fast, E5M1 parity). |
|
||
| Full-stack epic decomposition | **Matches** — E5M1-02 is **server/content** only; client counterparts documented in backlog (NEO-85 / NEO-86); no false “prototype complete” claim. |
|
||
| Manual QA | **N/A** — plan documents skip rationale: automated loader/host tests + Bruno health smoke. |
|
||
|
||
Register/tracking: alignment table E5.M1 **In Progress** with NEO-77 note is correct.
|
||
|
||
## Blocking issues
|
||
|
||
None.
|
||
|
||
## Suggestions
|
||
|
||
1. ~~**Add `folder.bru` under `bruno/neon-sprawl-server/ability-catalog/`** — Other catalog smoke folders (`item-catalog`, `recipe-catalog`, `resource-node-catalog`, `mastery-catalog`) include a `folder.bru` meta file; ability-catalog currently has only the health request. Add for Bruno collection parity.~~ **Done.** Added `bruno/neon-sprawl-server/ability-catalog/folder.bru`.
|
||
|
||
2. ~~**E5M1 gate test — extra/wrong id** — `Load_ShouldThrow_WhenE5M1GateFails` removes one ability (missing id). Plan test table also lists wrong roster; consider an AAA case that swaps in an unknown id (e.g. `prototype_extra`) to cover the “got […]” branch, not only “missing”.~~ **Done.** Renamed missing-id test to `Load_ShouldThrow_WhenE5M1GateFailsWithMissingId`; added `Load_ShouldThrow_WhenE5M1GateFailsWithExtraId`. Plan test table updated.
|
||
|
||
## Nits
|
||
|
||
- Nit: `ValidPrototypeCatalogJson` duplicates `content/abilities/prototype_abilities.json` inline — fine for isolated temp-dir fixtures (same pattern as NEO-66 recipe tests); optional future refactor to copy repo file.
|
||
|
||
- Nit: E5M1 gate error string format differs slightly from Python (`'prototype_pulse', …` vs `sorted(...)!r`); behavior is equivalent and messages are actionable.
|
||
|
||
- Nit: Repeated `services.AddOptions<ContentPathsOptions>().Bind(...)` in each catalog extension — established repo pattern (NEO-51/NEO-66); harmless duplicate binds.
|
||
|
||
## Verification
|
||
|
||
```bash
|
||
cd /home/don/neon-sprawl/server
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~AbilityDefinitionCatalogLoaderTests"
|
||
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj
|
||
|
||
# Content CI parity (repo root)
|
||
python3 scripts/validate_content.py
|
||
# Expect: 4 unique ability id(s)
|
||
```
|
||
|
||
Manual (optional):
|
||
|
||
- Start server from repo root; confirm Information log: ability count **4**, catalog directory, file count **1**.
|
||
- Bruno: `bruno/neon-sprawl-server/ability-catalog/Health after ability catalog load.bru` → 200 + `service: NeonSprawl.Server`.
|
||
- Negative: set `Content__AbilitiesDirectory` to an empty temp dir → startup fails with `Ability catalog validation failed:`.
|