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/reviews/2026-05-24-NEO-77.md

5.3 KiB
Raw Permalink Blame History

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 Matches — kickoff decisions, loader/catalog/DI scope, acceptance checklist, reconciliation section; IAbilityDefinitionRegistry correctly out of scope.
docs/plans/E5M1-prototype-backlog.md · E5M1-02 Matches — AC checkboxes + landed note.
docs/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 Matches — E5.M1 row updated with NEO-77 landed note; next backlog item NEO-79.
docs/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 N/A — server startup catalog only; no client mutation or combat resolution.
scripts/validate_content.py Matches — C# loader mirrors _validate_ability_catalogs + _prototype_e5m1_ability_gate; sync comment on PrototypeE5M1AbilityCatalogRules.ExpectedAbilityIds.
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

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:.