From 42ea2598c389aae695d31158519737127bd85bbb Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 24 May 2026 21:42:44 -0400 Subject: [PATCH] NEO-77: Add code review for E5M1-02 server ability catalog load. --- docs/reviews/2026-05-24-NEO-77.md | 66 +++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 docs/reviews/2026-05-24-NEO-77.md diff --git a/docs/reviews/2026-05-24-NEO-77.md b/docs/reviews/2026-05-24-NEO-77.md new file mode 100644 index 0000000..fc438b1 --- /dev/null +++ b/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().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:`.