# Code review — NEO-29

- **Date:** 2026-04-25
- **Scope:** Branch `NEO-29-hotbarloadout-v1-contract-baseline-persistence-path` (`origin/main...HEAD`)
- **Base:** `origin/main`

## Verdict

Approve with nits.

## Summary

This branch delivers the planned `HotbarLoadout` v1 vertical slice end-to-end: server `GET`/`POST` APIs with stable deny reason codes, persistence selection (Postgres when configured, in-memory fallback otherwise), client hydration wiring, and targeted tests/manual QA artifacts. The API behavior and persistence wiring align with the NEO-29 plan decisions (per-player scope, fixed eight slots, server-owned validation). Contract safety is good for a prototype slice: deny responses preserve authoritative loadout snapshots, and request validation covers out-of-bounds, unknown abilities, and duplicate slots. Overall runtime risk is low; most residual risk is documentation-state drift rather than code behavior.

## Documentation checked

- `docs/plans/NEO-29-implementation-plan.md` — **matches** (scope, API shape, persistence policy, deny reasons, tests/docs artifacts all present).
- `docs/plans/E1M4-prototype-backlog.md` — **matches** (E1M4-01 slice framing and NEO-29 implementation note are consistent).
- `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` — **partially matches** (implementation snapshot updated, but summary `Status` still `Planned` despite landed implementation).
- `docs/decomposition/modules/module_dependency_register.md` — **partially matches** (E1.M4 row remains `Planned`; this story appears to satisfy “work started,” which should move to `In Progress` per alignment policy).
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **conflicts** with current status labels (rule says move module status from `Planned` to `In Progress` when implementation stories merge).
- `docs/decomposition/modules/contracts.md` — **matches** for prototype scope (JSON/HTTP spike is acceptable for early slice; versioned envelope and stable reason codes are documented).
- `docs/decomposition/modules/client_server_authority.md` — **matches** (server remains authority for persistent gameplay-relevant loadout state; client acts as view/sync).

Register/tracking follow-up needed after merge: update E1.M4 status in `module_dependency_register.md`, `E1_M4_AbilityInputScaffold.md` summary table, and the implementation tracking table entry expectations.

## Blocking issues

None.

## Suggestions

1. ~~Update decomposition status tracking for E1.M4 from `Planned` to `In Progress` in the register/module summary (and corresponding tracking inventory) so docs comply with `documentation_and_implementation_alignment.md` and do not under-report shipped progress.~~ Done. Updated `docs/decomposition/modules/module_dependency_register.md` (row + note), `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` (summary status), and `docs/decomposition/modules/documentation_and_implementation_alignment.md` (new E1.M4 tracking row).

## Nits

- ~~Nit: Consider adding one API test for `slotIndex < 0` deny parity with the existing `slotIndex == 8` test, to lock the lower-bound contract explicitly.~~ Done. Added `PostHotbarLoadout_ShouldDenyNegativeSlot_WithReasonCode` in `server/NeonSprawl.Server.Tests/Game/AbilityInput/HotbarLoadoutApiTests.cs`.

## Verification

- `dotnet test NeonSprawl.sln --filter "FullyQualifiedName~HotbarLoadoutApiTests"`
- `dotnet test NeonSprawl.sln --filter "FullyQualifiedName~HotbarLoadoutPersistenceIntegrationTests"`
- `godot --headless --path client -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/hotbar_loadout_client_test.gd`
- Manual checklist: `docs/manual-qa/NEO-29.md`