# Code Review — NEO-39

- **Date:** 2026-05-06
- **Scope:** Working tree / unstaged changes for NEO-39 (`Data-driven skill level curve + CI validation`)
- **Base:** `main` (inferred)

## Verdict

Request changes.

## Summary

The change successfully introduces data-driven skill level thresholds, adds CI validation for `*_skills.json` and `*_level_curve.json`, and wires runtime level resolution through `ISkillLevelCurve`. API behavior for progression GET/POST remains stable and test coverage is healthy overall, with full server test suite passing.  
The main risk is a startup-validation gap: runtime curve loading currently performs custom checks instead of schema evaluation and therefore does not enforce schema constraints such as `additionalProperties: false`. That allows schema-invalid curve content to pass startup, which conflicts with the story’s fail-fast validation intent.

## Documentation checked

- `docs/plans/NEO-39-implementation-plan.md` — **partially matches** (most acceptance criteria are implemented; startup validation is not schema-equivalent to declared contract strictness).
- `docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md` — **matches** (module snapshot updated for NEO-39 landed behavior).
- `docs/decomposition/modules/module_dependency_register.md` — **matches** (E2.M2 remains In Progress; dependencies/contract shape unchanged).
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches** (E2.M2 tracking row still lists NEO-39 as follow-on; should be updated after merge to avoid stale status narrative).

## Blocking issues

1. ~~Runtime level-curve validation is not schema-equivalent, so schema-invalid files can still boot successfully.~~
   - ~~Evidence: `server/NeonSprawl.Server/Game/Skills/ContentBackedSkillLevelCurve.cs` validates only selected fields manually and does not evaluate `content/schemas/level-curve.schema.json` constraints like `additionalProperties: false`.~~
   - ~~Impact: Violates fail-fast startup validation intent for invalid curve content and can permit config drift between CI and runtime.~~
   - ~~Recommendation: Validate the loaded JSON against `level-curve.schema.json` at startup (or implement strict parity checks for all schema constraints, including additional properties and structural limits), then keep existing domain checks (strictly increasing rows, first row = level 1 / xp 0).~~
   Done. Startup load now evaluates `level-curve.schema.json` via `JsonSchema.Net` in `ContentBackedSkillLevelCurve.Load(...)` before domain checks, and `ContentBackedSkillLevelCurveTests.Load_ShouldThrow_WhenCurveHasUnexpectedTopLevelProperty` verifies `additionalProperties: false` enforcement.

## Suggestions

1. ~~Update `docs/decomposition/modules/documentation_and_implementation_alignment.md` E2.M2 snapshot text to reflect that NEO-39 is landed (currently phrased as a follow-on item).~~  
   Done. E2.M2 tracking row now records NEO-39 as landed (data-driven `LevelCurve` + CI + fail-fast startup checks) and links `docs/plans/NEO-39-implementation-plan.md` plus `docs/manual-qa/NEO-39.md`.
2. ~~Refresh stale placeholder wording in `server/NeonSprawl.Server.Tests/Game/Skills/SkillProgressionGrantApiTests.cs` comments (the threshold behavior is now content-backed, not placeholder-based).~~  
   Done. Updated the stale arrange comment in `PostSkillProgression_ShouldReportLevelUps_WhenThresholdCrossed` to reference content-backed thresholds from `prototype_level_curve.json`.

## Nits

- ~~Nit: `SkillDefinitionCatalogLoader` XML summary still says `content/skills/*.json`; implementation now intentionally scopes to `*_skills.json`.~~  
  Done. Updated XML summary in `server/NeonSprawl.Server/Game/Skills/SkillDefinitionCatalogLoader.cs` to `content/skills/*_skills.json`.

## Verification

- `python3 scripts/validate_content.py`
- `dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --no-restore`
- Manual negative check: add an extra top-level property to `content/skills/prototype_level_curve.json` and verify server startup fails (expected once runtime schema validation parity is implemented).