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-23-NEO-53.md

4.4 KiB
Raw Blame History

Code review — NEO-53 GET world item-definitions + Bruno

Date: 2026-05-23
Scope: Branch NEO-53-get-world-item-definitions-bruno · commits 910bd48–08428c4 vs origin/main
Base: origin/main

Verdict

Approve with nits

Summary

NEO-53 adds GET /game/world/item-definitions: a versioned read-only JSON projection (schemaVersion 1, items) backed by IItemDefinitionRegistry, mirroring SkillDefinitionsWorldApi (NEO-36). Implementation is thin (ItemDefinitionsWorldApi, DTOs, Program.cs wiring), optional catalog fields are correctly omitted from the contract, and integration tests assert the frozen six ids in ordinal order plus row spot-checks. Bruno, manual QA, README, and E3.M3 alignment docs are in place. Risk is low: no auth change, no second catalog source, no inventory mutation.

Documentation checked

Document Result
docs/plans/NEO-53-implementation-plan.md Matches — route, five required row fields, registry-only injection, frozen six order, Bruno/manual QA/README, acceptance checklist complete.
docs/decomposition/modules/E3_M3_ItemizationAndInventorySchema.md Matches — NEO-53 HTTP bullet under Related implementation slices; frozen roster table aligns with test/Bruno assertions.
docs/decomposition/modules/documentation_and_implementation_alignment.md Matches — E3.M3 row notes NEO-53 landed with plan, manual QA, README, Bruno links.
docs/decomposition/modules/module_dependency_register.md Matches — E3.M3 In Progress; footnote includes NEO-53.
docs/decomposition/modules/client_server_authority.md Matches — read-only world catalog; server remains source of truth; no client-trusted mutation.
docs/manual-qa/NEO-53.md Matches — curl, schema v1, six ids, spot-checks aligned with automated tests.
server/README.md Matches — Item definitions section with GET path and curl example.

Register/tracking: E3.M3 alignment row update is appropriate; module Status stays In Progress until inventory slices (NEO-54+) land.

Blocking issues

None.

Suggestions

  1. Dependency register footnote (optional) — Extend the E3.M3 note in module_dependency_register.md to mention NEO-53 alongside NEO-50–NEO-52 so the register matches the alignment table (same pattern as NEO-52 review). Done.

  2. Bruno — exact id order (optional) — C# integration test asserts the full frozen six sequence; Bruno only checks ascending sort plus set membership. Adding an explicit ids array assertion (or mirroring the C# frozen list) would catch registry ordering regressions in manual runs without running dotnet test. Done. — frozen prototype six matches registry id order test in Get item definitions.bru.

Nits

  • Nit: ItemDefinitionsWorldApiTests — [Fact] and method body use 2-space indent while the class uses 4-space; SkillDefinitionsWorldApiTests.cs is consistently 4-space. Format-only tidy. Done.

  • Nit: Bruno items are ascending by id block omits the StringComparer.Ordinal comment present in Get skill definitions.bru; harmless for ASCII snake_case ids but copy the comment for parity if touching the file. Done.

  • Nit: ItemDefinitionsWorldApi allocates a List<ItemDefinitionJson> per request — same pattern as skills; fine at six rows.

Verification

cd server
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~ItemDefinitionsWorldApi"
dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj

Manual: docs/manual-qa/NEO-53.md and bruno/neon-sprawl-server/item-definitions/Get item definitions.bru against dev baseUrl.