neon-sprawl/docs/reviews/2026-05-04-NEO-35.md

Code review: NEO-35 (SkillDefinitionRegistry)

Date: 2026-05-04

Scope: Branch NEO-35-e2m1-skilldefinition-registry vs origin/main (ahead 2 commits): ISkillDefinitionRegistry, SkillDefinitionRegistry, DI registration, SkillDefinitionRegistryTests, plan docs/plans/NEO-35-implementation-plan.md, comment tweak on SkillDefinitionCatalog.

Base: origin/main

Verdict

Approve with nits

Summary

The change adds a thin registry adapter over SkillDefinitionCatalog with TryGetDefinition, ordered enumeration, XML guidance for E2.M2 and NEO-36, and singleton DI registration. Unit tests cover happy path, unknown id, id-ordered listing, loader-backed consistency, and host DI resolution. Behavior matches the NEO-35 plan and E2.M1 module intent for this slice (read-only metadata; grant enforcement remains E2.M2). dotnet test on NeonSprawl.Server.Tests passes (110 tests).

Documentation checked

Document	Result
docs/plans/NEO-35-implementation-plan.md	Matches — interface + adapter + DI + tests align with acceptance checklist and technical approach (TryGet* semantics, SkillDefRow, enumeration, AAA tests).
docs/decomposition/modules/E2_M1_SkillDefinitionRegistry.md	Matches — exposes stable ids, categories, display names, allowedXpSourceKinds; documents E2.M2 validation expectation on the interface; does not implement unlock gating or XP engine (correctly out of scope).
docs/decomposition/modules/module_dependency_register.md	Matches — E2.M1 row describes registry / allowedXpSourceKinds; implementation fits.
docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md (cross-read)	Matches — interface remarks align with “reject grants when sourceKind not listed.”
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches — E2.M1 snapshot row updated for NEO-35 landed.

Blocking issues

(none)

Suggestions

1. Host_ShouldResolveRegistryFromDi_WhenStartupSucceeds AAA (SkillDefinitionRegistryTests.cs) — // Act currently performs GetAsync("/health"), GetRequiredService<ISkillDefinitionRegistry>(), and TryGetDefinition together. Per csharp-style, Act should be the behavior under test only (typically one call, or a short intentional sequence for one scenario). Prefer moving /health to Arrange as a startup warm-up if ordering matters, dropping it if CreateClient() already builds the host enough for the catalog, or splitting host-health vs DI-resolution into separate tests so each method has a single clear Act. Done.

2. Post-merge doc hygiene — Update documentation_and_implementation_alignment.md E2.M1 / NEO-35 line when this story merges so the implementation tracking table matches reality. Done.

Nits

1. Nit: finally block uses an empty catch { } around Directory.Delete in TryGetDefinition_ShouldMatchLoaderCatalog_WhenUsingPrototypeFixture. csharp-style prefers avoiding empty catches; for tests, catching a specific exception type (or a tiny helper) is enough if you want to silence flaky cleanup on Windows/locked files. Done.

2. Nit: TryGetDefinition with a null skillId will throw from the underlying dictionary lookup; the XML says misses do not throw. That is fine for normal string ids; if you want the contract to include null as “not found,” add an explicit guard—only worth it if public API should treat null like unknown. Done.

Verification

• dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj — all tests passed at review time.