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-37.md

4.5 KiB
Raw Permalink Blame History

Code review — NEO-37 skill progression snapshot

Date: 2026-05-06 (initial); re-reviewed 2026-05-07.

Scope: Branch NEO-37-skill-progression-snapshot-read-model; git range f89efdce..a3cbef0 (merge-base f89efdce with origin/main through current HEAD a3cbef0). Issue NEO-37.

Base: origin/main @ f89efdce.

Follow-up commits since first review: 295eb34 (decomposition docs + review feedback); a3cbef0 (skills non-contractual ordering, ordinal sort for stable serialization only, tests keyed by id, Bruno + README + plan + manual QA aligned).

Verdict

Approve.

Summary

Adds GET /game/players/{id}/skill-progression with server-authoritative defaults (every registered skill row at xp 0, level 1), gated on IPositionStateStore.TryGetPosition like CooldownSnapshotApi/HotbarLoadoutApi. Response DTOs follow the NEO-36-style versioned envelope (schemaVersion, camelCase JSON names); skills array order is not contractual — clients key by id. Integration-style tests exercise 404, frozen trio membership + defaults (order-independent), and Bruno no longer asserts sort order on the progression array. README, manual QA, and implementation plan document the same contract. Risk is low: read-only projection with no persistence or grant path yet.

Documentation checked

  • docs/plans/NEO-37-implementation-plan.md — matches route, player gate (404), JSON shape, skills unordered-by-contract (+ optional stable ordinal sort server-side only), Bruno/README/manual QA expectations, and deferral of persistence to NEO-38.

  • docs/manual-qa/NEO-37.md — matches curl steps and assertions.

  • docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md — matches story placement and NEO-37 delivery in Implementation snapshot; full engine responsibilities in the module body remain description of the target module (NEO-38+). Updated after review (2026-05-07) so tracking does not drift.

  • docs/decomposition/modules/documentation_and_implementation_alignment.md — matches (E2.M2 row In Progress, NEO-37 landed narrative + follow-on backlog). Updated after review.

  • docs/decomposition/modules/client_server_authority.md, contracts.md — N/A for this additive read-model slice (no new cross-cutting gameplay authority beyond existing “known player via position store” pattern).

Blocking issues

(none)

Suggestions

  1. Documentation follow-up after merge: Update docs/decomposition/modules/E2_M2_XpAwardAndLevelEngine.md (implementation snapshot — remove or qualify “No implementation landed yet”) and confirm documentation_and_implementation_alignment.md E2.M2 row still reads correctly once NEO-37 is merged. Prefer a short PR stacked on this merge or the same sprint so tracking does not drift. Done.

  2. docs/plans/NEO-37-implementation-plan.md typography: Insert a missing space after the comma (“When grants land, prefer …”) under Open questions. Done.

Nits

  • Nit: Bruno skills are ascending by id matches skill-definitions folder style but is weaker than the C# tests for exact catalog order; if regressions swapped registry order without changing ids, lexical sort alone would not catch misuse of a different comparator. Acceptable Bruno scope; parity with sibling collection is coherent. Superseded — progression skills is explicitly unordered by contract; Bruno + C# tests key by id only (2026-05-07).

Re-review notes

Explicit unordered-by-contract semantics for skills are coherent: they differentiate this read snapshot from GET …/skill-definitions, whose catalog order stays pinned for discovery. Server-side string.CompareOrdinal sort before JSON preserves stable payloads without inviting clients to depend on index ordering—documented on the response DTO, README, plan, manual QA, and BuildSnapshot.

Prior review items (module snapshot, alignment row, plan typo, Bruno-vs-order nit) are addressed or superseded; no new blocking findings.

Verification

  • Re-review (2026-05-07):
    cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~SkillProgressionSnapshotApiTests" — passed (2 tests).

  • Recommended before merge: full server test suite:
    cd server && dotnet test

  • Manual: steps in docs/manual-qa/NEO-37.md; optional Bruno bruno/neon-sprawl-server/skill-progression/Get skill progression.bru with {{baseUrl}} from environments/Local.bru.