NEO-53: Add code review for item-definitions HTTP + Bruno.

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<ItemDefinitionJson>` 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`.