47 lines
4.1 KiB
Markdown
47 lines
4.1 KiB
Markdown
# 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<SkillDefinitionCatalog>()` 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.
|