49 lines
4.4 KiB
Markdown
49 lines
4.4 KiB
Markdown
# 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).
|