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-66.md

5.7 KiB
Raw Permalink Blame History

Code review — NEO-66 server recipe catalog load (fail-fast)

Date: 2026-05-24
Scope: Branch NEO-66-e3m2-server-recipe-catalog-load-fail-fast · commits ccf4cf8–f209636 vs origin/main
Base: origin/main

Verdict

Approve with nits

Summary

NEO-66 delivers E3M2-02: fail-fast startup load of content/recipes/*_recipes.json under server/NeonSprawl.Server/Game/Crafting/, mirroring the NEO-51 item-catalog pattern. The loader validates Draft 2020-12 schemas (with $ref registration for I/O rows), cross-checks itemId / requiredSkillId against already-loaded item and skill catalogs, enforces the frozen Slice 3 eight-recipe gate (PrototypeSlice3RecipeCatalogRules), registers RecipeDefinitionCatalog in DI, and eagerly resolves it in Program.cs. Fourteen AAA loader/host tests pass; docs (plan, E3M2 backlog, E3.M2 snapshot, alignment register, server/README.md) are updated. Bruno adds a health smoke request for post-boot verification. No client or HTTP recipe API — correctly deferred to NEO-67+. Risk is low: infrastructure-only, CI already gates content; main residual risk is test-factory path pinning consistency across parallel host boots.

Documentation checked

Document Result
docs/plans/NEO-66-implementation-plan.md Matches — loader, catalog, path resolution, Slice 3 rules, DI, Program.cs, tests, doc touches; acceptance checklist complete; IRecipeDefinitionRegistry correctly out of scope.
docs/plans/E3M2-prototype-backlog.md Matches — E3M2-02 checkboxes + landed note.
docs/decomposition/modules/E3_M2_RefinementAndRecipeExecution.md Matches — implementation snapshot references NEO-66 load; freeze table and CI rules unchanged.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E3.M2 row updated with NEO-66 landed note; next backlog item NEO-67.
docs/decomposition/modules/module_dependency_register.md Partially matches — table Status still Planned for E3.M2 (same pattern as post–NEO-65); optional bump when Slice 3 server spine completes.
docs/decomposition/modules/client_server_authority.md N/A — server startup catalog only; no client mutation.
scripts/validate_content.py Matches — C# gate mirrors _validate_recipe_catalogs + _prototype_slice3_recipe_gate; sync comments on PrototypeSlice3RecipeCatalogRules.
server/README.md Matches — Recipe catalog section (config keys, discovery, fail-fast, Slice 3 parity).
Manual QA N/A — plan documents skip rationale: automated loader/host tests + Bruno health smoke (same pattern as NEO-51); no player-visible HTTP.

Register/tracking: alignment table E3.M2 In Progress with NEO-66 note is correct.

Blocking issues

None.

Suggestions

  1. Pin Content:RecipesDirectory in other test factories — InMemoryWebApplicationFactory pins recipes (per plan). Also pin in PostgresWebApplicationFactory, MissionRewardDeniedRegistryWebApplicationFactory, and RefineActivityDeniedRegistryWebApplicationFactory (and any future host factories) so CI never depends on ancestor discovery alone when layout differs. SalvageActivityDeniedRegistryWebApplicationFactory already pins items/resource-nodes; add recipes there too for symmetry. Done. All five host factories now pin Content:RecipesDirectory.

  2. Loader test gaps vs plan test table — Add AAA cases for: empty recipes directory (no *_recipes.json), invalid JSON file, unknown itemId in outputs (inputs covered). Low risk given CI + existing coverage, but closes the plan’s enumerated failure modes. Done. Added Load_ShouldThrow_WhenNoRecipeCatalogFiles, Load_ShouldThrow_WhenJsonIsInvalid, Load_ShouldThrow_WhenUnknownItemIdInOutputs.

  3. Plan Tests section — manual QA skip note — Add one line stating manual QA is skipped because acceptance is fully covered by unit/host tests + Bruno health (per planning-implementation-docs). Done. See NEO-66 plan Tests section.

Nits

  • Nit: SchemaRegistry.Global.Register in RecipeDefinitionCatalogLoader mutates process-global state; acceptable for startup load, but parallel test runs share the registry (same as any future multi-catalog $ref loaders).

  • Nit: ValidPrototypeCatalogJson duplicates content/recipes/prototype_recipes.json inline — fine for isolated temp-dir fixtures; optional future refactor to read/copy repo file.

  • Nit: Full dotnet test suite showed occasional parallel host-boot flakes (unrelated tests pass in isolation); not introduced by recipe tests specifically, but extra eager catalog resolve adds startup work — watch CI if flakes increase.

Verification

cd /home/don/neon-sprawl/server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~RecipeDefinitionCatalogLoaderTests"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

Manual (optional):

  • Start server from repo root; confirm Information log: recipe count 8, catalog directory, file count 1.
  • Bruno: bruno/neon-sprawl-server/recipe-catalog/Health after recipe catalog load.bru → 200 + service: NeonSprawl.Server.
  • Negative: set Content__RecipesDirectory to an empty temp dir → startup fails with Recipe catalog validation failed:.