# 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](../../.cursor/rules/csharp-style.md#unit-and-integration-tests-arrange-act-assert), 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](../../.cursor/rules/csharp-style.md) 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.