don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-05-06-NEO-39.md

4.1 KiB
Raw Permalink Blame History

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