NEO-77: Add code review for E5M1-02 server ability catalog load.

docs/reviews/2026-05-24-NEO-77.md

@@ -0,0 +1,66 @@
+# 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) | **Partially matches** — E5.M1 table **Status** **In Progress** is correct; **E5.M1 note** still cites NEO-76 only (optional bump to mention NEO-77 server load, same pattern as post–NEO-66 E3.M2 note lag). |
+| [`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.
+
+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”.
+
+## 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:`.