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