neon-sprawl/docs/reviews/2026-04-27-NEO-31.md

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).

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.

Verification

• dotnet test server/NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj — passed: 86 tests.
• git diff --check origin/main...HEAD && git diff --check — passed.
• Cursor lints checked for the touched C# and GDScript files — no linter errors reported.
• Not run: GdUnit client suite or Bruno collection. Run godot --headless --path client -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/ability_cast_client_test.gd and the NEO-31 Bruno/manual checklist before merge.