# Code review: NEO-11 client path-follow (`NEO-11-client-path-follow`)

**Date:** 2026-04-05  
**Scope:** Branch `NEO-11-client-path-follow` vs `origin/main` (full NEO-11 slice: client nav, wiring, docs, small server remark).  
**Base:** `origin/main`

**Follow-up:** Design and docs were aligned after the initial review; blocking items and suggestions below are **done** unless marked otherwise.

## Verdict

**Approve** — original **Request changes** (plan/README vs obstacle wording) and review **suggestions** 1–3 are **resolved** in follow-up commits. Optional **nit** (rename `DESCEND_GOAL_Y_MARGIN`) remains open.

## Summary

The branch adds **`NavigationRegion3D`** / **`NavigationAgent3D`**, sync nav bake in **`main.gd`**, boot snap vs post-move nav goal via **`PositionAuthorityClient`**’s second signal argument, and horizontal path-follow in **`player.gd`**. **`ground_pick.gd`** gains stepped walkable picking for steep surfaces. Server change is documentation-only on **`MoveCommandValidation`** (vertical step remark).

**`player.gd`** intentionally skips nav waypoints when **`_auth_walk_goal.y < global_position.y - DESCEND_GOAL_Y_MARGIN`**, so for typical picks (floor **Y** vs mid-capsule origin) the client often **bee-lines in xz** toward the goal. That matches the **locked prototype tradeoff** (smooth bumps; multi-click around obstacles when needed).

**Idle optimization:** skipping **`move_and_slide()`** when there is no walk goal, velocity is zero, and **`is_on_floor()`** is a reasonable jitter mitigation; residual vibration is tracked in **[NEO-14](https://linear.app/neon-sprawl/issue/NEO-14)** (Tech Debt).

## Documentation checked

| Document | Result (at closure) |
|----------|---------------------|
| `docs/plans/NEO-11-implementation-plan.md` | **Matches** after **Locked — prototype movement tradeoff**, updated AC, tests, and **NEO-14** link. |
| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **Matches** — snapshot notes single-click obstacle detours not guaranteed. |
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — server owns **`PositionState`**; client presentation path valid. |
| `docs/decomposition/modules/module_dependency_register.md` / `documentation_and_implementation_alignment.md` | **N/A** deep diff. |
| `client/README.md` | **Matches** — tradeoff, manual steps, **Known issue** + **NEO-14** link. |
| `server/README.md` | **Matches** — client nav presentation; detours not guaranteed on one click. |

## Blocking issues

**All resolved.**

1. ~~**Plan and README vs behavior:**~~ **Done.** Plan and **`client/README.md`** now state the **locked tradeoff** (descend bypass → xz bee-line; navmesh detours not guaranteed on one click; multi-click OK). AC checklist and manual QA updated accordingly.

## Suggestions

**All done.**

1. ~~**`DESCEND_GOAL_Y_MARGIN`:**~~ **Done.** Comment added in `client/scripts/player.gd` (surface pick Y vs mid-capsule; bumps vs obstacles).
2. ~~**`_skip_move_when_idle_on_floor`:**~~ **Done.** Doc comment above the helper (`is_on_floor()` / last `move_and_slide()`; static floors).
3. ~~**NEO-14:**~~ **Done.** Out of NEO-11 scope in plan; **[NEO-14](https://linear.app/neon-sprawl/issue/NEO-14)** linked from **`NEO-11-implementation-plan.md`** and **`client/README.md`**.

## Nits

- **Nit (optional):** Consider renaming **`DESCEND_GOAL_Y_MARGIN`** to something that signals “auth pick Y vs body origin” to reduce future “why do we always bee-line?” confusion. **Not done** — purely optional.

## Verification

- `cd server/NeonSprawl.Server && dotnet test` (no logic change expected, sanity).
- Godot **F5** with server: boot snap; click floor; bump flow; gray **Obstacle** may need **multiple clicks** per docs.
- Confirm NEO-10 reject targets still show rejection UX (`reasonCode` / label).
- `gdlint` / `gdformat` on touched GDScript if CI does not already run on the branch.