# Code review: NEO-7 (MoveCommand → PositionState)

**Date:** 2026-03-30  
**Scope:** Branch `NEO-7-move-command-position-state` vs `origin/main` (working tree / uncommitted changes included at review time).  
**Issue:** [NEO-7](https://linear.app/neon-sprawl/issue/NEO-7) — E1.M1: Submit MoveCommand → server updates PositionState  
**Base:** `origin/main`

## Verdict

**Approve** (after removing machine-specific `launchSettings.json` entries and refreshing decomposition docs; see below).

## Summary

The branch delivers NEO-7 end-to-end: **`POST /game/players/{id}/move`** with versioned **`MoveCommandRequest`**, thread-safe snap + **`sequence`** in **`InMemoryPositionStateStore`**, **`PositionStateResponse`** on success, integration tests under **`Game/PositionState/`** with **`MethodName_ShouldExpectedOutcome_WhenScenario`** naming and AAA layout, and a Godot client split into **`ground_pick.gd`**, **`position_authority_client.gd`**, and a thin **`main.gd`**. **`UseAppHost=false`** for Debug avoids Linux apphost probing **`/usr/share/dotnet`** when .NET 10 lives under **`~/.dotnet`**. Overall risk is **low** for a prototype slice; there is **no auth** on move (acceptable for the story). Repo rules and READMEs were extended for tests, Godot layout, and Linux/Rider notes.

## Documentation checked

| Document | Result |
|----------|--------|
| `docs/plans/NEO-7-implementation-plan.md` | **Matches** — endpoint, snap rule, client split, tests, README expectations align with implementation. |
| `docs/plans/NEO-6-implementation-plan.md` | **Partially matches** — PR table row for **`sequence`** was stale (“v1 is 0” only); **updated** in this review pass to reference NEO-7 semantics. |
| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **Partially matched → updated** — snapshot still said “Not yet: MoveCommand”; **updated** to reflect NEO-7. |
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matched → updated** — E1.M1 row still said no MoveCommand/client sync; **updated**. |
| `docs/decomposition/modules/module_dependency_register.md` | **N/A** — no row text change required for this review beyond alignment table (register Status unchanged **In Progress**). |
| `docs/decomposition/modules/contracts.md` | **Matches** — JSON spike / throwaway until protobuf acknowledged in code comments. |
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — server owns **`PositionState`**; client sends intent via HTTP. |

## Blocking issues

1. **`launchSettings.json` contained user-local `DOTNET_ROOT` and `PATH`** (`/home/don/.dotnet`, …) — unsuitable for shared repo and redundant with **Debug `UseAppHost=false`**. **Removed** as part of closing this review so the branch is safe to merge.

*(No remaining blockers after that change.)*

## Suggestions

1. **Linear / plan checkboxes** — `docs/plans/NEO-7-implementation-plan.md` acceptance bullets are still unchecked; tick them when you close NEO-7 or paste PR proof on the Linear issue.
2. **`PositionAuthorityClient` boot failure** — if the initial GET fails, the client fails quietly (warnings only); acceptable for the spike; consider a single user-visible error later.
3. **Observability** — server does not log failed moves or bad payloads; fine for prototype; add when debugging multiplayer.

## Nits

*(None remaining.)*

**Resolved after review:**

- NEO-7 plan now states explicitly that scripts are **split per client policy**, and **`HTTPRequest`** is created in **`position_authority_client._ready`** (not placed in **`main.tscn`**).
- **`PositionAuthorityClient`** reuses **`_request_get()`** for the post-POST verify GET instead of duplicating the position URL.

## Verification

- `dotnet build NeonSprawl.sln -c Release`
- `dotnet test NeonSprawl.sln -c Release`
- Godot: `godot --headless --path client --quit-after 5` (with Godot 4.6 on `PATH`)
- Manual: server `dotnet run`, client F5, click floor → snap to server position
