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-04-NEO-36.md

3.8 KiB
Raw Permalink Blame History

Code review: NEO-36 (read-only skill catalog HTTP)

Date: 2026-05-04

Scope: Branch NEO-36-e2m1-read-only-skill-catalog-http-projection vs merge-base a5f7f1b4aedfb7928440ac2e1fef7035895544a6 (origin/main at review time): GET /game/world/skill-definitions, SkillDefinitionsWorldApi, SkillDefinitionsListDtos, SkillDefinitionsWorldApiTests, Bruno skill-definitions/, docs/plans/NEO-36-implementation-plan.md, docs/manual-qa/NEO-36.md.

Base: origin/main (merge-base above)

Verdict

Approve with nits

Summary

The branch adds a versioned read-only JSON projection backed by ISkillDefinitionRegistry.GetDefinitionsInIdOrder(), wired next to existing world projections in Program.cs, with integration tests mirroring InteractablesWorldApiTests (HTTP 200, ReadFromJsonAsync, schema version, frozen trio order, and spot-checks on allowedXpSourceKinds). Bruno requests assert the same contract. Behavior matches the NEO-36 plan and E2.M1 catalog intent: no mutations, no duplicate source of truth beyond the registry.

Documentation checked

Document Result
docs/plans/NEO-36-implementation-plan.md Matches — route /game/world/skill-definitions, schemaVersion + skills rows with the four fields, registry-backed mapping, tests + Bruno + manual QA as listed.
docs/decomposition/modules/E2_M1_SkillDefinitionRegistry.md Matches — read-only exposure of stable ids, display names, categories, and allowedXpSourceKinds; aligns with “dev tooling / QA” style use; no XP grant enforcement here (E2.M2).
docs/decomposition/modules/module_dependency_register.md Matches — E2.M1 in progress; HTTP projection is consistent with module scope.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E2.M1 row updated for NEO-36 landed (route, paths, README + manual QA pointers).
docs/decomposition/modules/contracts.md N/A (not opened end-to-end) — response uses explicit JsonPropertyName and a schemaVersion const; same pattern as interactables list.

Blocking issues

(none)

Suggestions

  1. docs/manual-qa/NEO-36.md header — planning-implementation-docs asks for a small table with Key, Title, Linear link, Plan link, and Branch on ticketed manual QA files. Add that table so the checklist matches repo convention (same pattern as other docs/manual-qa/NEO-*.md files that follow the rule). Done.

  2. documentation_and_implementation_alignment.md — Extend the E2.M1 row to note NEO-36: GET /game/world/skill-definitions, Game/Skills/SkillDefinitionsWorldApi.cs / DTOs, and pointer to docs/plans/NEO-36-implementation-plan.md (same PR or immediate follow-up per documentation_and_implementation_alignment.md). Done.

  3. server/README.md (optional) — Skill catalog section documents load policy (NEO-34) but not the read projection. A one-line anchor to the GET route would match how NEO-25 interactables are surfaced for operators; only if you want parity with other HTTP surfaces. Done.

Nits

  1. Nit: Bruno test sorts ids with localeCompare; server order is ordinal (GetDefinitionsInIdOrder). For current ASCII snake_case ids they agree; if ids ever used non-ASCII, consider documenting or aligning sort semantics in Bruno. Done. (ordinal sort + comment in Bruno.)

Verification

  • dotnet test NeonSprawl.sln --filter "FullyQualifiedName~SkillDefinitionsWorldApiTests" — passed at review time.
  • Optional: full dotnet test NeonSprawl.sln before merge.
  • Manual: follow docs/manual-qa/NEO-36.md (after adding the header table if you take suggestion 1) and/or run the Bruno request against Local environment.