From 8c9db789d051470fca8df9e138d07b5533fc28eb Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 17 Apr 2026 21:04:01 -0400 Subject: [PATCH] NEO-23: lock JSON shape, GET envelope, selectionApplied, soft lock in plan --- docs/plans/NEO-23-implementation-plan.md | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/docs/plans/NEO-23-implementation-plan.md b/docs/plans/NEO-23-implementation-plan.md index 463b24e..2ebf334 100644 --- a/docs/plans/NEO-23-implementation-plan.md +++ b/docs/plans/NEO-23-implementation-plan.md @@ -19,7 +19,7 @@ - **Stub eligible combat targets:** static registry (≥ **2** distinct ids with world anchors + horizontal **lock radius**) so [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24) can assume deterministic cycling without expanding scope here beyond “exists and validates.” - **Selection intent:** `POST` with v1 body; **clear lock** when `targetId` is **omitted or JSON `null`** (explicit rule in XML + README). **Trim** non-null ids; **whitespace-only** after trim → **400** (same spirit as empty `interactableId` on NEO-9). -- **Authoritative read model:** `TargetState` includes **locked target id** (empty when none), **`validity`** derived from **current** `PositionState` vs stub anchor + radius using **`HorizontalReach`** (X/Z only, inclusive boundary — same policy as NEO-9), and **`sequence`** bumped only when the **persisted lock id** changes (aligns with “optional sequence” in the module’s illustrative table). +- **Authoritative read model:** `TargetState` includes **`lockedTargetId`** (**always** serialized; JSON **`null`** when no lock — never omit the key), **`validity`** derived from **current** `PositionState` vs stub anchor + radius using **`HorizontalReach`** (X/Z only, inclusive boundary — same policy as NEO-9), and **`sequence`** bumped only when the **persisted lock id** changes (aligns with “optional sequence” in the module’s illustrative table). - **`GET`** current `TargetState` for the same player id normalization rules as position/interact APIs. - **Unit + integration tests** mirroring `InteractionApiTests` patterns; **server README** subsection documenting routes, curl, and `reasonCode` table. @@ -32,7 +32,7 @@ ## Acceptance criteria checklist - [ ] **Server source of truth:** Client cannot persist a lock the server rejects; every **`POST`** response includes the **authoritative** `targetState` after evaluation (success or denial). -- [ ] **Denials:** **`selectionApplied`: `false`** (or equivalent boolean agreed in implementation) ⇒ **required non-empty `reasonCode`**; **HTTP 200** for logical denials; **400** / **404** rules match NEO-9 spirit (unknown player → **404** before body; malformed v1 → **400**). +- [ ] **Denials:** **`selectionApplied`: `false`** ⇒ **required non-empty `reasonCode`**; **HTTP 200** for logical denials; **400** / **404** rules match NEO-9 spirit (unknown player → **404** before body; malformed v1 → **400**). - [ ] **Movement coupling:** After **`POST /move`** or **`POST /move-stream`**, the same **`GET`**/ **`POST`** response reflects updated **`validity`** (e.g. walk out of stub radius ⇒ `out_of_range` on read without requiring a new select). - [ ] **Contract names** in README + XML match the **illustrative** E1.M3 table ([Key contracts](../decomposition/modules/E1_M3_InteractionAndTargetingLayer.md#key-contracts)); any renames are visible in this plan and in PR text, not only in code. @@ -44,21 +44,26 @@ 3. **Mutable per-player lock** (e.g. `IPlayerTargetLockStore` + `InMemoryPlayerTargetLockStore`): stores **nullable locked target id** + **monotonic `sequence`** incremented **only** when the lock id changes (clear or swap). Thread-safe in-memory singleton for prototype (same lifetime pattern as `InMemoryPositionStateStore`). -4. **`TargetState` DTO (v1 JSON):** `schemaVersion`, `lockedTargetId` (null or absent in JSON when none — pick one style and document), `validity` enum serialized as stable strings (`ok`, `out_of_range`, `invalid_target`, `none` for “no lock”), `sequence` (int). **Validity** is **computed** on each read: no lock ⇒ `none`; unknown id in store (should not happen) ⇒ `invalid_target`; else horizontal distance vs **`lockRadius`**. +4. **`TargetState` fields (v1 JSON, shared by GET body and nested POST echo):** `schemaVersion`, `playerId` (echo), **`lockedTargetId`** (string or JSON **`null`** — key **always present**), `validity` (`ok`, `out_of_range`, `invalid_target`, `none`), `sequence` (int). **Validity** is **computed** on each read: no lock ⇒ `validity: none` and `lockedTargetId: null`; unknown id in store (should not happen) ⇒ `invalid_target`; else horizontal distance vs **`lockRadius`**. -5. **`TargetSelectRequest` / `TargetSelectResponse` v1:** Request: `schemaVersion`, optional `targetId`. Response: `schemaVersion`, `selectionApplied` (bool), **`targetState`** (always), **`reasonCode`** (required when `selectionApplied` is false, omitted or null when true — mirror `InteractionResponse` / `reasonCode` rules). Successful **clear** ⇒ `selectionApplied: true`, `targetState` shows no lock. +5. **`TargetSelectRequest` / `TargetSelectResponse` v1:** Request: `schemaVersion`, optional `targetId`. Response: `schemaVersion`, **`selectionApplied`** (bool), **`targetState`** (object matching the GET field set above — always present), **`reasonCode`** (required when `selectionApplied` is `false`, omitted when `true` — same spirit as `InteractionResponse`). Successful **clear** ⇒ `selectionApplied: true`, `targetState` shows `lockedTargetId: null`, `validity: none`. 6. **Routes** (exact paths in README; suggested): - - `GET /game/players/{id}/target` → `TargetState` envelope (or bare `TargetState` if we prefer — **decide in PR** and keep README consistent). + - `GET /game/players/{id}/target` → **envelope** aligned with **`PositionStateResponse`**: top-level `schemaVersion`, `playerId`, plus lock fields (`lockedTargetId`, `validity`, `sequence`) so read and move APIs share the same “version + player echo + payload” pattern. - `POST /game/players/{id}/target/select` → body `TargetSelectRequest`, response `TargetSelectResponse`. 7. **`Program.cs`:** register store + `app.MapTargetingApi()` (static extension parallel to `MapInteractionApi`). 8. **README:** new subsection **Targeting (NEO-23)** with curl, status table, `reasonCode` list (`unknown_target`, `out_of_range`, optional `invalid_target`, …). -**Decisions (pre-implementation)** +**Decisions (locked — stakeholder 2026-04-18)** -- **Validity vs persistence:** Lock **id** persists until cleared or swapped; **`validity`** reflects live range so GET after move shows `out_of_range` without auto-clearing (matches “soft lock” until NEO-24 reconciliation policy). *If product prefers auto-clear when out of range, update this plan in the PR.* +| Topic | Choice | +|--------|--------| +| **`lockedTargetId` when no lock** | Property **always present**; value JSON **`null`** (never omit the key). | +| **GET body shape** | **Envelope** like `PositionStateResponse`: `schemaVersion`, `playerId`, then lock fields (easier evolution and parity with position GET). | +| **POST success flag** | **`selectionApplied`** (distinct from interact’s `allowed` — avoids “permission” ambiguity). | +| **Out of range vs stored lock** | **Soft lock:** persisted lock **id** unchanged until clear/swap; **`validity`** becomes `out_of_range` when the player moves away until they clear or select again. **No auto-clear** on read or timer in NEO-23. | ## Files to add @@ -90,5 +95,5 @@ No automated Godot harness for NEO-23; manual verification is optional HTTP clie ## Open questions / risks -- **Auto-clear on prolonged `out_of_range`:** deferred; current plan keeps id until explicit clear/swap (document for NEO-24 UX). +- **NEO-24 UX:** document that UI may show **out of range** while **`lockedTargetId`** stays set until an explicit clear or a successful swap (soft lock). - **Postgres vs in-memory target store:** prototype lock store is in-memory only for NEO-23; multi-instance deployment would need shared storage — out of scope until infra story exists.