NEO-25: close code review follow-up (alignment, warning, tests)

client/scripts/main.gd

@@ -368,5 +368,9 @@ func _on_interactables_catalog_ready(descriptors: Array) -> void:
 		_radius_preview.call("setup_glow_groups", groups as Array)
 
 
-func _on_interactables_catalog_failed(_reason: String) -> void:
+func _on_interactables_catalog_failed(reason: String) -> void:
-	pass
+	# `InteractablesCatalogClient` already `push_error`s; this hook exists for wiring clarity.
+	push_warning(
+		"Interactables catalog failed (%s) — start the game server first; see client README section \"Interaction + range preview (NEO-9 + NEO-25)\"."
+		% reason
+	)

client/test/interaction_request_client_test.gd

@@ -28,6 +28,34 @@ class MockHttpTransport:
 		return OK
 
 
+## First POST defers completion one frame so a second press can queue as `_pending_interactable_id`.
+class HoldFirstThenAutoTransport:
+	extends Node
+	signal request_completed(
+		result: int, response_code: int, headers: PackedStringArray, body: PackedByteArray
+	)
+	var bodies: Array[String] = []
+	var _first: bool = true
+
+	func request(
+		_url: String,
+		_custom_headers: PackedStringArray = PackedStringArray(),
+		_method: HTTPClient.Method = HTTPClient.METHOD_GET,
+		request_data: String = ""
+	) -> Error:
+		bodies.append(request_data)
+		var body_bytes: PackedByteArray = '{"schemaVersion":1,"allowed":true}'.to_utf8_buffer()
+		if _first:
+			_first = false
+			call_deferred("_emit", body_bytes)
+		else:
+			request_completed.emit(HTTPRequest.RESULT_SUCCESS, 200, PackedStringArray(), body_bytes)
+		return OK
+
+	func _emit(body_bytes: PackedByteArray) -> void:
+		request_completed.emit(HTTPRequest.RESULT_SUCCESS, 200, PackedStringArray(), body_bytes)
+
+
 func _make_ix(transport: Node) -> Node:
 	var ix: Node = IxClient.new()
 	ix.set("injected_http", transport)
@@ -50,3 +78,29 @@ func test_post_resource_body_contains_resource_node_id() -> void:
 	var ix := _make_ix(transport)
 	ix.call("_post_interact", "prototype_resource_node_alpha")
 	assert_that(transport.last_body).contains("prototype_resource_node_alpha")
+
+
+func test_post_interact_terminal_public_entrypoint() -> void:
+	var transport := MockHttpTransport.new()
+	var ix := _make_ix(transport)
+	ix.call("post_interact_terminal")
+	assert_that(transport.last_body).contains("prototype_terminal")
+
+
+func test_post_interact_resource_public_entrypoint() -> void:
+	var transport := MockHttpTransport.new()
+	var ix := _make_ix(transport)
+	ix.call("post_interact_resource")
+	assert_that(transport.last_body).contains("prototype_resource_node_alpha")
+
+
+func test_last_wins_second_press_before_first_completes() -> void:
+	var transport := HoldFirstThenAutoTransport.new()
+	var ix := _make_ix(transport)
+	ix.call("_post_interact", "prototype_terminal")
+	ix.call("_post_interact", "prototype_resource_node_alpha")
+	await get_tree().process_frame
+	await get_tree().process_frame
+	assert_that(transport.bodies.size()).is_equal(2)
+	assert_that(transport.bodies[0]).contains("prototype_terminal")
+	assert_that(transport.bodies[1]).contains("prototype_resource_node_alpha")

docs/decomposition/modules/documentation_and_implementation_alignment.md

@@ -48,7 +48,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). **`InteractableDescriptor`** and **`SelectionEvent`** still open. **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), [Interaction](../../../server/README.md#interaction-neo-9) |
+| 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). **Still open:** **`SelectionEvent`** and richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact` (see [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md)). **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) |
 
 ---
 

docs/decomposition/modules/module_dependency_register.md

@@ -19,7 +19,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
 
 **E1.M2 note:** Prototype slice **Ready**: `client/scripts/isometric_follow_camera.gd` + child `Camera3D` on **`World/IsometricFollowCamera`** in `client/scenes/main.tscn`; per-tick **`CameraState`** (`client/scripts/camera_state.gd`); **`ZoomBandConfig`** ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); **`OcclusionPolicy`** ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); pick-through **`"occluder"`** convention ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); consumer contract + occluder lifecycle hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). See [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md).
 
-**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). **`InteractableDescriptor`** / **`SelectionEvent`** and a Godot consumer for targeting remain **in progress** 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.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)). **`SelectionEvent`** and richer multi-consumer **`InteractableDescriptor`** work remain **in progress** on later Linear issues. **Precursor (E1.M1):** **`InteractionRequest`** + horizontal reach ([NEO-9](../../plans/NEO-9-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`).
 
 ### Epic 2 — Skills and Progression Framework
 

docs/reviews/2026-04-24-NEO-25.md

@@ -0,0 +1,53 @@
+# Code review — NEO-25 (InteractableDescriptor list / fetch-driven client)
+
+**Date:** 2026-04-24
+
+**Scope:** Branch `NEO-25-e1m3-interactable-descriptor-list` vs `main` (full story: server `GET /game/world/interactables`, registry + two rows, Godot catalog + world build + dual keys, tests, Bruno, docs).
+
+**Base:** `main` (inferred from `git diff main`).
+
+**Follow-up:** Suggestions below addressed in-repo (decomposition alignment, catalog-failure warning, GdUnit public + last-wins tests).
+
+## Verdict
+
+**Approve with nits**
+
+## Summary
+
+The branch delivers the NEO-25 plan end-to-end: versioned `GET /game/world/interactables` with stable ascending ids, `PrototypeInteractableRegistry` extended with `kind` and a second anchor clear of the scene obstacle, integration tests for GET and per-id `POST …/interact` range behavior, Godot fetch-driven props and multi-row glow preview, `interact` / `interact_secondary` wired to fixed ids with `main.gd` forwarding keys through `_unhandled_key_input` (reasonable fix for embedded dock focus), and last-wins queuing on the interaction client when POST overlaps. Overall risk is **low** for a prototype spike; server authority for interact outcomes is unchanged. All **65** `dotnet test` solutions tests pass in this environment.
+
+## Documentation checked
+
+| Path | Assessment |
+|------|------------|
+| `docs/plans/NEO-25-implementation-plan.md` | **Matches** — acceptance criteria, route shape, anchors, client option A, tests, Bruno, manual QA path align with the diff. |
+| `docs/manual-qa/NEO-25.md` | **Matches** — steps match implemented behavior and anchors. |
+| `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` | **Matches** — implementation snapshot updated to cite `GET /game/world/interactables` and client scripts. |
+| `docs/decomposition/modules/client_server_authority.md` | **Matches** (spot-check) — client preview + server `POST …/interact` authority preserved. |
+| `docs/decomposition/modules/contracts.md` | **N/A** for blocking — JSON v1 spike discipline unchanged; no contradiction. |
+| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E1.M3 note now includes **NEO-25** partial (`GET /game/world/interactables`, DTOs/API, Godot catalog + builder); **SelectionEvent** / richer descriptor consumers called out as still in progress. *(Was **Partially matches** at review time; register updated in follow-up.)* |
+| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E1.M3 snapshot documents **NEO-25** list projection + open **SelectionEvent** / richer consumers. *(Was **Partially matches** at review time; tracking row updated in follow-up.)* |
+
+## Blocking issues
+
+*(none)*
+
+## Suggestions
+
+1. ~~**Tracking table / register copy** — Update `documentation_and_implementation_alignment.md` (E1.M3 row) and optionally `module_dependency_register.md` (E1.M3 note) to record NEO-25: versioned **`GET /game/world/interactables`**, `InteractablesListDtos` / `InteractablesWorldApi`, and Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd`, clarifying that full **`SelectionEvent`** / richer descriptor consumers remain future work.~~ **Done.** Both files updated (E1.M3 snapshot + E1.M3 note; server README anchor links).
+2. ~~**`main.gd` catalog failure handler** — `_on_interactables_catalog_failed` is intentionally empty (errors already come from `InteractablesCatalogClient`). A one-line `push_warning` or comment pointing to README “server first” would make the no-op obvious to future readers.~~ **Done.** `push_warning` with reason + pointer to client README section “Interaction + range preview (NEO-9 + NEO-25)”; comment notes catalog client already `push_error`s.
+
+## Nits
+
+- **Nit:** `InteractableWorldBuilder` uses `extends Object` and is invoked via `load(...).call("build_from_catalog", …)` from `main.gd`; a `RefCounted` or `Node`-attached helper would avoid `load` at runtime, but this is consistent enough for prototype scope.
+- ~~**Nit:** GdUnit `interaction_request_client_test.gd` calls `_post_interact` directly; optionally add cases for `post_interact_terminal` / `post_interact_resource` public entrypoints and for last-wins `_pending_interactable_id` behavior if regressions become a concern.~~ **Done.** Added `test_post_interact_*_public_entrypoint` and `test_last_wins_second_press_before_first_completes` (`HoldFirstThenAutoTransport`).
+
+## Verification
+
+- `dotnet test /path/to/NeonSprawl.sln` (from repo root) — expect all tests green.
+- Manual: follow `docs/manual-qa/NEO-25.md` — curl GET, F5 with server, E/R at both anchors, server-down degradation.
+- Optional: run GdUnit client suites if your CI does not already.
+
+---
+
+*Review produced per [code review agent](.cursor/rules/code-review-agent.md).*