# Code review — NEO-29

- **Date:** 2026-04-25
- **Scope:** Branch `NEO-29-hotbarloadout-v1-contract-baseline-persistence-path` (`origin/main...HEAD`)
- **Base:** `origin/main`
- **Follow-up:** Re-review after `NEO-29: apply code review follow-ups` and subsequent test refactor commits (`d25d7d0..HEAD`).

## Verdict

Approve with nits.

## Summary

The earlier documentation-alignment and lower-bound slot test suggestions were addressed correctly. E1.M4 status tracking now matches implementation state, the `slotIndex < 0` deny-path test was added, and the client GDScript parse regression in `hotbar_loadout_client_test.gd` is now fixed. Refactored server AAA tests run cleanly in the targeted suites, and the hotbar client test suite now executes successfully. Remaining risk is low and limited to test-run warning noise from the headless Godot runner rather than functional test failures.

## 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` — **matches** (summary status and implementation snapshot now align with landed NEO-29 work).
- `docs/decomposition/modules/module_dependency_register.md` — **matches** (E1.M4 row and note now show `In Progress`).
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches** (tracking table now includes E1.M4 implementation snapshot/pointers).
- `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 from the first pass is now closed.

## 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).
2. ~~Several C# test files now place AAA markers inconsistently (e.g., `// Act` above `// Arrange` and inline comment placement on brace lines). This does not break runtime behavior, but it undermines the readability goal of the AAA refactor and conflicts with the documented template/order in `server/NeonSprawl.Server.Tests/TestTemplates/AAA_TEST_METHOD_TEMPLATE.cs.txt`.~~ Done. Normalized affected suites so AAA markers are consistently ordered and placed on their own lines in `server/NeonSprawl.Server.Tests/Game/Interaction/HorizontalReachTests.cs`, `server/NeonSprawl.Server.Tests/Game/Interaction/InteractionApiTests.cs`, `server/NeonSprawl.Server.Tests/Game/PositionState/MoveCommandValidationTests.cs`, `server/NeonSprawl.Server.Tests/Game/Targeting/PlayerTargetStateReaderTests.cs`, and `server/NeonSprawl.Server.Tests/Game/Targeting/TargetingApiTests.cs`.

## 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`
- Follow-up rerun (re-review):
  - `dotnet test NeonSprawl.sln --filter "FullyQualifiedName~HotbarLoadoutApiTests|FullyQualifiedName~HotbarLoadoutPersistenceIntegrationTests|FullyQualifiedName~TargetingApiTests|FullyQualifiedName~InteractionApiTests|FullyQualifiedName~MoveCommandApiTests|FullyQualifiedName~MoveCommandValidationTests|FullyQualifiedName~MoveStreamApiTests|FullyQualifiedName~PlayerTargetStateReaderTests|FullyQualifiedName~HorizontalReachTests|FullyQualifiedName~InteractablesWorldApiTests"` ✅ (71 passed)
  - `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/hotbar_loadout_client_test.gd` ❌ (parse/type error in `client/test/hotbar_loadout_client_test.gd`)
- Final rerun (latest working tree):
  - `dotnet test NeonSprawl.sln --filter "FullyQualifiedName~HotbarLoadoutApiTests|FullyQualifiedName~HotbarLoadoutPersistenceIntegrationTests|FullyQualifiedName~TargetingApiTests|FullyQualifiedName~InteractionApiTests|FullyQualifiedName~MoveCommandApiTests|FullyQualifiedName~MoveCommandValidationTests|FullyQualifiedName~MoveStreamApiTests|FullyQualifiedName~PlayerTargetStateReaderTests|FullyQualifiedName~HorizontalReachTests|FullyQualifiedName~InteractablesWorldApiTests"` ✅ (71 passed)
  - `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/hotbar_loadout_client_test.gd` ✅ (6 passed; runner still prints non-fatal warning/error noise at shutdown)