From 922e81eb0f141afceb5169cf381da3c77550e302 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 2 May 2026 19:47:45 -0400 Subject: [PATCH] NEO-34: add code review for skill catalog startup branch --- docs/reviews/2026-05-02-NEO-34.md | 46 +++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 docs/reviews/2026-05-02-NEO-34.md diff --git a/docs/reviews/2026-05-02-NEO-34.md b/docs/reviews/2026-05-02-NEO-34.md new file mode 100644 index 0000000..cb18810 --- /dev/null +++ b/docs/reviews/2026-05-02-NEO-34.md @@ -0,0 +1,46 @@ +# 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` | Partially matches | Acceptance criteria and technical approach are largely implemented. The plan’s **Files to modify** still lists `appsettings.json` for an optional `Content` section; the diff does **not** change `appsettings.json` (README documents config instead—either add a commented `Content` block or amend the plan). The plan’s **Tests** table names `SkillCatalogStartupIntegrationTests.cs`; implementation merged host tests into `SkillDefinitionCatalogLoaderTests.cs`, which the plan explicitly allows as an alternative. | +| `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` | Partially matches | The **Implementation tracking table** E2.M1 row still describes NEO-34 server load as **follow-on** only. After merge, that row should be updated to record **NEO-34 landed** (paths: `server/NeonSprawl.Server/Game/Skills/`, README section) per the doc’s rule on updating the table when slices land. | +| `docs/manual-qa/NEO-34.md` | Partially matches | Checklist content is useful, but it does not follow the project’s usual manual QA header (**Key / Title / Linear / Plan / Branch** table) described in `planning-implementation-docs.md` and exemplified by e.g. `docs/manual-qa/NEO-31.md`. | + +## 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. +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. +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. + +## 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. +- **`SkillDefinitionCatalogLoaderTests.cs`**: `Host_ShouldResolveCatalogFromDi_WhenStartupSucceeds` puts `GetRequiredService()` under **Act** alongside `GetAsync("/health")`; strict AAA would treat DI resolution as **Assert** only (style only). + +## 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.