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-02-NEO-34.md

3.6 KiB
Raw Blame History

Code review — NEO-34 (skill catalog startup)

Date: 2026-05-02

Scope: Branch NEO-34-e2m1-skill-catalog-startup vs origin/main (commits through 1a9088c); working tree review.

Base: origin/main

Verdict

Approve with nits

Summary

The branch adds fail-fast loading of content/skills/*.json at server startup: JSON Schema validation (JsonSchema.Net), duplicate id detection, and a C# mirror of the NEO-33 prototype Slice 1 gate aligned with scripts/validate_content.py. SkillDefinitionCatalog is registered as a singleton and eagerly resolved in Program.cs so a bad catalog prevents the host from reaching a listening state. Unit tests cover the loader; an integration-style test uses InMemoryWebApplicationFactory and a WebApplicationFactory negative case for an empty skills directory. server/README.md documents Content:SkillsDirectory and schema resolution. All 105 tests in NeonSprawl.sln passed locally (dotnet test NeonSprawl.sln).

Documentation checked

Document Result Notes
docs/plans/NEO-34-implementation-plan.md Matches Acceptance criteria and technical approach match implementation; appsettings.json includes optional Content keys; Tests table documents merged host coverage in SkillDefinitionCatalogLoaderTests.cs. Done.
docs/decomposition/modules/E2_M1_SkillDefinitionRegistry.md Matches Load-time validation against skill-def.schema.json, duplicate id, and Slice 1 roster rules align with module intent.
docs/decomposition/modules/CT_M1_ContentValidationPipeline.md Matches Server-side validation complements CI validate_content.py; same schema and gate semantics.
docs/decomposition/modules/module_dependency_register.md Matches E2.M1 remains an appropriate home; no contract contradiction.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches E2.M1 tracking row updated for NEO-34 landed + pointers. Done.
docs/manual-qa/NEO-34.md Matches Standard header table and numbered sections aligned with NEO-31 style. Done.

Blocking issues

(none)

Suggestions

  1. After merge (or in this PR): update documentation_and_implementation_alignment.md E2.M1 snapshot to include NEO-34 server load and pointers to Game/Skills/ + README, so the tracking table matches reality. Done.
  2. Either add a short commented Content subsection to server/NeonSprawl.Server/appsettings.json as the plan described, or remove/adjust that row in NEO-34-implementation-plan.md so the plan matches what shipped. Done (placeholder Content keys in appsettings.json + plan row aligned).
  3. Expand docs/manual-qa/NEO-34.md with the standard header table (Key, Title, Linear, Plan, Branch) and numbered sections for consistency with other story QA files. Done.

Nits

  • bruno/neon-sprawl-server/core/Health.bru: Added Bruno tests blocks for status and service identity. Useful, but not called out in the NEO-34 plan; consider mentioning in the PR description or splitting if you want a strictly single-concern PR. (Unchanged — optional PR hygiene.)
  • SkillDefinitionCatalogLoaderTests.cs: Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds puts GetRequiredService<SkillDefinitionCatalog>() under Act alongside GetAsync("/health"); strict AAA would treat DI resolution as Assert only (style only). Done.

Verification

  • dotnet test NeonSprawl.sln (from repo root) — expect all tests green.
  • Manual: follow docs/manual-qa/NEO-34.md after any edits to that file.