# Code review — NEO-31

- **Date:** 2026-04-27
- **Scope:** Branch `NEO-31-e1m4-02-input-to-abilitycastrequest-path` (`origin/main...HEAD`) plus current untracked Bruno/Godot UID files in the working tree.
- **Base:** `origin/main`

## Verdict

Approve with nits. **Follow-up:** blocking coordinator-test gap addressed in-repo (`hotbar_cast_slot_resolver.gd` + `hotbar_cast_slot_resolver_test.gd`, `documentation_and_implementation_alignment.md` + register note).

## Summary

NEO-31 adds the prototype `POST /game/players/{id}/ability-cast` route, client digit-key hotbar cast wiring, Bruno requests, manual QA, and E1.M4 documentation updates. The server API is small and well covered, and `dotnet test` passes. **Update:** client-side slot/target resolution is extracted to **`hotbar_cast_slot_resolver.gd`** with GdUnit coverage so unbound vs bound and `lockedTargetId` sourcing are automated without booting `main.gd`.

## Documentation checked

- `docs/plans/NEO-31-implementation-plan.md` — **matches** after follow-up (resolver + resolver tests listed).
- `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` — **matches** for the NEO-31 snapshot and Linear row update.
- `docs/decomposition/modules/module_dependency_register.md` — **matches** after follow-up (E1.M4 note mentions NEO-31).
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches** after follow-up (E1.M4 row documents NEO-31 cast path landed; remaining open items are accept/deny UX, cooldown, telemetry).
- `docs/decomposition/modules/contracts.md` — **matches** for prototype scope. JSON/HTTP remains acceptable as an early spike with explicit `schemaVersion`.
- `docs/decomposition/modules/client_server_authority.md` — **matches** for this slice. The client sends cast intent, while the server validates player existence, hotbar binding, slot bounds, and known abilities; full combat authority remains deferred to E5.M1.

## Blocking Issues

1. ~~`client/test/ability_cast_client_test.gd` does not cover the behavior that lives in `client/scripts/main.gd`, so two plan acceptance criteria are only manually checked. The NEO-31 plan explicitly calls for client harness tests around "formation, send triggers, and unbound-slot behavior" plus target id propagation from a stub target state provider. Current tests verify `AbilityCastClient.request_cast()` serializes a provided `target_id`, but they do not exercise `_request_hotbar_cast_slot()`, do not prove empty slots avoid POSTs, and do not prove `lockedTargetId` is read from `TargetSelectionClient.cached_state()`. Add a focused GdUnit test around the coordinator path, or extract that small decision into a testable helper and cover bound, unbound, and locked-target cases.~~ **Done.** Added `client/scripts/hotbar_cast_slot_resolver.gd` + `client/test/hotbar_cast_slot_resolver_test.gd`; `main.gd` delegates resolution before `request_cast`.

## Suggestions

1. ~~Update `docs/decomposition/modules/documentation_and_implementation_alignment.md` for NEO-31. The module page now says NEO-31 landed, but the tracking table still says "Still open: cast request path"; this will mislead the next implementation/review pass.~~ **Done.**

2. ~~Consider making `AbilityCastClient.request_cast()` return whether a POST actually started, then log `ability_cast_requested` only on success. Today `client/scripts/main.gd` prints `ability_cast_requested` before calling the client, while `client/scripts/ability_cast_client.gd` silently returns when `_busy` is true or when the request fails to start. That can over-count cast requests in the future telemetry hook and conflicts with the plan wording that the hook sits immediately before a successful POST start.~~ **Done.** `request_cast` → `bool`; `_busy` set only after `HTTPRequest.request` returns `OK`; `main.gd` prints only when `true`.

3. ~~Consider adding a short `server/README.md` section for `POST /game/players/{id}/ability-cast`, matching the existing server-side endpoint documentation pattern used for hotbar loadout and targeting. The client README and Bruno collection cover manual usage, but the server API index currently has no NEO-31 entry.~~ **Done.** See [server README — Ability cast (NEO-31)](../../server/README.md#ability-cast-neo-31).

## Nits

- ~~Nit: The new `main.gd` variables `authority_base_url2` and `authority_player_id2` are clear enough, but `ability_base_url` / `ability_player_id` would be easier to scan if this setup grows.~~ **Done.** One `_authority` read applies `base_url` / `dev_player_id` to both HTTP clients after both nodes exist.

## Verification

- `dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj` — passed: 86 tests.
- `git diff --check origin/main...HEAD && git diff --check` — passed.
- `godot --headless --path client -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/ability_cast_client_test.gd` — passed: 4 tests.
- `godot --headless --path client -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/hotbar_cast_slot_resolver_test.gd` — passed: 6 tests.
- Cursor lints checked for the touched C#, GDScript, and docs files — no linter errors reported.
- Not run: Bruno collection / manual checklist `docs/manual-qa/NEO-31.md`.