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