diff --git a/docs/reviews/2026-04-17-NEO-23.md b/docs/reviews/2026-04-17-NEO-23.md new file mode 100644 index 0000000..5d68044 --- /dev/null +++ b/docs/reviews/2026-04-17-NEO-23.md @@ -0,0 +1,48 @@ +# 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` | **Partially matches** — authority, horizontal reach policy, denial `reasonCode` pattern, and “intent vs state” story align with the implementation. The module **Implementation snapshot** still states that versioned `TargetState` wire is not in the repo; after merge, that paragraph (and the Summary **Status** if tied to the register) should reflect NEO-23’s JSON v1 spike or stay accurate with “partial / prototype only” wording. | +| `docs/decomposition/modules/module_dependency_register.md` | **Conflicts (post-merge drift risk)** — E1.M3 row remains **Planned** with text implying no first-party targeting contracts in code. This branch implements a deliberate JSON v1 slice. Per `documentation_and_implementation_alignment.md`, the register (and alignment table) should move to **In Progress** (or similar) when this merges, unless intentionally deferred in a follow-up PR called out in the PR description. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Conflicts (post-merge drift risk)** — E1.M3 snapshot row still says no E1.M3-named contracts in code and register unchanged until NEO-23 merges; updating this file (and the register) in the same merge window avoids silent doc/code disagreement. | +| `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. + +## 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. + +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. + +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. + +## 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).