don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-03-30-NEON-4.md

3.9 KiB
Raw Blame History

Code review: NEON-4 (MoveCommand → PositionState)

Date: 2026-03-30
Scope: Branch NEON-4-move-command-position-state vs origin/main (working tree / uncommitted changes included at review time).
Issue: NEON-4 — 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 NEON-4 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/NEON-4-implementation-plan.md Matches — endpoint, snap rule, client split, tests, README expectations align with implementation.
docs/plans/NEON-3-implementation-plan.md Partially matches — PR table row for sequence was stale (“v1 is 0” only); updated in this review pass to reference NEON-4 semantics.
docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md Partially matched → updated — snapshot still said “Not yet: MoveCommand”; updated to reflect NEON-4.
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. Jira / plan checkboxes — docs/plans/NEON-4-implementation-plan.md acceptance bullets are still unchecked; tick them when you close NEON-4 or paste PR proof in Jira.
  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:

  • NEON-4 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