diff --git a/docs/reviews/2026-05-23-NEO-53.md b/docs/reviews/2026-05-23-NEO-53.md new file mode 100644 index 0000000..21b197e --- /dev/null +++ b/docs/reviews/2026-05-23-NEO-53.md @@ -0,0 +1,55 @@ +# 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`](../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`](../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`](../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`](../decomposition/modules/module_dependency_register.md) | **Partially matches** — E3.M3 **In Progress**; footnote still lists NEO-50–NEO-52 only (see Suggestion 1). | +| [`docs/decomposition/modules/client_server_authority.md`](../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`](../manual-qa/NEO-53.md) | **Matches** — curl, schema v1, six ids, spot-checks aligned with automated tests. | +| [`server/README.md`](../../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`](../decomposition/modules/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). + +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`. + +## Nits + +- Nit: **`ItemDefinitionsWorldApiTests`** — `[Fact]` and method body use **2-space** indent while the class uses **4-space**; [`SkillDefinitionsWorldApiTests.cs`](../../server/NeonSprawl.Server.Tests/Game/Skills/SkillDefinitionsWorldApiTests.cs) is consistently 4-space. Format-only tidy. + +- Nit: Bruno **`items are ascending by id`** block omits the **StringComparer.Ordinal** comment present in [`Get skill definitions.bru`](../../bruno/neon-sprawl-server/skill-definitions/Get%20skill%20definitions.bru); harmless for ASCII snake_case ids but copy the comment for parity if touching the file. + +- Nit: **`ItemDefinitionsWorldApi`** allocates a `List` per request — same pattern as skills; fine at six rows. + +## Verification + +```bash +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`](../manual-qa/NEO-53.md) and `bruno/neon-sprawl-server/item-definitions/Get item definitions.bru` against dev `baseUrl`.