diff --git a/.cursor/rules/commit-and-review.md b/.cursor/rules/commit-and-review.md index c9e4a2b..ba0e248 100644 --- a/.cursor/rules/commit-and-review.md +++ b/.cursor/rules/commit-and-review.md @@ -1,5 +1,5 @@ --- -description: Commits at agent discretion; push only when user asks; gh pr create with NEO-* title when opening PRs; PR text without tool boilerplate +description: Commits at agent discretion; push only when user asks; GitHub MCP create_pull_request with NEO-* title when opening PRs; PR text without tool boilerplate alwaysApply: true --- @@ -25,12 +25,14 @@ alwaysApply: true When the user asks to **open**, **create**, or **file** a pull request (or clearly wants one opened for the current branch): -1. **Use the GitHub CLI** — Run **`gh pr create`** (not only a browser **`/pull/new/...`** link). That way you control the **title** and avoid GitHub’s default, which title-cases the branch name and turns **`NEO-25`** into **`Neo 25`**. -2. **Title** — **`--title`** must **begin** with the **Linear issue key** in canonical form: **`NEO-123:`** (see [linear-git-naming](linear-git-naming.md): uppercase team prefix, hyphen, issue number), then a short human summary. Example: **`NEO-25: InteractableDescriptor list / fetch-driven client`**. -3. **Infer the key** — In order: the **current branch**’s leading segment (e.g. `NEO-25-my-slug` → `NEO-25`); the matching **`docs/plans/{KEY}-implementation-plan.md`** on the branch; Linear MCP / issue in chat; or **ask** the user if ambiguous. -4. **Body** — Pass **`--body`** or **`--body-file`** with scope, how to verify (`dotnet test`, manual QA path, etc.), and pointers to `docs/plans/` when useful. -5. **Upstream** — If **`gh pr create`** fails because the branch is not on `origin`, push first (**only** as allowed under **Never push** above—i.e. the same user message asked to open the PR or explicitly to push). -6. **Fallback** — If **`gh`** is missing or not authenticated (`gh auth status`), tell the user and optionally still give the **`pull/new/...`** URL, with an explicit reminder to **edit the title** so it starts with **`NEO-123:`** (not `Neo 123`). +1. **Use the GitHub MCP** — Call **`create_pull_request`** on the **`user-github`** MCP server (read the tool descriptor under the workspace MCP folder first, same as other MCP tools). Pass **`owner`**, **`repo`**, **`head`**, **`base`**, **`title`**, and **`body`**. You set the **title** explicitly—do not rely only on GitHub’s **Compare & pull request** default, which title-cases the branch and can turn **`NEO-25`** into **`Neo 25`**. +2. **Repository** — Set **`owner`** and **`repo`** from **`git remote get-url origin`** (e.g. `git@github.com:ViPro-Technologies/neon-sprawl.git` → `ViPro-Technologies`, `neon-sprawl`). For this repo they are almost always **`ViPro-Technologies`** / **`neon-sprawl`**. +3. **Branches** — **`head`** is the branch that contains the work (e.g. `NEO-26-selectionevent-surface-debug-consumer`); **`base`** is usually **`main`** unless the user names another target. +4. **Title** — Must **begin** with the **Linear issue key** in canonical form: **`NEO-123:`** (see [linear-git-naming](linear-git-naming.md): uppercase team prefix, hyphen, issue number), then a short human summary. Example: **`NEO-25: InteractableDescriptor list / fetch-driven client`**. +5. **Infer the key** — In order: the **current branch**’s leading segment (e.g. `NEO-25-my-slug` → `NEO-25`); the matching **`docs/plans/{KEY}-implementation-plan.md`** on the branch; Linear MCP / issue in chat; or **ask** the user if ambiguous. +6. **Body** — Pass **`body`** with scope, how to verify (`dotnet test`, manual QA path, etc.), and pointers to `docs/plans/` when useful. When building JSON for the MCP tool, use **real newlines** in markdown strings (not literal `\n` escape sequences), per MCP guidance for this server. +7. **Upstream** — If **`create_pull_request`** fails because **`head`** is not on **`origin`**, push first (**only** as allowed under **Never push** above—i.e. the same user message asked to open the PR or explicitly to push). +8. **Fallback** — If the GitHub MCP is unavailable or the call fails after push, give **`https://github.com/{owner}/{repo}/pull/new/{head}`** and tell the user to set the PR title to **`NEO-123:`** … . They may use **`gh pr create`** locally if they prefer the CLI. ## Commit message format when a Linear issue applies diff --git a/.cursor/rules/git-workflow.md b/.cursor/rules/git-workflow.md index d687da0..9fe4f2e 100644 --- a/.cursor/rules/git-workflow.md +++ b/.cursor/rules/git-workflow.md @@ -17,4 +17,4 @@ When a change mixes documentation and code, treat it as a **code change** (use a After the PR for a story branch is **merged**, clean up locally per [story-end](story-end.md) (`checkout main`, `pull`, `branch -d`). -When the user asks the agent to **open a PR**, use **`gh pr create`** with a **`NEO-*:`** title prefix per [commit-and-review](commit-and-review.md) **Opening a PR when the user asks**—not only a GitHub **Compare & pull request** link (the web UI mangles issue-key casing in the default title). +When the user asks the agent to **open a PR**, use the **GitHub MCP** **`create_pull_request`** tool with a **`NEO-*:`** title prefix per [commit-and-review](commit-and-review.md) **Opening a PR when the user asks**—not only a GitHub **Compare & pull request** link (the web UI mangles issue-key casing in the default title). diff --git a/.cursor/rules/linear-git-naming.md b/.cursor/rules/linear-git-naming.md index 8dca119..cab194e 100644 --- a/.cursor/rules/linear-git-naming.md +++ b/.cursor/rules/linear-git-naming.md @@ -32,4 +32,4 @@ When suggesting or creating a branch for **NEO-5**, use something like **`NEO-5- ## Agent behavior - When creating a **branch** or **commit** for tracked work, **infer the id** from the current story (e.g. user says NEO-5), the branch already in use, or the Linear issue fetched via MCP (`plugin-linear-linear`, e.g. `get_issue` / `list_issues`). -- When the user asks the agent to **open a PR**, the PR **title** must start with that same **`NEO-*:`** form (use **`gh pr create --title`** per [commit-and-review](commit-and-review.md)); do not ship a default web-only title that lowercases the key to **`Neo`**. +- When the user asks the agent to **open a PR**, the PR **title** must start with that same **`NEO-*:`** form (set **`title`** on the GitHub MCP **`create_pull_request`** call per [commit-and-review](commit-and-review.md)); do not ship a default web-only title that lowercases the key to **`Neo`**. diff --git a/.cursor/rules/story-end.md b/.cursor/rules/story-end.md index 4106e73..b992b50 100644 --- a/.cursor/rules/story-end.md +++ b/.cursor/rules/story-end.md @@ -20,7 +20,7 @@ When the user **ends story work** on a ticketed branch—phrases like “end sto ## 2a. When `main` is push-protected -If the user asks to **push** changes that sit on **`main`** and **`git push origin main` fails** (required PR / status checks), create a branch from current `main` (e.g. `chore/…`), **`git push -u origin`** that branch, then open the PR with **`gh pr create`** and a title keyed per [commit-and-review](commit-and-review.md) **Opening a PR when the user asks** (or give the **`pull/new/...`** URL **and** remind them to fix the title to **`NEO-123:`**). **`git reset --hard origin/main`** if you had temporarily advanced local `main` so local `main` matches remote until the PR merges. +If the user asks to **push** changes that sit on **`main`** and **`git push origin main` fails** (required PR / status checks), create a branch from current `main` (e.g. `chore/…`), **`git push -u origin`** that branch, then open the PR with the **GitHub MCP** **`create_pull_request`** tool and a title keyed per [commit-and-review](commit-and-review.md) **Opening a PR when the user asks** (or give the **`pull/new/...`** URL **and** remind them to fix the title to **`NEO-123:`**). **`git reset --hard origin/main`** if you had temporarily advanced local `main` so local `main` matches remote until the PR merges. ## 3. What not to do @@ -58,4 +58,4 @@ Then give any **git** details (branch deleted, `main` ahead, etc.) **after** the - [story-kickoff](story-kickoff.md) — start of story workflow - [git-workflow](git-workflow.md) — branch vs `main` - [linear-git-naming](linear-git-naming.md) — branch names -- [commit-and-review](commit-and-review.md) — push and **`gh pr create`** only when the user requests it; PR title **`NEO-*:`** prefix +- [commit-and-review](commit-and-review.md) — push and **GitHub MCP `create_pull_request`** only when the user requests it; PR title **`NEO-*:`** prefix diff --git a/.cursor/rules/story-kickoff.md b/.cursor/rules/story-kickoff.md index bcb1ab8..2e287c7 100644 --- a/.cursor/rules/story-kickoff.md +++ b/.cursor/rules/story-kickoff.md @@ -67,4 +67,4 @@ These three lists (**files to add**, **files to modify**, **tests**) must **alwa When the story is **done** and merged to `main`, follow [story-end](story-end.md) to return to `main`, pull, and remove the local story branch. -When the user asks to **open or create a PR** for this story, follow [commit-and-review](commit-and-review.md) **Opening a PR when the user asks**: use **`gh pr create`** with a title that **starts with the Linear key** (e.g. **`NEO-25:`**), not GitHub’s default title from the web UI alone. +When the user asks to **open or create a PR** for this story, follow [commit-and-review](commit-and-review.md) **Opening a PR when the user asks**: use the **GitHub MCP** **`create_pull_request`** tool with a title that **starts with the Linear key** (e.g. **`NEO-25:`**), not GitHub’s default title from the web UI alone. diff --git a/AGENTS.md b/AGENTS.md index a9714cc..265308f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,6 +7,6 @@ Optional **personas** for Cursor. Enable by **@ mentioning** the rule in chat or | **Code review** | [`.cursor/rules/code-review-agent.md`](.cursor/rules/code-review-agent.md) | PR / diff / pre-merge review; **always writes** `docs/reviews/YYYY-MM-DD-{slug}.md` with **Documentation checked** vs `docs/plans/` and `docs/decomposition/modules/`; **when suggestions are fixed, strike through those bullets + `Done.` in that file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); short chat pointer | | **Docs review** | [`.cursor/rules/docs-review-agent.md`](.cursor/rules/docs-review-agent.md) | Coherence / links / dev-guide fitness for `docs/` (especially `docs/game-design/` + decomposition); **writes** `docs/reviews/YYYY-MM-DD-{slug}.md`; **when suggestions are fixed, strike through + `Done.` in that file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); use when working in **documents** or @ mention | -Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Planning / implementation decisions** must be written into `docs/plans/` (and related docs)—[`.cursor/rules/planning-implementation-docs.md`](.cursor/rules/planning-implementation-docs.md). **Manual QA checklists** for user-visible ticketed work are generated during implementation at `docs/manual-qa/{KEY}.md` (same rule). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Open PR:** when the user asks, use **`gh pr create`** with a title starting **`NEO-123:`** (same rule file). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **Linear:** on **end story**, use **`AskQuestion`** multiple choice for **In Test** / **Done** / **Skip** when Agent mode supports it ([story-end](.cursor/rules/story-end.md) §4a); otherwise ask in prose. Wait for the choice (or the same message naming the state) **before** `save_issue`; do not guess. **PR / push text:** no “Made-with: Cursor” boilerplate (same file). +Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Planning / implementation decisions** must be written into `docs/plans/` (and related docs)—[`.cursor/rules/planning-implementation-docs.md`](.cursor/rules/planning-implementation-docs.md). **Manual QA checklists** for user-visible ticketed work are generated during implementation at `docs/manual-qa/{KEY}.md` (same rule). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Open PR:** when the user asks, use the **GitHub MCP** **`create_pull_request`** tool (`user-github`) with a title starting **`NEO-123:`** ([commit-and-review](.cursor/rules/commit-and-review.md)). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **Linear:** on **end story**, use **`AskQuestion`** multiple choice for **In Test** / **Done** / **Skip** when Agent mode supports it ([story-end](.cursor/rules/story-end.md) §4a); otherwise ask in prose. Wait for the choice (or the same message naming the state) **before** `save_issue`; do not guess. **PR / push text:** no “Made-with: Cursor” boilerplate (same file). **Commits tied to a Linear issue** must start the subject with the **issue id** and a colon (e.g. `NEO-8: …`). Branch naming and exceptions (`chore:` when there is no ticket) — [`.cursor/rules/linear-git-naming.md`](.cursor/rules/linear-git-naming.md). diff --git a/client/README.md b/client/README.md index 333b82e..3d06e75 100644 --- a/client/README.md +++ b/client/README.md @@ -107,6 +107,15 @@ Full checklist: [`docs/manual-qa/NEO-25.md`](../manual-qa/NEO-25.md). - **Soft lock refresh:** the client does **not** poll on a timer. It refreshes via `GET …/target` on boot, on every `POST …/target/select` response (body already carries `targetState`), and — while a lock is currently held — on **`authoritative_ack`** from `PositionAuthorityClient`, throttled to at most one GET per **250 ms**. `authoritative_ack` fires on every server-confirmed position (boot `GET` 200 **and** successful `move-stream` 200), so a held lock revalidates during normal WASD locomotion without re-introducing snap rubber-banding. Boot is naturally skipped because no lock exists yet. Purely server-driven state flips (future PvP / combat categories) would require revisiting this policy; today NEO-23 has none. - **Inspector:** match **`base_url`** / **`dev_player_id`** on the `TargetSelectionClient` node to the other HTTP clients / `Game:DevPlayerId`. +### SelectionEvent (NEO-26) + +- **`TargetSelectionClient`** emits **`selection_event(event: Dictionary)`** only when the server-acknowledged **`lockedTargetId`** changes (not on **`validity`-only** updates; those stay on **`target_state_changed`**). +- Payload keys: **`previous`** ([String] or `null`), **`next`** (same), **`cause`** (`tab` \| `clear` \| `server_correction`), **`sequence`** (int from the new state). **`POST …/target/select`** from **`request_select_target_id`** (including the path used by Tab) uses cause **`tab`**; **`request_clear_target`** uses **`clear`**; every **`GET …/target`** completion uses **`server_correction`** (including boot sync). +- **Dev logging:** enable **`log_selection_events`** on the `TargetSelectionClient` node to mirror each event to the Godot **Output** (`print`). +- **E1.M4+:** connect to **`$TargetSelectionClient.selection_event`** (or your scene’s path to that node) without reshaping the payload. + +Full checklist: [`docs/manual-qa/NEO-26.md`](../docs/manual-qa/NEO-26.md). + ### Manual check (NEO-24) 1. Run the server and client as in NEO-7 / NEO-9; confirm spawn snap at `(-5, 0.9, -5)`. diff --git a/client/scripts/target_selection_client.gd b/client/scripts/target_selection_client.gd index 56b96a3..b6d85f2 100644 --- a/client/scripts/target_selection_client.gd +++ b/client/scripts/target_selection_client.gd @@ -1,6 +1,8 @@ extends Node ## NEO-24 (E1M3-02): client tab-target + lock UI synced to server (NEO-23 `TargetState` v1). +## NEO-26 (E1M3-04): emits [signal selection_event] when server-acknowledged +## [code]lockedTargetId[/code] changes (see [member log_selection_events]). ## ## Flow: ## - `request_sync_from_server()` → `GET …/target` returns `PlayerTargetStateResponse` v1. @@ -19,6 +21,13 @@ extends Node signal target_state_changed(state: Dictionary) +## NEO-26: fired only when normalized [code]lockedTargetId[/code] changes after a GET/POST. +## Payload: [code]previous[/code] ([String] or [code]null[/code]), [code]next[/code] (same), +## [code]cause[/code] ([code]"tab"[/code] | [code]"clear"[/code] | +## [code]"server_correction"[/code]), [code]sequence[/code] ([int], from new state). +## E1.M4+ may [code]connect[/code] here without reshaping. +signal selection_event(event: Dictionary) + enum Phase { IDLE, GET, POST_SELECT } const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") @@ -30,6 +39,10 @@ const _REFRESH_COOLDOWN_MSEC: int = 250 @export var base_url: String = "http://127.0.0.1:5253" @export var dev_player_id: String = "dev-local-1" +## When true, each [signal selection_event] is also printed to the Godot output +## (dev aid only). +@export var log_selection_events: bool = false + var _http: Node var _busy: bool = false var _phase: Phase = Phase.IDLE @@ -54,6 +67,11 @@ var _state: Dictionary = {} ## `-_REFRESH_COOLDOWN_MSEC` so the first snap is never blocked by the window on boot. var _last_refresh_msec: int = -_REFRESH_COOLDOWN_MSEC +## Cause for the in-flight HTTP request (GET → [code]server_correction[/code]; +## POST → [code]tab[/code] / [code]clear[/code]). +## Set immediately before a request starts; cleared when the response is applied or dropped. +var _http_selection_cause: String = "" + func _create_http_request() -> Node: return HTTPRequest.new() @@ -81,6 +99,7 @@ func _input(event: InputEvent) -> void: func request_sync_from_server() -> void: if _busy: return + _http_selection_cause = "server_correction" _busy = true _phase = Phase.GET _last_refresh_msec = Time.get_ticks_msec() @@ -90,6 +109,7 @@ func request_sync_from_server() -> void: push_warning("TargetSelectionClient: GET failed to start (%s)" % err) _busy = false _phase = Phase.IDLE + _http_selection_cause = "" func request_tab_next() -> void: @@ -117,12 +137,12 @@ func request_select_target_id(target_id: String) -> void: return var payload: Dictionary = {"schemaVersion": 1, "targetId": trimmed} _maybe_attach_position_hint(payload) - _post_select(payload) + _post_select(payload, "tab") func request_clear_target() -> void: # NEO-23: omit `targetId` to clear. JSON `null` also works; omission keeps the body minimal. - _post_select({"schemaVersion": 1}) + _post_select({"schemaVersion": 1}, "clear") ## NEO-24 follow-up #5: attach the live capsule position as an advisory `positionHint` so the @@ -172,9 +192,10 @@ func _has_lock() -> bool: return v is String and not (v as String).is_empty() -func _post_select(payload: Dictionary) -> void: +func _post_select(payload: Dictionary, selection_cause: String) -> void: if _busy: return + _http_selection_cause = selection_cause _kick_freshness_stream() _busy = true _phase = Phase.POST_SELECT @@ -186,6 +207,7 @@ func _post_select(payload: Dictionary) -> void: push_warning("TargetSelectionClient: POST failed to start (%s)" % err) _busy = false _phase = Phase.IDLE + _http_selection_cause = "" ## Nudges the authority with the current capsule position so the server's stored snapshot @@ -220,6 +242,7 @@ func _on_request_completed( if _result != HTTPRequest.RESULT_SUCCESS: push_warning("TargetSelectionClient: HTTP failed (result=%s)" % _result) + _http_selection_cause = "" return var text := body.get_string_from_utf8() @@ -227,6 +250,7 @@ func _on_request_completed( if not parsed is Dictionary: if response_code != 200: push_warning("TargetSelectionClient: HTTP %s non-JSON body" % response_code) + _http_selection_cause = "" return var data: Dictionary = parsed @@ -234,37 +258,51 @@ func _on_request_completed( Phase.GET: if response_code == 200: _update_state_from_get(data) + else: + _http_selection_cause = "" Phase.POST_SELECT: # Server includes authoritative `targetState` on 200 (apply and denial). 400 / 404 # bodies may lack it — swallow quietly; `_state` keeps last-good. if response_code == 200: _update_state_from_post(data) + else: + _http_selection_cause = "" _: - pass + _http_selection_cause = "" func _update_state_from_get(data: Dictionary) -> void: var state := _extract_target_state_fields(data) + var cause := _http_selection_cause + _http_selection_cause = "" if state.is_empty(): return + var prev: Dictionary = _state.duplicate() _state = state target_state_changed.emit(_state.duplicate()) + _maybe_emit_selection_event(prev, state, cause) func _update_state_from_post(data: Dictionary) -> void: var target_state_variant: Variant = data.get("targetState", null) if not target_state_variant is Dictionary: + _http_selection_cause = "" push_warning("TargetSelectionClient: POST 200 missing targetState") return var state := _extract_target_state_fields(target_state_variant as Dictionary) if state.is_empty(): + _http_selection_cause = "" return + var cause := _http_selection_cause + _http_selection_cause = "" state["selectionApplied"] = bool(data.get("selectionApplied", false)) var reason_variant: Variant = data.get("reasonCode", null) if reason_variant is String and not (reason_variant as String).is_empty(): state["reasonCode"] = reason_variant as String + var prev: Dictionary = _state.duplicate() _state = state target_state_changed.emit(_state.duplicate()) + _maybe_emit_selection_event(prev, state, cause) ## Shape-only parse of the v1 target state payload (works for GET body and POST echo). @@ -285,3 +323,32 @@ func _extract_target_state_fields(data: Dictionary) -> Dictionary: out["validity"] = (validity_variant as String) if validity_variant is String else "none" out["sequence"] = int(data.get("sequence", 0)) return out + + +func _normalized_lock_id(state: Dictionary) -> Variant: + if state.is_empty(): + return null + var v: Variant = state.get("lockedTargetId", null) + if v is String and not (v as String).is_empty(): + return v as String + return null + + +func _maybe_emit_selection_event( + prev_state: Dictionary, new_state: Dictionary, cause: String +) -> void: + var prev_id: Variant = _normalized_lock_id(prev_state) + var next_id: Variant = _normalized_lock_id(new_state) + if prev_id == next_id: + return + if cause.is_empty(): + return + var ev: Dictionary = { + "previous": prev_id, + "next": next_id, + "cause": cause, + "sequence": int(new_state.get("sequence", 0)), + } + selection_event.emit(ev) + if log_selection_events: + print("[TargetSelectionClient] SelectionEvent ", ev) diff --git a/client/test/selection_event_client_test.gd b/client/test/selection_event_client_test.gd new file mode 100644 index 0000000..178458b --- /dev/null +++ b/client/test/selection_event_client_test.gd @@ -0,0 +1,157 @@ +extends GdUnitTestSuite + +## NEO-26: `TargetSelectionClient.selection_event` — cause strings and lock-id-only rule. + +const TargetSelectionTestDouble := preload("res://test/target_selection_test_double.gd") + +const ALPHA_ID := "prototype_target_alpha" +const BETA_ID := "prototype_target_beta" + + +class MockHttpTransport: + extends Node + signal request_completed( + result: int, response_code: int, headers: PackedStringArray, body: PackedByteArray + ) + var _queue: Array[Dictionary] = [] + + func enqueue(result: int, code: int, body: String) -> void: + _queue.append({"result": result, "code": code, "body": body}) + + func request( + _url: String, + _custom_headers: PackedStringArray = PackedStringArray(), + _method: HTTPClient.Method = HTTPClient.METHOD_GET, + _request_data: String = "" + ) -> Error: + if _queue.is_empty(): + return ERR_UNAVAILABLE + var r: Dictionary = _queue.pop_front() + var body_bytes: PackedByteArray = str(r["body"]).to_utf8_buffer() + request_completed.emit(r["result"], r["code"], PackedStringArray(), body_bytes) + return OK + + +func _make_client(http_transport: Node) -> Node: + var c: Node = TargetSelectionTestDouble.new() + c.set("injected_http", http_transport) + auto_free(http_transport) + auto_free(c) + add_child(c) + return c + + +func _target_state_json(locked_id_or_null: Variant, validity: String, sequence: int = 0) -> String: + var locked := "null" + if locked_id_or_null is String and not (locked_id_or_null as String).is_empty(): + locked = '"%s"' % (locked_id_or_null as String) + return ( + '{"schemaVersion":1,"playerId":"dev-local-1","lockedTargetId":%s,' % locked + + '"validity":"%s","sequence":%d}' % [validity, sequence] + ) + + +func _select_response_json( + applied: bool, + locked_id_or_null: Variant, + validity: String, + sequence: int = 0, + reason: String = "" +) -> String: + var target_state := _target_state_json(locked_id_or_null, validity, sequence) + if applied: + return '{"schemaVersion":1,"selectionApplied":true,"targetState":%s}' % target_state + var reason_segment := "" + if not reason.is_empty(): + reason_segment = ',"reasonCode":"%s"' % reason + return ( + '{"schemaVersion":1,"selectionApplied":false%s,"targetState":%s}' + % [reason_segment, target_state] + ) + + +func test_tab_lock_change_emits_tab() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + var c := _make_client(transport) + var events: Array = [] + c.selection_event.connect(func(ev: Dictionary) -> void: events.append(ev.duplicate())) + c.request_tab_next() + assert_that(events.size()).is_equal(1) + var last: Dictionary = events[0] as Dictionary + assert_that(last.get("cause", "")).is_equal("tab") + assert_that(last.get("previous")).is_null() + assert_that(last.get("next")).is_equal(ALPHA_ID) + assert_that(int(last.get("sequence", -1))).is_equal(1) + + +func test_clear_lock_change_emits_clear() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + transport.enqueue(HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, null, "none", 2)) + var c := _make_client(transport) + var events: Array = [] + c.selection_event.connect(func(ev: Dictionary) -> void: events.append(ev.duplicate())) + c.request_tab_next() + c.request_clear_target() + assert_that(events.size()).is_equal(2) + assert_that(int((events[0] as Dictionary).get("sequence", -1))).is_equal(1) + assert_that((events[1] as Dictionary).get("cause", "")).is_equal("clear") + assert_that((events[1] as Dictionary).get("previous")).is_equal(ALPHA_ID) + assert_that((events[1] as Dictionary).get("next")).is_null() + assert_that(int((events[1] as Dictionary).get("sequence", -1))).is_equal(2) + + +func test_get_lock_id_change_emits_server_correction() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + transport.enqueue(HTTPRequest.RESULT_SUCCESS, 200, _target_state_json(BETA_ID, "ok", 2)) + var c := _make_client(transport) + var events: Array = [] + c.selection_event.connect(func(ev: Dictionary) -> void: events.append(ev.duplicate())) + c.request_tab_next() + c.request_sync_from_server() + assert_that(events.size()).is_equal(2) + var last: Dictionary = events[1] as Dictionary + assert_that(last.get("cause", "")).is_equal("server_correction") + assert_that(last.get("previous")).is_equal(ALPHA_ID) + assert_that(last.get("next")).is_equal(BETA_ID) + assert_that(int(last.get("sequence", -1))).is_equal(2) + + +func test_get_validity_only_change_emits_nothing() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _target_state_json(ALPHA_ID, "out_of_range", 1) + ) + var c := _make_client(transport) + var events: Array = [] + c.selection_event.connect(func(ev: Dictionary) -> void: events.append(ev.duplicate())) + c.request_tab_next() + c.request_sync_from_server() + assert_that(events.size()).is_equal(1) + assert_that((events[0] as Dictionary).get("cause", "")).is_equal("tab") + assert_that(int((events[0] as Dictionary).get("sequence", -1))).is_equal(1) + + +func test_denial_without_lock_id_change_emits_nothing() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, + 200, + _select_response_json(false, null, "none", 0, "out_of_range") + ) + var c := _make_client(transport) + var events: Array = [] + c.selection_event.connect(func(ev: Dictionary) -> void: events.append(ev.duplicate())) + c.request_select_target_id(ALPHA_ID) + assert_that(events.size()).is_equal(0) diff --git a/client/test/selection_event_client_test.gd.uid b/client/test/selection_event_client_test.gd.uid new file mode 100644 index 0000000..c590918 --- /dev/null +++ b/client/test/selection_event_client_test.gd.uid @@ -0,0 +1 @@ +uid://ksj15nsjl6ph diff --git a/docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md b/docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md index f9611a7..04e6f6b 100644 --- a/docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md +++ b/docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md @@ -100,7 +100,7 @@ flowchart LR ## Implementation snapshot -- **`TargetState` (prototype JSON v1 — NEO-23):** server **`GET /game/players/{id}/target`** and **`POST /game/players/{id}/target/select`** with versioned **`PlayerTargetStateResponse`** / **`TargetSelectRequest`** / **`TargetSelectResponse`**; stub registry + in-memory lock under `server/NeonSprawl.Server/Game/Targeting/`; horizontal reach + soft lock semantics — [NEO-23](../../plans/NEO-23-implementation-plan.md), [server README — Targeting](../../../server/README.md#targeting-neo-23). Godot tab-target: [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24). **`InteractableDescriptor` list:** **`GET /game/world/interactables`** (NEO-25). **Still open on later issues:** **`SelectionEvent`**. **Story backlog:** [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md). +- **`TargetState` (prototype JSON v1 — NEO-23):** server **`GET /game/players/{id}/target`** and **`POST /game/players/{id}/target/select`** with versioned **`PlayerTargetStateResponse`** / **`TargetSelectRequest`** / **`TargetSelectResponse`**; stub registry + in-memory lock under `server/NeonSprawl.Server/Game/Targeting/`; horizontal reach + soft lock semantics — [NEO-23](../../plans/NEO-23-implementation-plan.md), [server README — Targeting](../../../server/README.md#targeting-neo-23). Godot tab-target: [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24). **`InteractableDescriptor` list:** **`GET /game/world/interactables`** (NEO-25). **`SelectionEvent` (prototype):** Godot **`TargetSelectionClient.selection_event`** when **`lockedTargetId`** changes — [NEO-26](../../plans/NEO-26-implementation-plan.md). **Story backlog:** [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md). - **Precursor under E1.M1 (NEO-9 + NEO-25):** server **`POST /game/players/{id}/interact`** with **`InteractionRequest`** / **`InteractionResponse`**; **`GET /game/world/interactables`** (versioned descriptor list); **`HorizontalReach`** (`server/NeonSprawl.Server/Game/World/HorizontalReach.cs`); prototype registry under `server/NeonSprawl.Server/Game/Interaction/`; client **`interactables_catalog_client.gd`**, **`interactable_world_builder.gd`**, **`interaction_request_client.gd`**, **`interaction_radius_indicators.gd`** — see [NEO-9](../../plans/NEO-9-implementation-plan.md), [NEO-25](../../plans/NEO-25-implementation-plan.md), and [server README — Interaction](../../../server/README.md#interaction-neo-9). - **Alignment:** [Documentation and implementation alignment](documentation_and_implementation_alignment.md). diff --git a/docs/decomposition/modules/documentation_and_implementation_alignment.md b/docs/decomposition/modules/documentation_and_implementation_alignment.md index 2470e59..ac523a9 100644 --- a/docs/decomposition/modules/documentation_and_implementation_alignment.md +++ b/docs/decomposition/modules/documentation_and_implementation_alignment.md @@ -48,7 +48,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not |--------|-----------------|----------|-------------------| | E1.M1 | Ready | Prototype milestone **Done** ([E1.M1](https://linear.app/neon-sprawl/project/e1m1-inputandmovementruntime-20eb28dd3db4)). Authoritative `PositionState` + **MoveCommand** over HTTP (JSON v1 snap); **in-memory** store by default, **Postgres** when configured ([NEO-8](../../plans/NEO-8-implementation-plan.md)); shared **NpgsqlDataSource** disposed on host shutdown ([NEO-13](../../plans/NEO-13-implementation-plan.md)); Godot sync + path-follow ([NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md)); **InteractionRequest** + horizontal range ([NEO-9](../../plans/NEO-9-implementation-plan.md)). Follow-on: prediction/reconciliation, Slice 1 telemetry, Protobuf wire per [contracts.md](contracts.md). | [NEO-6](../../plans/NEO-6-implementation-plan.md), [NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-8](../../plans/NEO-8-implementation-plan.md), [NEO-9](../../plans/NEO-9-implementation-plan.md), [NEO-10](../../plans/NEO-10-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md), [NEO-13](../../plans/NEO-13-implementation-plan.md); `server/NeonSprawl.Server/Game/PositionState/`, `Game/Interaction/`; [server README](../../../server/README.md) | | E1.M2 | Ready | **Slice 2 prototype slice closed:** follow + `CameraState` ([NEO-15](https://linear.app/neon-sprawl/issue/NEO-15)); zoom bands ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); occlusion ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); occluder pick-through ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); contracts + hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). Client-local; no server use of camera pose. | [NEO-15](../../plans/NEO-15-implementation-plan.md), [NEO-16](../../plans/NEO-16-implementation-plan.md), [NEO-17](../../plans/NEO-17-implementation-plan.md), [NEO-18](../../plans/NEO-18-implementation-plan.md), [NEO-20](../../plans/NEO-20-implementation-plan.md); `client/scripts/isometric_follow_camera.gd`, `camera_state.gd`, `zoom_band_config.gd`, `occlusion_policy.gd`, `ground_pick.gd`; [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md) | -| E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **NEO-25 landed:** versioned **`GET /game/world/interactables`** ([NEO-25](../../plans/NEO-25-implementation-plan.md)) — `InteractablesListResponse` / `InteractablesWorldApi` in `server/NeonSprawl.Server/Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` (fetch-driven props + glow); [server README — Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25). **Still open:** **`SelectionEvent`** and richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact` (see [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md)). **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25), [Interaction](../../../server/README.md#interaction-neo-9) | +| E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **NEO-25 landed:** versioned **`GET /game/world/interactables`** ([NEO-25](../../plans/NEO-25-implementation-plan.md)) — `InteractablesListResponse` / `InteractablesWorldApi` in `server/NeonSprawl.Server/Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` (fetch-driven props + glow); [server README — Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25). **NEO-26 landed:** prototype **`SelectionEvent`** — **`TargetSelectionClient.selection_event`** ([NEO-26](../../plans/NEO-26-implementation-plan.md)) when **`lockedTargetId`** changes; optional **`log_selection_events`**. **Follow-on / still open:** richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact`, Slice 3 telemetry (see [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md)). **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25), [Interaction](../../../server/README.md#interaction-neo-9) | --- diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index e28eed2..fd8bd1e 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -19,7 +19,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen **E1.M2 note:** Prototype slice **Ready**: `client/scripts/isometric_follow_camera.gd` + child `Camera3D` on **`World/IsometricFollowCamera`** in `client/scenes/main.tscn`; per-tick **`CameraState`** (`client/scripts/camera_state.gd`); **`ZoomBandConfig`** ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); **`OcclusionPolicy`** ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); pick-through **`"occluder"`** convention ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); consumer contract + occluder lifecycle hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). See [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md). -**E1.M3 note:** Decomposition expanded in [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md) (E1.M1 vs E1.M3 boundary, contract sketches, Slice 3 alignment). **Partial (NEO-23):** versioned JSON v1 **`PlayerTargetStateResponse`** + **`POST …/target/select`** under `server/NeonSprawl.Server/Game/Targeting/` ([NEO-23](../../plans/NEO-23-implementation-plan.md); [server README — Targeting](../../../server/README.md#targeting-neo-23)) — throwaway HTTP spike per [contracts.md](contracts.md). **Partial (NEO-25):** versioned **`GET /game/world/interactables`** + `InteractablesListDtos` / `InteractablesWorldApi` in `Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` ([NEO-25](../../plans/NEO-25-implementation-plan.md); [server README — Interactable descriptors](../../../server/README.md#interactable-descriptors-neo-25)). **`SelectionEvent`** and richer multi-consumer **`InteractableDescriptor`** work remain **in progress** on later Linear issues. **Precursor (E1.M1):** **`InteractionRequest`** + horizontal reach ([NEO-9](../../plans/NEO-9-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`). +**E1.M3 note:** Decomposition expanded in [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md) (E1.M1 vs E1.M3 boundary, contract sketches, Slice 3 alignment). **Partial (NEO-23):** versioned JSON v1 **`PlayerTargetStateResponse`** + **`POST …/target/select`** under `server/NeonSprawl.Server/Game/Targeting/` ([NEO-23](../../plans/NEO-23-implementation-plan.md); [server README — Targeting](../../../server/README.md#targeting-neo-23)) — throwaway HTTP spike per [contracts.md](contracts.md). **Partial (NEO-25):** versioned **`GET /game/world/interactables`** + `InteractablesListDtos` / `InteractablesWorldApi` in `Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` ([NEO-25](../../plans/NEO-25-implementation-plan.md); [server README — Interactable descriptors](../../../server/README.md#interactable-descriptors-neo-25)). **NEO-26 (prototype `SelectionEvent`):** Godot **`TargetSelectionClient.selection_event`** on **`lockedTargetId`** changes ([NEO-26](../../plans/NEO-26-implementation-plan.md)). **Follow-on / in progress:** richer multi-consumer **`InteractableDescriptor`** targeting on later Linear issues. **Precursor (E1.M1):** **`InteractionRequest`** + horizontal reach ([NEO-9](../../plans/NEO-9-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`). ### Epic 2 — Skills and Progression Framework diff --git a/docs/manual-qa/NEO-26.md b/docs/manual-qa/NEO-26.md new file mode 100644 index 0000000..b8d4419 --- /dev/null +++ b/docs/manual-qa/NEO-26.md @@ -0,0 +1,25 @@ +# NEO-26 — Manual QA checklist + +| Field | Value | +|--------|--------| +| **Key** | NEO-26 | +| **Title** | E1.M3: SelectionEvent surface + debug or HUD consumer | +| **Linear** | [NEO-26](https://linear.app/neon-sprawl/issue/NEO-26/e1m3-selectionevent-surface-debug-or-hud-consumer) | +| **Plan** | [`docs/plans/NEO-26-implementation-plan.md`](../plans/NEO-26-implementation-plan.md) | + +Pair client (**F5**) with the server. Open the Godot **Output** dock. + +## 1. `log_selection_events` + +- [ ] Select **`TargetSelectionClient`** in the scene tree; set **`log_selection_events`** to **On** in the Inspector. +- [ ] Run with server. Press **Tab** to lock a target: Output shows a line **`[TargetSelectionClient] SelectionEvent`** with `cause=tab`, `previous` null or prior id, `next` = locked id. +- [ ] Press **Esc** to clear: next line shows **`cause=clear`**, `next` null when lock cleared. +- [ ] Turn **`log_selection_events`** **Off**; Tab / Esc no longer print selection lines (HUD from NEO-24 unchanged). + +## 2. Movement GET (validity-only) + +- [ ] With logging **off**, lock **alpha**, **WASD** until HUD shows **`Validity: out_of_range`** (NEO-24 soft lock). No requirement for extra **Output** lines from NEO-26 here — **id-only** events suppress **`validity`-only** GET updates. + +## 3. Subscription sanity (optional) + +- [ ] In a throwaway local script or remote inspector, **`connect`** to **`selection_event`** and confirm the same payloads as printed when logging is on. diff --git a/docs/plans/NEO-26-implementation-plan.md b/docs/plans/NEO-26-implementation-plan.md new file mode 100644 index 0000000..56c4421 --- /dev/null +++ b/docs/plans/NEO-26-implementation-plan.md @@ -0,0 +1,97 @@ +# NEO-26 — Implementation plan + +## Story reference + +| Field | Value | +|--------|--------| +| **Key** | NEO-26 | +| **Title** | E1.M3: SelectionEvent surface + debug or HUD consumer | +| **Linear** | [NEO-26](https://linear.app/neon-sprawl/issue/NEO-26/e1m3-selectionevent-surface-debug-or-hud-consumer) | +| **Slug** | E1M3-04 | +| **Git branch** | `NEO-26-selectionevent-surface-debug-consumer` | +| **Parent context** | [Epic 1 — Core Player Runtime](https://linear.app/neon-sprawl/project/epic-1-core-player-runtime-client-controls-character-loop-66bd590cd016) · [E1M3 prototype backlog](E1M3-prototype-backlog.md) | +| **Depends on** | [NEO-23](NEO-23-implementation-plan.md) (`TargetState` v1), [NEO-24](NEO-24-implementation-plan.md) (tab/clear + `TargetSelectionClient`) — code paths exist on `main`; Linear may still list relations until those issues are closed. | +| **Decomposition** | [E1.M3 — InteractionAndTargetingLayer](../decomposition/modules/E1_M3_InteractionAndTargetingLayer.md) (`SelectionEvent` contract); [E1.M4 — AbilityInputScaffold](../decomposition/modules/E1_M4_AbilityInputScaffold.md) (future subscriber). | + +## Goal, scope, and out-of-scope + +**Goal:** Every **material change** to the client’s **server-acknowledged combat target lock id** (`lockedTargetId` in the cached `TargetState` mirror) is observable as a **`SelectionEvent`** carrying **previous** id, **next** id, and **cause** (`tab`, `clear`, `server_correction`, …). Ship **one** built-in consumer: **dev-only `print`** behind an `@export` flag (planning choice 2026-04-25) so the bus is exercised without new HUD nodes. + +**In scope** + +- Minimal **GDScript** event shape (Dictionary or small typed helper) aligned with the illustrative table in E1.M3: `previous` (Variant `String` or `null`), `next` (same), `cause` (`String` enum-like literals). +- **Cause attribution** at the authoritative update boundary inside the existing selection client (see Technical approach) so we do not guess causes from state diffs alone. +- **Consumer:** optional **`print`** when `@export var log_selection_events` (name TBD) is true on **`TargetSelectionClient`** (or tiny child node); document the **subscription point** for E1.M4 (`signal` name + emitter node path pattern). +- **Tests:** tab vs clear vs movement-driven GET produce **distinct** `cause` strings when they each produce a lock-id transition (or documented no-op when id unchanged). + +**Out of scope (per Linear / backlog)** + +- Full prompt / party UI; wire protobuf events. +- **Interactable** focus / primary selection as `SelectionEvent` — **target lock** only for this ticket (see D6); catalog exists regardless of NEO-25 lifecycle. +- Changing server **`TargetState`** contracts (NEO-23); new HTTP routes. + +## Acceptance criteria checklist + +- [x] **Tab** path: after a successful selection cycle that **changes** `lockedTargetId`, a `SelectionEvent` is emitted with `cause` **`tab`** (and correct previous/next ids). +- [x] **Clear** path: clearing the lock emits **`clear`** when `lockedTargetId` transitions from non-null to `null`. +- [x] **Server correction:** a **GET**-driven authoritative update that **changes** `lockedTargetId` emits **`server_correction`**. **Id-unchanged** GETs (e.g. **`validity`** only) do **not** emit `SelectionEvent` for v1 — `test_get_validity_only_change_emits_nothing`; id change via mock in `test_get_lock_id_change_emits_server_correction`. +- [x] **E1.M4 hook:** this plan + **`client/README.md`** (“SelectionEvent (NEO-26)”) document **`TargetSelectionClient.selection_event`** and stable payload keys. + +## Technical approach + +1. **Single emitter:** Extend **`TargetSelectionClient`** (`client/scripts/target_selection_client.gd`) rather than diffing `target_state_changed` in a second node — only this class knows whether the in-flight HTTP phase was **GET** (refresh) vs **POST** from **tab**, **clear**, or direct **`request_select_target_id`** (if used). + +2. **Intent / phase tags:** Before starting HTTP, set a private enum or String tag, e.g. `_pending_selection_cause`, cleared after handling the response: + - `request_tab_next` → `tab` (even though it calls `request_select_target_id` internally, set cause at the tab entrypoint before POST). + - `request_clear_target` → `clear`. + - `request_select_target_id` when **not** entered from `request_tab_next` → use cause **`tab`** (same string as Tab path — planning choice 2026-04-25: no separate `direct_select`). + - `request_sync_from_server` when invoked from **`on_authoritative_ack`** → mark refresh as **`server_correction`** for the next GET completion. + - Boot / one-shot sync from `main.gd` → **Decision (2026-04-25):** first and later **GET** completions use cause **`server_correction`**; **only emit** if normalized `lockedTargetId` differs from prior snapshot (avoids spam on identical boot GET). + +3. **Emit rule:** After `_state` is updated in `_update_state_from_get` / `_update_state_from_post`, compare **previous** normalized `lockedTargetId` to **new**; if equal, **no** `SelectionEvent` (validity-only changes stay on `target_state_changed` only unless product later expands the contract). + +4. **Signal:** Add e.g. **`selection_event(event: Dictionary)`** with keys `previous`, `next`, `cause` (and optional `sequence` int passthrough for debugging). Keep **`target_state_changed`** unchanged for existing HUD. + +5. **Consumer:** No new scene nodes — **`TargetSelectionClient.log_selection_events`** mirrors each **`selection_event`** to **Output** via **`print`**. + +6. **Docs:** Update **`client/README.md`** “Target lock + tab cycle” area with a short **“SelectionEvent (NEO-26)”** subsection: signal name, payload keys, print flag, E1.M4 subscription note. + +## Files to add + +| Path | Purpose | +|------|---------| +| `client/test/selection_event_client_test.gd` | GdUnit4: assert causes for tab POST, clear POST, and GET-with-mock-body changing lock id (`TargetSelectionTestDouble` pattern). | +| `docs/manual-qa/NEO-26.md` | Manual QA for `log_selection_events` + optional subscription check. | + +## Files to modify + +| Path | Rationale | +|------|-----------| +| `client/scripts/target_selection_client.gd` | Track pending cause through GET/POST; compare old/new `lockedTargetId`; emit `selection_event`; optional `print` behind `@export`; document next to `target_state_changed`. | +| `client/README.md` | Document subscription point + payload for E1.M4. | +| `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` | **Implementation snapshot** bullet: replace “Still open — `SelectionEvent`” with pointer to NEO-26 + signal name (keeps decomposition aligned with shipped code). | + +## Tests + +| Test file | What it covers | +|-----------|------------------| +| `client/test/selection_event_client_test.gd` | **Add:** Tab mock POST changes lock → event `cause == "tab"`. Clear mock → `"clear"`. Simulated GET completion after flagging movement refresh → `"server_correction"` when body changes `lockedTargetId`. Assert **no** event when id unchanged. | +| `client/test/target_selection_client_test.gd` | **Change (optional):** If new behavior duplicates setup, keep NEO-24 tests focused on HTTP/state; only touch this file when sharing helpers avoids copy-paste. | + +**Manual verification:** [`docs/manual-qa/NEO-26.md`](../../manual-qa/NEO-26.md). + +## Open questions / risks + +- **Validity-only updates:** If v1 stays **lock-id-only** (recommended), extend later with `kind` or dual fields if telemetry needs **`validity`** transitions on the same bus. +- **Linear blocked-by:** Relations may still show NEO-23/NEO-24 until closed; implementation assumes their client/server surfaces on `main` are present. + +## Decisions + +| Topic | Decision | +|--------|----------| +| D1 — Emitter location | **`TargetSelectionClient`** owns `selection_event` (intent-aware). | +| D2 — Boot GET | Use cause **`server_correction`** for all GET completions (including first sync); **suppress** event if normalized `lockedTargetId` unchanged vs prior snapshot. | +| D3 — Non-tab `request_select_target_id` | Use cause **`tab`** (same literal as Tab path; no `direct_select`). | +| D4 — Consumer | **Print only** behind `@export` flag on emitter; no HUD panel in NEO-26. | +| D5 — Emit rule (v1) | **Lock id only:** emit `SelectionEvent` only when normalized **`lockedTargetId`** changes; **`validity`** / sequence updates without id change → no event (HUD keeps using `target_state_changed`). *Rationale (2026-04-25):* matches backlog “previous / next” id story, smallest payload, clear E1.M4 hook for cast target; avoids conflating soft-lock range telemetry with selection changes. Override if you want validity on this signal too. | +| D6 — Interactables | **Out of scope:** target lock only for NEO-26. *Rationale:* Linear / E1M3-04 acceptance is about **combat target** lock changes (`SelectionEvent` for tab-target), not interactable focus or “use now” flow ([NEO-9](NEO-9-implementation-plan.md) / catalog). Descriptor work ([NEO-25](NEO-25-implementation-plan.md)) can be **done** and this story still stays narrow: a unified “all selection kinds” bus would be **extra contract design** (payload + causes + wiring) that this ticket never asked for—not a dependency on NEO-25 being open. | diff --git a/docs/reviews/2026-04-25-NEO-26.md b/docs/reviews/2026-04-25-NEO-26.md new file mode 100644 index 0000000..859c489 --- /dev/null +++ b/docs/reviews/2026-04-25-NEO-26.md @@ -0,0 +1,39 @@ +# Code Review — NEO-26 SelectionEvent + +- **Date:** 2026-04-25 +- **Scope:** Branch `NEO-26-selectionevent-surface-debug-consumer`; diff `origin/main...HEAD` (NEO-26 `SelectionEvent` surface + debug consumer) +- **Base:** `origin/main` + +## Verdict + +Approve with nits. + +## Summary + +This branch adds a focused `SelectionEvent` surface in `TargetSelectionClient` and keeps it intentionally narrow to `lockedTargetId` transitions, with explicit `cause` attribution (`tab`, `clear`, `server_correction`). The implementation is coherent with the NEO-26 plan: cause tagging is set at request start, stale/failed paths clear the pending cause, and emit is suppressed for id-unchanged updates. Coverage in `client/test/selection_event_client_test.gd` exercises both positive and no-op rules, and the targeted suite passes locally. Risk is low and mostly around documentation tracking consistency rather than runtime behavior. + +## Documentation checked + +- `docs/plans/NEO-26-implementation-plan.md` — **matches** (signal shape, cause rules, lock-id-only emit rule, print consumer, QA artifact all implemented). +- `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` — **matches** (implementation snapshot updated to include NEO-26 `selection_event` prototype). +- ~~`docs/decomposition/modules/module_dependency_register.md` — **partially matches** (E1.M3 note still says "`SelectionEvent` ... remain in progress"; …).~~ **Addressed:** E1.M3 note now records **NEO-26** prototype `SelectionEvent` and separates follow-on `InteractableDescriptor` / multi-consumer work. +- ~~`docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches** (E1.M3 row still says "`SelectionEvent` ... still open"; …).~~ **Addressed:** E1.M3 snapshot row records **NEO-26** landed vs follow-on scope. + +~~Implementation tracking update needed after merge: update the E1.M3 row wording in `documentation_and_implementation_alignment.md` (and optionally the E1.M3 note in `module_dependency_register.md`) so the docs reflect that prototype `SelectionEvent` now exists while richer downstream consumers remain open.~~ **Done.** + +## Blocking issues + +None. + +## Suggestions + +1. ~~Update the E1.M3 tracking row in `docs/decomposition/modules/documentation_and_implementation_alignment.md` to explicitly record NEO-26 as landed, replacing the blanket "`SelectionEvent` still open" phrasing with "prototype landed; richer consumers follow-on."~~ **Done.** + +## Nits + +- ~~Nit: Consider adding one assertion for `sequence` in `client/test/selection_event_client_test.gd` to lock payload stability for downstream subscribers.~~ **Done.** (`sequence` asserted on tab, clear, GET id-change, and validity-only cases.) + +## Verification + +- `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/selection_event_client_test.gd` (from `client/`) — passed (`5 test cases`, `0 failures`). +- Manual checks documented in `docs/manual-qa/NEO-26.md`.