# Review — NEO-24

Date: 2026-04-21
Scope: `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` (`origin/main...HEAD`)
Base: `origin/main` (merge-base `37f44a494400f4fdd369077c52dfc012df73a5d5`)
Follow-up: blocking issue + suggestions below are **done** (strikethrough + **Done.**). See the [NEO-24 plan](../plans/NEO-24-implementation-plan.md) Decision 3 for the signal change.

## Verdict

~~Request changes~~ — **Addressed.** Blocking issue fixed; integration test added; doc-alignment row updated.

## Summary

This branch adds the client-side target selection node, HUD label, input bindings, tests, and story docs for NEO-24. The overall direction matches the E1.M3 plan well: selection intent is POSTed to the server, the HUD paints only server-acknowledged target state, and denial handling is intentionally authoritative.

The main risk is that the advertised movement-driven soft-lock refresh is not actually wired to normal locomotion. The new unit suite passes, but it only proves `TargetSelectionClient` reacts correctly when its refresh hook is called directly; it does not cover the real signal behavior of `PositionAuthorityClient`, so the branch can merge with a broken end-to-end acceptance criterion.

## Documentation checked

- `docs/plans/NEO-24-implementation-plan.md` — **conflicts**. Decision 2 and the acceptance criteria require movement-triggered refresh while a lock is held, but the current wiring only refreshes on boot sync / rejection resync, not on ordinary movement.
- `docs/manual-qa/NEO-24.md` — **conflicts**. Section 4 expects walking out of range to flip HUD validity within ~250 ms during normal locomotion; that flow is not reachable with the current signal source.
- `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` — **matches** for server-authoritative target state and denial reconciliation.
- `docs/decomposition/modules/client_server_authority.md` — **matches** for client-as-intent / server-as-truth behavior.
- `docs/decomposition/modules/module_dependency_register.md` — **matches**. `E1.M3` remains `In Progress`.
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches**. The `E1.M3` tracking row still says client tab-target is open; if/when this story lands, that snapshot should be updated to reflect NEO-24 progress.

## Blocking issues

1. ~~The movement-driven refresh path described by the plan, README, and manual QA is not reachable during normal locomotion. `main.gd` connects `TargetSelectionClient.on_authoritative_position_snap()` to `PositionAuthorityClient.authoritative_position_received`, but `position_authority_client.gd` only emits that signal from `_emit_position_from_response()` during `sync_from_server()` and the move-rejection recovery path. Successful `move-stream` POSTs explicitly do not emit the signal, so walking around with a held lock never triggers the throttled `GET /target` refresh that NEO-24 depends on. As a result, the HUD can stay stuck at `Validity: ok` after the player walks out of range, which breaks the plan's Decision 2, the acceptance criterion for soft-lock refresh, and manual QA section 4.~~ **Done.** Added a separate `authoritative_ack(world)` signal on `PositionAuthorityClient` that fires on both `BOOT_GET` 200 and successful `move-stream` 200 (extracted a shared `_parse_world_from_response` helper). `authoritative_position_received` keeps its current snap-only semantics so rubber-band suppression is untouched. `TargetSelectionClient.on_authoritative_ack(world)` replaces `on_authoritative_position_snap`; cooldown + lock gate unchanged. `main.gd` connects to the new signal. See `client/scripts/position_authority_client.gd`, `target_selection_client.gd`, `main.gd`, and plan Decision 3.

   Relevant behavior in `client/scripts/position_authority_client.gd`:

   ```text
   match _phase:
   	Phase.BOOT_GET:
   		_busy = false
   		if response_code == 200:
   			_emit_position_from_response(text, true)
   	Phase.STREAM_POST:
   		...
   		if response_code == 200:
   			# Do not emit `authoritative_position_received` here.
   			pass
   ```

   Meanwhile `client/scripts/target_selection_client.gd` only refreshes from that signal:

   ```text
   func on_authoritative_position_snap(_world: Vector3, apply_as_snap: bool) -> void:
   	if not apply_as_snap:
   		return
   	if not _has_lock():
   		return
   	...
   	request_sync_from_server()
   ```

   The fix needs to either emit a suitable movement/reconciliation signal during normal locomotion or hook the target refresh to another authoritative movement event that actually occurs while moving.

## Suggestions

1. ~~Add an integration-level test for the real movement/targeting wiring. The current `client/test/target_selection_client_test.gd` suite calls `on_authoritative_position_snap()` directly, which is why it misses the broken connection to `PositionAuthorityClient`. A focused test around `main.gd` wiring or `PositionAuthorityClient` signal behavior would prevent this regression.~~ **Done.** Added `client/test/target_refresh_on_locomotion_test.gd`: wires a real `PositionAuthorityClient` and `TargetSelectionClient` with mock transports and the same `authoritative_ack` → `on_authoritative_ack` connection `main.gd` uses. Verifies (a) a `move-stream` 200 with a held lock produces exactly one throttled `GET /target` and the cached state flips to `out_of_range`, and (b) the same path is a no-op when no lock is held. Also added `test_sync_boot_get_200_emits_authoritative_ack` and `test_move_stream_post_200_emits_authoritative_ack` to `position_authority_client_test.gd` so renames on either side will break the per-script suites too.
2. ~~Update `docs/decomposition/modules/documentation_and_implementation_alignment.md` if this story lands. Its `E1.M3` snapshot still says client tab-target is open, which will be stale after merge.~~ **Done.** E1.M3 row now reflects NEO-24 landed (HUD + `authoritative_ack` refresh) with links to the plan and manual QA file.

## Nits

None.

## Verification

- Ran `godot --headless --import --path . --quit-after 10` from `client/`.
- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_selection_client_test.gd` from `client/` — suite passed.
- I did not run the full manual client/server checklist, but `docs/manual-qa/NEO-24.md` section 4 is the critical end-to-end check to rerun after fixing the movement refresh trigger.