neon-sprawl/docs/reviews/2026-04-17-NEO-23.md

Code review — NEO-23 (targeting API)

Date: 2026-04-17

Scope: Branch NEO-23-e1m3-targetstate-selection-api; git range main...HEAD (working tree clean at review time). Linear NEO-23.

Base: main (local origin/main implied).

Verdict

Approve with nits

Summary

The branch adds the NEO-23 targeting surface under server/NeonSprawl.Server/Game/Targeting/: prototype registry, in-memory per-player lock + sequence, horizontal reach–based validity (soft lock), GET /game/players/{id}/target and POST …/target/select, DTOs aligned with the plan (selectionApplied, optional reasonCode, always-present lockedTargetId), integration tests in the same spirit as interaction APIs, and server README coverage. Behavior matches the written implementation plan for denials (200 + reasonCode when not applied), 400/404 boundaries, clear via omitted/null targetId, and movement-driven validity without auto-clear. dotnet test NeonSprawl.sln passes (55 tests).

Documentation checked

Path	Result
docs/plans/NEO-23-implementation-plan.md (branch)	Matches — routes, DTO names as documented (PlayerTargetStateResponse, TargetSelectRequest/Response), soft lock, sequence rules, README scope, and acceptance bullets align with the code under review.
docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md	Matches (post–follow-up) — Summary Status, Implementation snapshot, and NEO-23 Game/Targeting/ + README pointers reflect the JSON v1 spike; later contracts remain explicitly open.
docs/decomposition/modules/module_dependency_register.md	Matches (post–follow-up) — E1.M3 In Progress; note text covers NEO-23 partial slice vs remaining contracts.
docs/decomposition/modules/documentation_and_implementation_alignment.md	Matches (post–follow-up) — E1.M3 row register + snapshot + plans cell aligned with NEO-23 merge and follow-on Linear issues.
docs/decomposition/modules/contracts.md (versioned JSON spike discipline)	Matches — explicit schemaVersion, throwaway JSON over HTTP, consistent with other prototype routes.

Register / tracking table: After merge, should update module_dependency_register.md and documentation_and_implementation_alignment.md (and optionally the E1.M3 module snapshot) so status and inventory match the new Game/Targeting/ + README section. Done on branch (same PR window as NEO-23).

Blocking issues

(none)

Suggestions

1. Doc alignment in merge train: Update module_dependency_register.md, documentation_and_implementation_alignment.md, and the Implementation snapshot in E1_M3_InteractionAndTargetingLayer.md so E1.M3 is not described as having zero implementation once NEO-23 lands (per this repo’s own alignment policy). A follow-up doc-only PR is acceptable if called out explicitly for the merge. Done.

2. Test gap — GET 404: PostSelect_ShouldReturnNotFound_WhenPlayerUnknown covers unknown player on POST. Adding a parallel test for GET /game/players/{id}/target would lock the same contract as position/interact read paths. Done.

3. PlayerTargetStateReaderTests depth: The plan’s optional table called out inclusive-radius boundary cases (similar to HorizontalReachTests). Current unit tests cover null lock and unknown registry id only; adding one boundary assertion would better match the plan’s optional coverage intent. Done.

Nits

• Nit: Wire type name PlayerTargetStateResponse vs illustrative doc name TargetState is already explained in the NEO-23 plan and README; no change required if PR text points readers there once.

Verification

• dotnet test NeonSprawl.sln (author ran: all passed).
• Manual: curl examples from server/README.md Targeting (NEO-23) against a running server (optional smoke).