NEON-16: code review — idle jitter follow-up branch

docs/reviews/2026-04-10-NEON-16-followup.md: Request changes — two
blocking gdlint issues (export var order, line-length on
vertical_arrival_error) plus suggestions and nits.

docs/reviews/2026-04-10-NEON-16-followup.md

@@ -0,0 +1,126 @@
+# Code review — NEON-16 idle jitter follow-up (Godot client)
+
+**Date:** 2026-04-10
+**Scope:** Branch `NEON-16-idle-jitter-followup` vs `origin/main` @ `2aff663`. Issue **NEON-16** (re-opened). No PR URL supplied; working-tree diff reviewed.
+**Base:** `origin/main` @ `2aff663` (post-NEON-30 merge)
+
+---
+
+## Verdict
+
+**Request changes** — two CI-blocking lint issues must be fixed before merge.
+
+---
+
+## Summary
+
+This branch re-opens NEON-16 to eliminate residual idle x/z drift on flat ground that became worse after the NEON-25/26/27/30 follow-camera work. The fix is localized entirely to `player.gd`: a stable-flat-support fast-path that skips the corrective `move_and_slide()` / rim-escape loop, an explicit x/z anchor to clamp away residual horizontal creep while the player is classified as stable, a correction-budget cap (`IDLE_MANUAL_CORRECTION_MAX_TICKS`) to prevent edge contacts from running the loop indefinitely, and a capsule-feet reference point for both goal-arrival and descend-bypass checks. `_snap_capsule_upright()` gains an early-out when the basis is already identity. Debug tracing is gated behind `@export var debug_idle_trace`. Thirteen new unit tests cover the new static helpers and state invariants. Server contracts are untouched; risk is client-only.
+
+---
+
+## Documentation checked
+
+| Document | Result |
+|----------|--------|
+| `docs/plans/NEON-16-implementation-plan.md` | **Matches** — Resolution section updated, Decisions table present, Files to modify and Tests sections reflect what shipped. AC boxes not yet checked (branch not merged). |
+| `docs/reviews/2026-04-05-NEON-16.md` | **Matches** — existing review is the prior-story approval; this is a follow-up branch against the same issue. No bullets to strike through in the old file from this diff. |
+| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **Matches** — change is client movement-loop polish; server authority (`PositionState`, `MoveCommand`) and all contracts unchanged. Module status ("Ready — prototype") does not need updating for this fix. |
+| `docs/decomposition/modules/module_dependency_register.md` | **N/A** — no new modules or dependencies introduced. |
+| `docs/decomposition/modules/client_server_authority.md` | **N/A** — no authority boundary changes. |
+| `client/README.md` | **Matches** — README updated in both the tradeoff note (descend-bypass now uses feet height) and the Idle stability paragraph (flat-idle hardening, x/z anchor, identity-basis short-circuit). Manual verification step 6 updated to check x/z specifically on flat floor then on bumps. |
+
+---
+
+## Blocking issues
+
+1. **`@export var debug_idle_trace` declaration order violates `gdlint` `class-definitions-order` (CI will fail).**
+
+   `@export` vars must appear *before* public and private vars. Currently `debug_idle_trace` is on line 56, sandwiched between private vars that start on line 49:
+
+   ```gdscript
+   # current (wrong order)
+   var _idle_anchor_active: bool = false
+   var _idle_anchor_xz: Vector2 = Vector2.ZERO
+   var _idle_manual_correction_ticks: int = 0
+   @export var debug_idle_trace: bool = false   # ← must move up
+   var _debug_last_idle_xz: Vector2 = Vector2.INF
+   ...
+   ```
+
+   Move `@export var debug_idle_trace` to immediately after the last `const` block (before any `var` declarations), or at minimum before the first `var _…` line, so it precedes all non-export vars per gdlint order: `const → @export vars → public vars → private vars`.
+
+2. **`vertical_arrival_error` function signature is 109 characters — exceeds the 100-char `gdlint` `max-line-length` limit (CI will fail).**
+
+   ```gdscript
+   # 109 chars — too long
+   static func vertical_arrival_error(goal_y: float, body_origin_y: float, capsule_half_height: float) -> float:
+   ```
+
+   Wrap the parameter list:
+
+   ```gdscript
+   static func vertical_arrival_error(
+       goal_y: float, body_origin_y: float, capsule_half_height: float
+   ) -> float:
+   ```
+
+---
+
+## Suggestions
+
+1. **`set_notify_transform(true)` is always enabled regardless of `debug_idle_trace`.**
+   `_notification(NOTIFICATION_TRANSFORM_CHANGED)` is invoked by the engine on every transform write (physics, snap, anchor hold) in both debug and release builds. The callback itself bails quickly when `debug_idle_trace` is false, so functional correctness is fine, but the notification dispatch fires unconditionally at 120 TPS. Consider gating the registration:
+
+   ```gdscript
+   if debug_idle_trace:
+       set_notify_transform(true)
+   ```
+
+   Or enable it only in debug builds (`OS.is_debug_build()`). At two bumps and one player this overhead is negligible, but the pattern will scale poorly if the per-node notification cost is revisited later.
+
+2. **`idle_support_is_stable`: the `slide_normals` parameter is silently ignored via `@warning_ignore` + no-op self-assignment.**
+   The intent (reserve for future extra-contact analysis) is reasonable, but the current pattern is easy to misread:
+
+   ```gdscript
+   static func idle_support_is_stable(
+       on_floor: bool, floor_normal: Vector3, slide_normals: Array[Vector3], loose_ticks: int
+   ) -> bool:
+       @warning_ignore("unused_parameter")
+       slide_normals = slide_normals   # ← does nothing
+       ...
+   ```
+
+   Either drop the parameter until it is used (callers pass `_current_slide_normals()` which allocates a new array each call), or add a `##` doc comment explicitly noting "reserved — not yet evaluated". If the parameter is intentionally kept for API-stability reasons, a `@warning_ignore` on the declaration rather than a no-op assignment is cleaner in GDScript.
+
+3. **`set_authoritative_nav_goal` clears `_idle_anchor_active` but has no test coverage for that invariant.**
+   The new test `test_snap_to_server_clears_idle_anchor` covers `snap_to_server`, and `test_clear_nav_goal_clears_velocity_and_nav_target` checks `_idle_anchor_active`. The same assertion for `set_authoritative_nav_goal` is absent; a one-liner would close the gap.
+
+4. **`_floor_angle_loose_ticks` is never decremented in the "budgeted" anchor path.**
+   When `_idle_manual_correction_ticks >= IDLE_MANUAL_CORRECTION_MAX_TICKS` the code holds the anchor and returns early — skipping the decrement that only lives in the corrective branch. In practice the player is already anchored so the loose angle has no visible effect, but arrival with bumps could hold `FLOOR_MAX_ANGLE_MOVING_DEG` longer than intended if the budget is hit quickly. Consider adding a decrement in the budgeted path:
+
+   ```gdscript
+   if _floor_angle_loose_ticks > 0:
+       _floor_angle_loose_ticks -= 1
+   ```
+
+---
+
+## Nits
+
+- `_debug_trace_frame` increments every `_physics_process` tick unconditionally (including during walking). This is fine for a debug tool; just be aware it will wrap around to `int` overflow after very long sessions (non-issue at 120 TPS unless someone leaves it running for ~200 days).
+- The `@warning_ignore("unused_parameter")` annotation placement inside the function body (rather than on the parameter/declaration) is valid GDScript but unusual. See suggestion 2.
+- `_maybe_idle_bump_proximity_escape`: when `test_move` blocks an escape step the function returns `false` immediately, stopping any further bump scan. This was the pre-existing behavior and is unchanged; just noting it persists in the new `bool`-return form.
+
+---
+
+## Verification
+
+Per `NEON-16-implementation-plan.md` and updated `client/README.md` step 6:
+
+1. Fix the two blocking lint issues and run `gdlint client/scripts client/test` + `gdformat --check client/scripts client/test` locally to confirm clean.
+2. Run headless GdUnit: `godot --headless --script res://addons/gdUnit4/bin/GdUnitCmdTool.gd --add res://test/` — expect **52+ tests, 0 errors, 0 failures**.
+3. Run `main.tscn` with server: stand idle **10+ s** at spawn on flat ground; confirm no visible vibration and no x/z drift.
+4. Walk to a destination, stop, idle again — same stability check.
+5. Idle on the random green bumps for **10+ s** — capsule should not drift radially or oscillate.
+6. Click-move including stepped bumps (NEON-7 props); confirm walking, arrival, and `snap_to_server` on cold boot are unaffected.
+7. NEON-30 occluder click-through sanity (click behind an occluder body — ground pick still registers).