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

**Post-merge UX follow-up #5 (2026-04-21):** user reported "targeted A, moved out of A range and into B range — Tab gets denied incorrectly." HUD confirmed `Δ=0.00m` with `age≈7964ms` and beta distance `2.04 m / 6.0 (in)`, so client and server *agreed* on the player's stopped position and beta was clearly in range — but the select POST still denied. Root cause: follow-up #2's freshness kick fires a `move-stream` POST and the select POST on separate HTTP connections; nothing guarantees the server processes the stream first, so the select's range check can still race against a stale stored snap (or, for that matter, an ASP.NET request executing on another thread while the stream is still in flight). Fix: extend the NEO-23 `TargetSelectRequest` v1 with an **optional** `positionHint: {x,y,z}`. When present, `TargetingApi` uses the hint for both the radius check and the `validity` field in the echoed `targetState` (so an accepted select never reports a stale `out_of_range`). The hint is advisory — it does **not** write to `IPositionStateStore`, so `move-stream` remains the sole position-write path. `TargetSelectionClient` attaches the hint automatically whenever `set_freshness_kick(...)` is called (same player ref), keeping the headless unit suites that skip that wiring on the unchanged code path. The freshness kick stays in place to keep the stored snap fresh for subsequent validity GETs, but is no longer load-bearing for denial correctness. Tests: `test_select_post_includes_position_hint_when_player_wired` (client), `PostSelect_ShouldUsePositionHint_WhenStoredSnapIsStale` + `PostSelect_ShouldDenyOutOfRange_WhenHintIsFar` + `PostSelect_PositionHint_ShouldNotWriteToPositionStore` (server). Plan Decision 8 + NEO-23 contract notes updated.

**Post-merge UX follow-up #4 (2026-04-21):** user asked for a single shared radius across all prototype targets and overlapping rings in the test scene so Tab-flipping is observable without walking. `PrototypeTargetRegistry` now exposes a `SharedLockRadius = 6.0` constant used by both anchors; alpha moved to `(-3, 0.5, -3)` and beta to `(3, 0, 3)` so the rings overlap ~3.5 m wide centered at origin. Spawn at `(-5, -5)` keeps alpha in range / beta out (existing soft-lock denial QA path untouched); walking to origin flips both rings in-range and the range-aware Tab picker swaps alpha ↔ beta on every press. `PlayerTargetStateReaderTests` updated to the new `(3, -3)` alpha boundary plus a new origin-in-both test. `prototype_target_constants.gd` mirrors the server's `SHARED_LOCK_RADIUS`. Picker tightened to **skip the currently-locked id** — so with only one target in range and that target already locked, Tab falls back to the cycle and the server denies, keeping NEO-23's "Tab = visible denial when no swap is possible" rule. New tests: `test_tab_from_locked_alpha_at_origin_swaps_to_beta_when_both_in_range`, `test_tab_when_only_current_lock_is_in_range_falls_back_to_cycle_for_denial`. Plan Decision 7.

**Post-merge UX follow-up #3 (2026-04-21):** user reported "tab here does not acquire target" while standing 2.41 m from beta (well inside the 4 m ring) with `Δ=0.00m age=2357ms` — i.e. client and server agreed on position. Server denied with `out_of_range`. Root cause was the Tab cycle: with no lock held, `request_tab_next()` returned `ORDERED_IDS[0]` = `prototype_target_alpha`, which was 17.90 m away. The user was visibly next to `beta` but Tab was always trying `alpha` first. Fix: `PrototypeTargetConstants.next_in_range_id_after(current, world)` walks the cycle and returns the first anchor whose client-side horizontal distance ≤ radius; `TargetSelectionClient.request_tab_next()` prefers that pick when the player ref is wired and falls back to `next_id_after(current)` when nothing is in range (so the server still owns the denial reason). Server contract untouched. Tests: `test_tab_from_no_lock_skips_out_of_range_and_picks_beta`, `test_tab_from_no_lock_with_nothing_in_range_falls_back_to_first_id`, `test_tab_without_player_ref_still_uses_plain_cycle_order`.

**Post-merge UX follow-up #2 (2026-04-21):** user reported "when in range of beta, pressing Tab says denied" — HUD distance line confirmed `prototype_target_beta: 0.58 m / 4.0 (in)` but server denied with `out_of_range` (and `prototype_target_alpha: 18.95 m` — almost exactly the spawn-to-current distance, strongly suggesting the server's stored position was still near spawn). Root cause is a race between the `move-stream` 20 Hz sampler and the target-select POST: a Tab press right after you stop moving validates against the last sampled position, which can trail the visible capsule. Two changes:

- `PlayerPositionLabel` now shows the server-acknowledged position + horizontal Δ + ack age (cached from `PositionAuthorityClient.authoritative_ack` via a new `_on_authoritative_ack_for_hud` handler). Makes the divergence directly visible in future repro.
- `TargetSelectionClient` exposes `set_freshness_kick(authority, player)`; `main.gd` wires it so every `POST /target/select` is immediately preceded by a `submit_stream_targets([player.global_position])` nudge on the authority. The select POST does not wait for the stream response, but the kick shrinks the race window enough in practice that "stop, Tab, denied" stops firing. Tests cover both the kick happening (`test_select_post_kicks_freshness_stream_before_posting`) and no regression when the wiring is absent (`test_select_post_without_freshness_wiring_is_a_noop`).

Server contract (NEO-23) untouched; this is pure client-side.

**Post-merge UX follow-up #1 (2026-04-21):** user reported "targeting result feels very inconsistent and doesn't seem to matter whether i'm within range of the console or not." Root cause was not a correctness bug — the `PrototypeTargetRegistry` anchors (`alpha (-5,-5)` r=8, `beta (8,8)` r=4) have **no visible scene meshes**, so players were reasoning about targeting against the `PrototypeTerminal` box at origin (which belongs to NEO-9 interaction, not NEO-23/24 targeting). Added:

- `client/scripts/prototype_target_markers.gd` — spawns an unshaded colored mast + flat radius ring per anchor (mirrors the server registry via `prototype_target_constants.gd`).
- Per-anchor distance readout on `UICanvas/TargetLockLabel`: `<id>: <d> m / <radius> (in|out)` computed client-side from the live capsule position. Labeled explicitly as a lag diagnostic — if `Validity: ok` shows while the distance line reads `(out)`, the server's position snapshot is trailing the visible capsule and the next `authoritative_ack` refresh reconciles it.
- Updated `client/README.md` and `docs/manual-qa/NEO-24.md` (Section 1 boot state and Section 4 locomotion check) to describe the markers and distance lines.

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

Follow-up verification confirms the earlier review findings were addressed. The movement-triggered soft-lock refresh is now reachable during normal locomotion via `PositionAuthorityClient.authoritative_ack`, the new integration test covers the real authority-to-target wiring, and the E1.M3 implementation-alignment row was updated to reflect NEO-24 landing.

## Documentation checked

- `docs/plans/NEO-24-implementation-plan.md` — **matches**. Decision 2 now uses `PositionAuthorityClient.authoritative_ack`, and Decision 3 records why the new signal was added.
- `docs/manual-qa/NEO-24.md` — **matches**. Section 4’s locomotion-driven refresh path is now implemented by the new authority ack signal and target refresh wiring.
- `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` — **matches**. The `E1.M3` tracking row now reflects NEO-24 landed (HUD + `authoritative_ack` refresh) and points back to the plan/manual QA docs.

## 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/position_authority_client_test.gd` from `client/` — suite passed, including the new `authoritative_ack` assertions.
- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_selection_client_test.gd` from `client/` — suite passed, including the renamed ack-based refresh tests.
- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_refresh_on_locomotion_test.gd` from `client/` — integration suite passed, confirming `move-stream` 200 triggers exactly one throttled target refresh while locked and none while unlocked.
- Read diagnostics for the touched client/scripts, client/test, plan, and review files — no linter errors found.
- I did not run the full manual client/server checklist; `docs/manual-qa/NEO-24.md` section 4 remains the highest-value end-to-end sanity check if you want one more runtime pass before merge.