diff --git a/client/scripts/hotbar_cast_slot_resolver.gd b/client/scripts/hotbar_cast_slot_resolver.gd new file mode 100644 index 0000000..76ea168 --- /dev/null +++ b/client/scripts/hotbar_cast_slot_resolver.gd @@ -0,0 +1,38 @@ +extends Object + +## Pure resolver for digit hotbar → cast intent (NEO-31). [code]main.gd[/code] delegates here so +## GdUnit can cover unbound slots and [code]lockedTargetId[/code] sourcing without booting the scene. +## No [code]class_name[/code] — match [code]prototype_target_constants.gd[/code] preload pattern. + +const KEY_OK := "ok" +const KEY_REASON := "reason" +const KEY_SLOT_INDEX := "slotIndex" +const KEY_ABILITY_ID := "abilityId" +const KEY_TARGET_ID := "targetId" + +const REASON_INVALID_SLOT := "invalid_slot_index" +const REASON_EMPTY_SLOT := "empty_slot" + + +## [param slots] snapshot from [code]HotbarState.slots_snapshot()[/code] (fixed length, entries +## [code]null[/code] or [String] ability ids). +## [param target_cached_state] from [code]TargetSelectionClient.cached_state()[/code] (may be empty). +## Returns [code]{ "ok": true, "slotIndex", "abilityId", "targetId" }[/code] or +## [code]{ "ok": false, "reason" }[/code]. +static func resolve(slot_index: int, slots: Array, target_cached_state: Dictionary) -> Dictionary: + if slot_index < 0 or slot_index >= slots.size(): + return {KEY_OK: false, KEY_REASON: REASON_INVALID_SLOT} + var ability_variant: Variant = slots[slot_index] + if ability_variant == null or not (ability_variant is String) or (ability_variant as String).strip_edges().is_empty(): + return {KEY_OK: false, KEY_REASON: REASON_EMPTY_SLOT} + var ability_id: String = (ability_variant as String).strip_edges() + var target_id: Variant = null + var locked: Variant = target_cached_state.get("lockedTargetId", null) + if locked is String and not (locked as String).is_empty(): + target_id = locked as String + return { + KEY_OK: true, + KEY_SLOT_INDEX: slot_index, + KEY_ABILITY_ID: ability_id, + KEY_TARGET_ID: target_id, + } diff --git a/client/scripts/main.gd b/client/scripts/main.gd index d2ccf67..b35bd0a 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -38,6 +38,7 @@ const AUTH_SNAP_LOCOMOTION_SKIP_MAX_DH: float = 0.55 const AUTH_SNAP_LOCOMOTION_VEL_HZ_SQ: float = 0.12 const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") +const HotbarCastSlotResolver := preload("res://scripts/hotbar_cast_slot_resolver.gd") ## Bump on each rejection so older one-shot timers do not clear a newer message. var _move_reject_msg_token: int = 0 @@ -350,27 +351,31 @@ func _request_hotbar_cast_slot(slot_index: int) -> void: if not is_instance_valid(_hotbar_state) or not _hotbar_state.has_method("slots_snapshot"): return var slots: Array = _hotbar_state.call("slots_snapshot") as Array - if slot_index < 0 or slot_index >= slots.size(): - push_warning("Hotbar cast ignored: slot %d invalid_slot_index" % slot_index) - return - var ability_variant: Variant = slots[slot_index] - if ability_variant == null or not (ability_variant is String) or (ability_variant as String).strip_edges().is_empty(): - push_warning("Hotbar cast ignored: slot %d empty_slot" % slot_index) - return - var ability_id: String = ability_variant as String - var target_id: Variant = null + var target_state: Dictionary = {} if is_instance_valid(_target_client) and _target_client.has_method("cached_state"): var st: Variant = _target_client.call("cached_state") if st is Dictionary: - var locked: Variant = (st as Dictionary).get("lockedTargetId", null) - if locked is String and not (locked as String).is_empty(): - target_id = locked + target_state = st as Dictionary + var outcome: Dictionary = HotbarCastSlotResolver.resolve(slot_index, slots, target_state) + if not bool(outcome.get(HotbarCastSlotResolver.KEY_OK, false)): + var reason: String = str(outcome.get(HotbarCastSlotResolver.KEY_REASON, "")) + if reason == HotbarCastSlotResolver.REASON_INVALID_SLOT: + push_warning("Hotbar cast ignored: slot %d invalid_slot_index" % slot_index) + elif reason == HotbarCastSlotResolver.REASON_EMPTY_SLOT: + push_warning("Hotbar cast ignored: slot %d empty_slot" % slot_index) + return + var ability_id: String = outcome[HotbarCastSlotResolver.KEY_ABILITY_ID] as String + var resolved_slot: int = int(outcome.get(HotbarCastSlotResolver.KEY_SLOT_INDEX, slot_index)) + var target_id: Variant = outcome.get(HotbarCastSlotResolver.KEY_TARGET_ID, null) var target_label: String = "null" if target_id != null: target_label = str(target_id) - print("ability_cast_requested slot=%d ability_id=%s targetId=%s" % [slot_index, ability_id, target_label]) + print( + "ability_cast_requested slot=%d ability_id=%s targetId=%s" + % [resolved_slot, ability_id, target_label] + ) if is_instance_valid(_ability_cast_client) and _ability_cast_client.has_method("request_cast"): - _ability_cast_client.call("request_cast", slot_index, ability_id, target_id) + _ability_cast_client.call("request_cast", resolved_slot, ability_id, target_id) func _forward_interact_post(method: String) -> void: diff --git a/client/test/hotbar_cast_slot_resolver_test.gd b/client/test/hotbar_cast_slot_resolver_test.gd new file mode 100644 index 0000000..5174618 --- /dev/null +++ b/client/test/hotbar_cast_slot_resolver_test.gd @@ -0,0 +1,100 @@ +extends GdUnitTestSuite + +const Resolver := preload("res://scripts/hotbar_cast_slot_resolver.gd") + + +func _eight_empty_slots() -> Array: + var slots: Array = [] + slots.resize(8) + for i in range(8): + slots[i] = null + return slots + + +func test_resolve_returns_ok_with_target_from_locked_target_id() -> void: + # Arrange + var slots := _eight_empty_slots() + slots[0] = "prototype_pulse" + var target_state := {"lockedTargetId": "prototype_target_alpha", "sequence": 3} + + # Act + var outcome: Dictionary = Resolver.resolve(0, slots, target_state) + + # Assert + assert_that(bool(outcome.get(Resolver.KEY_OK, false))).is_true() + assert_that(int(outcome.get(Resolver.KEY_SLOT_INDEX, -1))).is_equal(0) + assert_that(outcome.get(Resolver.KEY_ABILITY_ID)).is_equal("prototype_pulse") + assert_that(outcome.get(Resolver.KEY_TARGET_ID)).is_equal("prototype_target_alpha") + + +func test_resolve_returns_null_target_when_no_lock() -> void: + # Arrange + var slots := _eight_empty_slots() + slots[1] = "prototype_guard" + var target_state := {"lockedTargetId": null, "validity": "none"} + + # Act + var outcome: Dictionary = Resolver.resolve(1, slots, target_state) + + # Assert + assert_that(bool(outcome.get(Resolver.KEY_OK, false))).is_true() + assert_that(outcome.get(Resolver.KEY_TARGET_ID, "sentinel")).is_null() + + +func test_resolve_returns_null_target_when_lock_empty_string() -> void: + # Arrange + var slots := _eight_empty_slots() + slots[2] = "prototype_dash" + var target_state := {"lockedTargetId": ""} + + # Act + var outcome: Dictionary = Resolver.resolve(2, slots, target_state) + + # Assert + assert_that(bool(outcome.get(Resolver.KEY_OK, false))).is_true() + assert_that(outcome.get(Resolver.KEY_TARGET_ID, "sentinel")).is_null() + + +func test_resolve_denies_empty_slot_without_ability() -> void: + # Arrange + var slots := _eight_empty_slots() + slots[3] = null + var target_state := {"lockedTargetId": "prototype_target_beta"} + + # Act + var outcome: Dictionary = Resolver.resolve(3, slots, target_state) + + # Assert + assert_that(bool(outcome.get(Resolver.KEY_OK, true))).is_false() + assert_that(str(outcome.get(Resolver.KEY_REASON, ""))).is_equal(Resolver.REASON_EMPTY_SLOT) + + +func test_resolve_denies_empty_string_ability() -> void: + # Arrange + var slots := _eight_empty_slots() + slots[4] = " " + var target_state := {} + + # Act + var outcome: Dictionary = Resolver.resolve(4, slots, target_state) + + # Assert + assert_that(bool(outcome.get(Resolver.KEY_OK, true))).is_false() + assert_that(str(outcome.get(Resolver.KEY_REASON, ""))).is_equal(Resolver.REASON_EMPTY_SLOT) + + +func test_resolve_denies_slot_index_out_of_range() -> void: + # Arrange + var slots := _eight_empty_slots() + slots[0] = "prototype_pulse" + var target_state := {} + + # Act + var high: Dictionary = Resolver.resolve(8, slots, target_state) + var low: Dictionary = Resolver.resolve(-1, slots, target_state) + + # Assert + assert_that(bool(high.get(Resolver.KEY_OK, true))).is_false() + assert_that(str(high.get(Resolver.KEY_REASON, ""))).is_equal(Resolver.REASON_INVALID_SLOT) + assert_that(bool(low.get(Resolver.KEY_OK, true))).is_false() + assert_that(str(low.get(Resolver.KEY_REASON, ""))).is_equal(Resolver.REASON_INVALID_SLOT) diff --git a/docs/decomposition/modules/documentation_and_implementation_alignment.md b/docs/decomposition/modules/documentation_and_implementation_alignment.md index 7a0b789..cb557c2 100644 --- a/docs/decomposition/modules/documentation_and_implementation_alignment.md +++ b/docs/decomposition/modules/documentation_and_implementation_alignment.md @@ -49,7 +49,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not | E1.M1 | Ready | Prototype milestone **Done** ([E1.M1](https://linear.app/neon-sprawl/project/e1m1-inputandmovementruntime-20eb28dd3db4)). Authoritative `PositionState` + **MoveCommand** over HTTP (JSON v1 snap); **in-memory** store by default, **Postgres** when configured ([NEO-8](../../plans/NEO-8-implementation-plan.md)); shared **NpgsqlDataSource** disposed on host shutdown ([NEO-13](../../plans/NEO-13-implementation-plan.md)); Godot sync + path-follow ([NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md)); **InteractionRequest** + horizontal range ([NEO-9](../../plans/NEO-9-implementation-plan.md)). Follow-on: prediction/reconciliation, Slice 1 telemetry, Protobuf wire per [contracts.md](contracts.md). | [NEO-6](../../plans/NEO-6-implementation-plan.md), [NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-8](../../plans/NEO-8-implementation-plan.md), [NEO-9](../../plans/NEO-9-implementation-plan.md), [NEO-10](../../plans/NEO-10-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md), [NEO-13](../../plans/NEO-13-implementation-plan.md); `server/NeonSprawl.Server/Game/PositionState/`, `Game/Interaction/`; [server README](../../../server/README.md) | | E1.M2 | Ready | **Slice 2 prototype slice closed:** follow + `CameraState` ([NEO-15](https://linear.app/neon-sprawl/issue/NEO-15)); zoom bands ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); occlusion ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); occluder pick-through ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); contracts + hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). Client-local; no server use of camera pose. | [NEO-15](../../plans/NEO-15-implementation-plan.md), [NEO-16](../../plans/NEO-16-implementation-plan.md), [NEO-17](../../plans/NEO-17-implementation-plan.md), [NEO-18](../../plans/NEO-18-implementation-plan.md), [NEO-20](../../plans/NEO-20-implementation-plan.md); `client/scripts/isometric_follow_camera.gd`, `camera_state.gd`, `zoom_band_config.gd`, `occlusion_policy.gd`, `ground_pick.gd`; [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md) | | E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **NEO-25 landed:** versioned **`GET /game/world/interactables`** ([NEO-25](../../plans/NEO-25-implementation-plan.md)) — `InteractablesListResponse` / `InteractablesWorldApi` in `server/NeonSprawl.Server/Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` (fetch-driven props + glow); [server README — Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25). **NEO-26 landed:** prototype **`SelectionEvent`** — **`TargetSelectionClient.selection_event`** ([NEO-26](../../plans/NEO-26-implementation-plan.md)) when **`lockedTargetId`** changes; optional **`log_selection_events`**. **NEO-27 landed:** Slice 3 telemetry hook sites documented — `selection_event` maps to product `target_changed` in `target_selection_client.gd`; server lock transition hook comments in `InMemoryPlayerTargetLockStore`; reserved `ability_cast_requested` / `ability_cast_denied` comments in `client/scripts/main.gd` (all TODO(E9.M1)); see [NEO-27](../../plans/NEO-27-implementation-plan.md) and [`docs/manual-qa/NEO-27.md`](../../manual-qa/NEO-27.md). **Follow-on / still open:** richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact`; production telemetry ingest/catalog after E9.M1. **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25), [Interaction](../../../server/README.md#interaction-neo-9) | -| E1.M4 | In Progress | **NEO-29 landed:** prototype `HotbarLoadout` v1 server contract (`GET`/`POST /game/players/{id}/hotbar-loadout`) with stable deny reason codes, per-player scope, and persistence policy matching NEO-8/NS-17 (Postgres when configured, in-memory fallback otherwise). Client boot hydration is wired via `hotbar_loadout_client.gd` + `hotbar_state.gd` from `main.gd`; manual QA checklist created. **Still open:** cast request path, accept/deny integration UX, cooldown snapshot sync, and telemetry hooks from later E1.M4 stories. | [NEO-29](../../plans/NEO-29-implementation-plan.md); [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md); [E1M4 prototype backlog](../../plans/E1M4-prototype-backlog.md); `server/NeonSprawl.Server/Game/AbilityInput/`; [`client/scripts/hotbar_loadout_client.gd`](../../../client/scripts/hotbar_loadout_client.gd), [`client/scripts/hotbar_state.gd`](../../../client/scripts/hotbar_state.gd); [server README — Hotbar loadout](../../../server/README.md#hotbar-loadout-neo-29) | +| E1.M4 | In Progress | **NEO-29 landed:** prototype `HotbarLoadout` v1 server contract (`GET`/`POST /game/players/{id}/hotbar-loadout`) with stable deny reason codes, per-player scope, and persistence policy matching NEO-8/NS-17 (Postgres when configured, in-memory fallback otherwise). Client boot hydration is wired via `hotbar_loadout_client.gd` + `hotbar_state.gd` from `main.gd`; manual QA checklist created. **NEO-31 landed:** prototype **`POST /game/players/{id}/ability-cast`**, digit-key cast wiring from `main.gd`, `ability_cast_client.gd`, and `hotbar_cast_slot_resolver.gd` (target id from `lockedTargetId` in cached target state); GdUnit covers resolver + HTTP client; manual QA [NEO-31](../../manual-qa/NEO-31.md). **Still open:** combat accept/deny integration UX, cooldown snapshot sync, and telemetry hooks from later E1.M4 stories. | [NEO-29](../../plans/NEO-29-implementation-plan.md), [NEO-31](../../plans/NEO-31-implementation-plan.md); [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md); [E1M4 prototype backlog](../../plans/E1M4-prototype-backlog.md); `server/NeonSprawl.Server/Game/AbilityInput/`; [`client/scripts/hotbar_loadout_client.gd`](../../../client/scripts/hotbar_loadout_client.gd), [`client/scripts/hotbar_state.gd`](../../../client/scripts/hotbar_state.gd), [`client/scripts/ability_cast_client.gd`](../../../client/scripts/ability_cast_client.gd), [`client/scripts/hotbar_cast_slot_resolver.gd`](../../../client/scripts/hotbar_cast_slot_resolver.gd); [server README — Hotbar loadout](../../../server/README.md#hotbar-loadout-neo-29) | --- diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index 880ff63..fee350a 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -21,7 +21,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen **E1.M3 note:** Decomposition expanded in [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md) (E1.M1 vs E1.M3 boundary, contract sketches, Slice 3 alignment). **Partial (NEO-23):** versioned JSON v1 **`PlayerTargetStateResponse`** + **`POST …/target/select`** under `server/NeonSprawl.Server/Game/Targeting/` ([NEO-23](../../plans/NEO-23-implementation-plan.md); [server README — Targeting](../../../server/README.md#targeting-neo-23)) — throwaway HTTP spike per [contracts.md](contracts.md). **Partial (NEO-25):** versioned **`GET /game/world/interactables`** + `InteractablesListDtos` / `InteractablesWorldApi` in `Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` ([NEO-25](../../plans/NEO-25-implementation-plan.md); [server README — Interactable descriptors](../../../server/README.md#interactable-descriptors-neo-25)). **NEO-26 (prototype `SelectionEvent`):** Godot **`TargetSelectionClient.selection_event`** on **`lockedTargetId`** changes ([NEO-26](../../plans/NEO-26-implementation-plan.md)). **Follow-on / in progress:** richer multi-consumer **`InteractableDescriptor`** targeting on later Linear issues. **Precursor (E1.M1):** **`InteractionRequest`** + horizontal reach ([NEO-9](../../plans/NEO-9-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`). -**E1.M4 note:** Vertical-slice decomposition is defined in [E1M4-prototype-backlog.md](../../plans/E1M4-prototype-backlog.md) with story slugs **E1M4-01** through **E1M4-05** (loadout contract, cast path, accept/deny UX, cooldown sync, telemetry hooks). **NEO-29** started implementation with `HotbarLoadout` v1 server APIs + persistence wiring and client hydration, so module status is **In Progress**; see [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md) and [NEO-29 plan](../../plans/NEO-29-implementation-plan.md). +**E1.M4 note:** Vertical-slice decomposition is defined in [E1M4-prototype-backlog.md](../../plans/E1M4-prototype-backlog.md) with story slugs **E1M4-01** through **E1M4-05** (loadout contract, cast path, accept/deny UX, cooldown sync, telemetry hooks). **NEO-29** started implementation with `HotbarLoadout` v1 server APIs + persistence wiring and client hydration; **NEO-31** added the prototype cast POST + client digit-key path and resolver tests — module status remains **In Progress**; see [E1_M4_AbilityInputScaffold.md](E1_M4_AbilityInputScaffold.md), [NEO-29 plan](../../plans/NEO-29-implementation-plan.md), and [NEO-31 plan](../../plans/NEO-31-implementation-plan.md). ### Epic 2 — Skills and Progression Framework diff --git a/docs/plans/NEO-31-implementation-plan.md b/docs/plans/NEO-31-implementation-plan.md index 8b760e9..11d8259 100644 --- a/docs/plans/NEO-31-implementation-plan.md +++ b/docs/plans/NEO-31-implementation-plan.md @@ -59,7 +59,9 @@ | `server/NeonSprawl.Server/Game/AbilityInput/AbilityCastApi.cs` | Route handler: validation, loadout cross-check, prototype response. | | `server/NeonSprawl.Server.Tests/Game/AbilityInput/AbilityCastApiTests.cs` | HTTP integration tests for accept/deny paths and payload shape. | | `client/scripts/ability_cast_client.gd` | Thin HTTP client for cast POST (injection pattern matches hotbar client). | -| `client/test/ability_cast_client_test.gd` | Mock-transport tests for payload formation and unbound-slot no-op. | +| `client/scripts/hotbar_cast_slot_resolver.gd` | Pure hotbar slot + cached target state → cast payload / deny reason (testable from `main.gd`). | +| `client/test/ability_cast_client_test.gd` | Mock-transport tests for payload formation and busy coalescing. | +| `client/test/hotbar_cast_slot_resolver_test.gd` | Resolver: `lockedTargetId` propagation, empty slot / OOB denies (acceptance criteria without booting `main.gd`). | | `docs/manual-qa/NEO-31.md` | Manual QA checklist (slot keys, bound vs unbound, target id from HUD lock). | | `bruno/neon-sprawl-server/ability-cast/Post hotbar bind slot0 pulse.bru` | POST hotbar bind slot 0 → `prototype_pulse` so cast happy/mismatch requests have deterministic loadout state. | | `bruno/neon-sprawl-server/ability-cast/Post cast happy.bru` | Happy-path `POST …/ability-cast` (run after preset when exercising alone). | @@ -73,7 +75,7 @@ |------|-----------| | `server/NeonSprawl.Server/Program.cs` | Register `MapAbilityCastApi()` (or named extension) alongside existing maps. | | `client/project.godot` | Add `hotbar_slot_1` … `hotbar_slot_8` (or compact equivalent) bound to digit keys for prototype. | -| `client/scripts/main.gd` | Wire input → hotbar snapshot + `TargetSelectionClient` + cast client; real `ability_cast_requested` hook site before submit. | +| `client/scripts/main.gd` | Wire input → `HotbarCastSlotResolver` + cast client; real `ability_cast_requested` hook site before submit. | | `client/README.md` | Document digit hotbar keys, cast endpoint, and manual QA link. | | `docs/decomposition/modules/E1_M4_AbilityInputScaffold.md` | Snapshot + Linear link for E1M4-02. | @@ -82,7 +84,8 @@ | Suite | What it covers | |--------|----------------| | `AbilityCastApiTests.cs` | POST success when loadout matches; denies with stable `reasonCode` for bad player, wrong slot/ability, empty binding. | -| `ability_cast_client_test.gd` | Client builds JSON with `targetId` from injected target state; unbound slot does not call `request`; bound slot emits exactly one POST body. | +| `ability_cast_client_test.gd` | Client builds JSON with `targetId` from caller; busy coalescing. | +| `hotbar_cast_slot_resolver_test.gd` | Bound slot + `lockedTargetId` / empty lock / unbound / OOB — same rules `main.gd` uses before `request_cast`. | | Manual `docs/manual-qa/NEO-31.md` | Human verification of keys, logs, and HUD target id alignment. | | Bruno `ability-cast/*.bru` | Embedded `tests { … }` blocks assert status + JSON fields for quick regression (same style as `hotbar-loadout/Post loadout bind.bru`). | diff --git a/docs/reviews/2026-04-27-NEO-31.md b/docs/reviews/2026-04-27-NEO-31.md new file mode 100644 index 0000000..86a0476 --- /dev/null +++ b/docs/reviews/2026-04-27-NEO-31.md @@ -0,0 +1,45 @@ +# 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. + +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. + +## 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.