From 2c32bba3204b126f73c6764ec3b82395958b3875 Mon Sep 17 00:00:00 2001 From: Phillip Tarrant Date: Fri, 10 Jul 2026 14:55:00 -0500 Subject: [PATCH] docs(plan): considering-state + transport timeout implementation plan 5 TDD tasks: ConsideringPhrases (pure) + authored considering.json; ProxyConfig.request_timeout_seconds (default 35s, reject <=0); HttpTransport applies it (closes the infinite-hang gap); ConsideringIndicator node shim (Timer rotates a Label); wire both harnesses around the await. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../plans/2026-07-10-considering-state.md | 520 ++++++++++++++++++ 1 file changed, 520 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-10-considering-state.md diff --git a/docs/superpowers/plans/2026-07-10-considering-state.md b/docs/superpowers/plans/2026-07-10-considering-state.md new file mode 100644 index 0000000..0c4e5bd --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-considering-state.md @@ -0,0 +1,520 @@ +# Considering-state + transport timeout Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** While a model call is in flight, the client shows a rotating in-character "the Master considers…" indicator instead of a frozen surface, and a hung call gives way to the authored fallback instead of spinning forever. + +**Architecture:** All client-side, no server change. A pure `ConsideringPhrases` (cycles an authored phrase pool loaded from content, robust to a missing file) drives a thin `ConsideringIndicator` node (owns a `Timer`, rotates a `Label`). `ProxyConfig` gains a request-timeout setting; `HttpTransport` applies it to its `HTTPRequest`, so a hang maps through the existing `RESULT_TIMEOUT → failed() → degrade` path. Both harnesses show the indicator around their `await`. + +**Tech Stack:** Godot 4.7 / GDScript / GUT. + +## Global Constraints + +- **§2 — this is text/UI only.** No game state, no AI, no canon-log mutation. The phrases are authored content. +- **§13 — loading is content.** Phrases live in `client/content/considering.json`, loaded like the fallback lines; a missing/malformed file degrades to one hardcoded constant so the indicator itself cannot fail. +- **JSON-parse GOTCHA:** in any code the tests touch, parse with `var j := JSON.new(); j.parse(text)` and check `!= OK` — NOT `JSON.parse_string()` (the project's `.gutconfig.json` `failure_error_types` turns its engine error into a false test failure). +- **Timeout default 35.0s** — just above the server's `OLLAMA_TIMEOUT_SECONDS=30`, so a slow *model* surfaces the server's own error first while a fully-hung proxy is still bounded. A hang reuses the existing `HttpTransport` `result != RESULT_SUCCESS → DmResponse.failed()` path (already covered by `test_transport_failure_degrades`); do not add a new service-level degrade test. +- **Rotation index is instance state, not reset per call** (spec decision 4): one `ConsideringPhrases`/`ConsideringIndicator` instance persists on the harness; `next()` advances a running index that `start()`/`stop()` never reset. +- **Branch:** all work on `feature/considering-state`, branched off `dev`. Never commit to `dev`/`master` directly (CLAUDE.md §18); the human owns the `dev` merge and push. +- **Client tests:** `cd client && ./run_tests.sh` (full) or `./run_tests.sh -gtest=res://tests/unit/test_X.gd` (one file). A new `class_name` may need `cd client && godot --headless --import` once before it resolves. + +--- + +## File Structure + +- `client/scripts/ui/considering_phrases.gd` — CREATE: pure phrase-pool cycler. +- `client/content/considering.json` — CREATE: authored phrase pool. +- `client/scripts/net/proxy_config.gd` — MODIFY: add `request_timeout_seconds()`. +- `client/scripts/net/http_transport.gd` — MODIFY: set `_http.timeout`. +- `client/scripts/ui/considering_indicator.gd` — CREATE: node shim (Timer + Label). +- `client/scripts/harness/narrate_harness.gd` — MODIFY: show indicator around the await. +- `client/scripts/harness/npc_harness.gd` — MODIFY: add a status Label + show indicator. +- Tests: `client/tests/unit/test_considering_phrases.gd` (create), `client/tests/unit/test_considering_indicator.gd` (create), `client/tests/unit/test_net_primitives.gd` (add ProxyConfig + HttpTransport timeout tests). + +--- + +## Task 1: `ConsideringPhrases` (pure) + authored pool + +**Files:** +- Create: `client/scripts/ui/considering_phrases.gd` +- Create: `client/content/considering.json` +- Test: `client/tests/unit/test_considering_phrases.gd` + +**Interfaces:** +- Produces: `ConsideringPhrases.new(path := PATH)`; `next() -> String` (returns the pool entry at a running index and advances it, wrapping; `LAST_RESORT` when the pool is empty/missing). Constants `PATH`, `LAST_RESORT`. + +- [ ] **Step 1: Write the failing test** + +```gdscript +# client/tests/unit/test_considering_phrases.gd +extends "res://addons/gut/test.gd" + +const ConsideringPhrases = preload("res://scripts/ui/considering_phrases.gd") + + +func test_missing_file_returns_last_resort(): + var p = ConsideringPhrases.new("res://content/does_not_exist.json") + assert_eq(p.next(), ConsideringPhrases.LAST_RESORT) + assert_eq(p.next(), ConsideringPhrases.LAST_RESORT) + + +func test_cycles_pool_in_order_and_wraps(): + # Real authored file has at least 3 lines; assert order + wrap on it. + var p = ConsideringPhrases.new() # default path — real content + var first = p.next() + var second = p.next() + assert_ne(first, "") + assert_ne(first, second) # advances, not stuck + # Drain a full cycle back to `first` to prove it wraps (bounded loop). + var seen := [first, second] + for i in range(20): + var n = p.next() + if n == first: + return # wrapped — success + seen.append(n) + assert_true(false, "did not wrap within 22 calls: %s" % [seen]) + + +func test_index_persists_across_calls_not_reset(): + var p = ConsideringPhrases.new() + var a = p.next() + var b = p.next() + assert_ne(a, b) # second call did not re-return the first phrase +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `cd client && ./run_tests.sh -gtest=res://tests/unit/test_considering_phrases.gd` +Expected: FAIL — script does not exist. + +- [ ] **Step 3: Implement** + +Create `client/content/considering.json`: + +```json +{ + "considering": [ + "The Master considers your words.", + "Somewhere, dice are gathered.", + "The tale weighs its next turn.", + "The dark shifts, thinking.", + "A page is turned, unhurried." + ] +} +``` + +Create `client/scripts/ui/considering_phrases.gd`: + +```gdscript +class_name ConsideringPhrases +extends RefCounted +## Authored "the GM is thinking" lines (charter §13 — even loading is content). +## Pure: cycles a phrase pool in order on a running index that is instance state +## (spec decision 4 — the indicator reuses one instance so the first phrase varies +## turn to turn without randomness). A missing/malformed file degrades to one +## hardcoded constant, so the indicator itself cannot fail. + +const PATH := "res://content/considering.json" +const LAST_RESORT := "The Master considers your words." + +var _pool: Array = [] +var _index: int = 0 + + +func _init(path := PATH) -> void: + _pool = _load(path) + + +func next() -> String: + if _pool.is_empty(): + return LAST_RESORT + var phrase := str(_pool[_index % _pool.size()]) + _index += 1 + return phrase + + +static func _load(path: String) -> Array: + if not FileAccess.file_exists(path): + return [] + var text := FileAccess.get_file_as_string(path) + # Instance parse() (not JSON.parse_string) so malformed input returns an + # Error code instead of the engine error the test config promotes to a fail. + var json := JSON.new() + if json.parse(text) != OK: + return [] + var data = json.get_data() + if typeof(data) != TYPE_DICTIONARY: + return [] + var lines = data.get("considering", []) + return lines if typeof(lines) == TYPE_ARRAY else [] +``` + +- [ ] **Step 4: Run to verify it passes** + +Run: `cd client && godot --headless --import >/dev/null 2>&1; ./run_tests.sh -gtest=res://tests/unit/test_considering_phrases.gd` +Expected: PASS (3 tests). + +- [ ] **Step 5: Commit** + +```bash +git add client/scripts/ui/considering_phrases.gd client/content/considering.json client/tests/unit/test_considering_phrases.gd +git commit -m "feat(client): ConsideringPhrases — authored rotating think-lines (§13)" +``` + +--- + +## Task 2: `ProxyConfig.request_timeout_seconds` + +**Files:** +- Modify: `client/scripts/net/proxy_config.gd` +- Test: `client/tests/unit/test_net_primitives.gd` + +**Interfaces:** +- Produces: `ProxyConfig.request_timeout_seconds() -> float` (a `ProjectSettings` key, default `TIMEOUT_DEFAULT := 35.0`; a non-positive/invalid value falls back to the default so a hang can never be re-enabled). Constants `TIMEOUT_SETTING`, `TIMEOUT_DEFAULT`. + +- [ ] **Step 1: Write the failing test** + +Add to `client/tests/unit/test_net_primitives.gd`: + +```gdscript +func test_request_timeout_default(): + var ProxyConfig = preload("res://scripts/net/proxy_config.gd") + if ProjectSettings.has_setting(ProxyConfig.TIMEOUT_SETTING): + ProjectSettings.clear(ProxyConfig.TIMEOUT_SETTING) + assert_eq(ProxyConfig.request_timeout_seconds(), ProxyConfig.TIMEOUT_DEFAULT) + + +func test_request_timeout_override_and_reject_nonpositive(): + var ProxyConfig = preload("res://scripts/net/proxy_config.gd") + ProjectSettings.set_setting(ProxyConfig.TIMEOUT_SETTING, 12.5) + assert_eq(ProxyConfig.request_timeout_seconds(), 12.5) + # A non-positive override must NOT re-enable the hang — falls back to default. + ProjectSettings.set_setting(ProxyConfig.TIMEOUT_SETTING, 0.0) + assert_eq(ProxyConfig.request_timeout_seconds(), ProxyConfig.TIMEOUT_DEFAULT) + ProjectSettings.clear(ProxyConfig.TIMEOUT_SETTING) +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `cd client && ./run_tests.sh -gtest=res://tests/unit/test_net_primitives.gd` +Expected: FAIL — `request_timeout_seconds` / `TIMEOUT_SETTING` not found. + +- [ ] **Step 3: Implement** + +In `client/scripts/net/proxy_config.gd`, add after the `base_url` block: + +```gdscript +const TIMEOUT_SETTING := "coc_rpg/proxy_request_timeout_seconds" +const TIMEOUT_DEFAULT := 35.0 + + +static func request_timeout_seconds() -> float: + # Just above the server's OLLAMA_TIMEOUT_SECONDS (30) so a slow model surfaces + # the server's own error first, while a fully-hung proxy is still bounded. A + # non-positive value would re-enable the infinite hang, so reject it. + var v := float(ProjectSettings.get_setting(TIMEOUT_SETTING, TIMEOUT_DEFAULT)) + return v if v > 0.0 else TIMEOUT_DEFAULT +``` + +- [ ] **Step 4: Run to verify it passes** + +Run: `cd client && ./run_tests.sh -gtest=res://tests/unit/test_net_primitives.gd` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add client/scripts/net/proxy_config.gd client/tests/unit/test_net_primitives.gd +git commit -m "feat(client): ProxyConfig.request_timeout_seconds (default 35s, reject <=0)" +``` + +--- + +## Task 3: `HttpTransport` applies the request timeout + +**Files:** +- Modify: `client/scripts/net/http_transport.gd` +- Test: `client/tests/unit/test_net_primitives.gd` + +**Interfaces:** +- Consumes: `ProxyConfig.request_timeout_seconds()` (Task 2). +- Produces: constructing `HttpTransport.new(http)` sets `http.timeout` to the configured value. Closes the `_http.timeout=0` gap; a hang now completes with `RESULT_TIMEOUT`, which the existing code maps to `DmResponse.failed()`. + +- [ ] **Step 1: Write the failing test** + +Add to `client/tests/unit/test_net_primitives.gd`: + +```gdscript +func test_http_transport_sets_request_timeout(): + var HttpTransport = preload("res://scripts/net/http_transport.gd") + var ProxyConfig = preload("res://scripts/net/proxy_config.gd") + var http := HTTPRequest.new() + autofree(http) + HttpTransport.new(http) + assert_eq(http.timeout, ProxyConfig.request_timeout_seconds()) + assert_gt(http.timeout, 0.0) # the whole point — no longer disabled +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `cd client && ./run_tests.sh -gtest=res://tests/unit/test_net_primitives.gd` +Expected: FAIL — `http.timeout` is `0.0` (the default), not the configured value. + +- [ ] **Step 3: Implement** + +In `client/scripts/net/http_transport.gd`, in `_init` (it already `preload`s `ProxyConfig`), set the timeout: + +```gdscript +func _init(http: HTTPRequest) -> void: + _http = http + # Bound a hung proxy so the request completes with RESULT_TIMEOUT (→ failed() + # → the service degrades) instead of never returning. Without this the + # considering-state indicator would rotate forever on a hang. + _http.timeout = ProxyConfig.request_timeout_seconds() +``` + +(Keep the rest of `_init`/`post_json` unchanged.) + +- [ ] **Step 4: Run to verify it passes** + +Run: `cd client && ./run_tests.sh -gtest=res://tests/unit/test_net_primitives.gd` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add client/scripts/net/http_transport.gd client/tests/unit/test_net_primitives.gd +git commit -m "fix(client): HttpTransport sets request timeout (no more infinite hang)" +``` + +--- + +## Task 4: `ConsideringIndicator` (node shim) + +**Files:** +- Create: `client/scripts/ui/considering_indicator.gd` +- Test: `client/tests/unit/test_considering_indicator.gd` + +**Interfaces:** +- Consumes: `ConsideringPhrases` (Task 1). +- Produces: `ConsideringIndicator.new(phrases := null)` (defaults to a fresh `ConsideringPhrases`); `start(label: Label)` shows the first phrase immediately and starts a repeating `Timer` (`ROTATE_SECONDS := 2.0`) that rotates the label; `stop()` stops the timer and clears the label. Extends `Node` (needs a `Timer` in the tree). Holds one `ConsideringPhrases` for its lifetime. + +- [ ] **Step 1: Write the failing test** + +```gdscript +# client/tests/unit/test_considering_indicator.gd +extends "res://addons/gut/test.gd" + +const ConsideringIndicator = preload("res://scripts/ui/considering_indicator.gd") +const ConsideringPhrases = preload("res://scripts/ui/considering_phrases.gd") + + +func test_start_shows_first_phrase_and_stop_clears(): + # Point phrases at a missing file so next() is deterministically LAST_RESORT. + var phrases = ConsideringPhrases.new("res://content/does_not_exist.json") + var ind = ConsideringIndicator.new(phrases) + add_child_autofree(ind) # _ready() creates the Timer child + var label := Label.new() + add_child_autofree(label) + + ind.start(label) + assert_eq(label.text, ConsideringPhrases.LAST_RESORT) # first phrase shown at once + + ind.stop() + assert_eq(label.text, "") # cleared on stop +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `cd client && ./run_tests.sh -gtest=res://tests/unit/test_considering_indicator.gd` +Expected: FAIL — script does not exist. + +- [ ] **Step 3: Implement** + +```gdscript +# client/scripts/ui/considering_indicator.gd +class_name ConsideringIndicator +extends Node +## Thin node shim (charter §13). While a call is in flight it rotates an authored +## ConsideringPhrases line onto a Label every ROTATE_SECONDS, reframing the wait +## as the GM thinking. The phrase logic is the pure, tested ConsideringPhrases; +## the Timer/tree wiring here is the untested shim (verified by running the +## harness). One instance persists on the harness so the rotation index is not +## reset per call (spec decision 4). + +const ROTATE_SECONDS := 2.0 + +var _phrases: ConsideringPhrases +var _timer: Timer +var _label: Label + + +func _init(phrases: ConsideringPhrases = null) -> void: + _phrases = phrases if phrases != null else ConsideringPhrases.new() + + +func _ready() -> void: + _timer = Timer.new() + _timer.wait_time = ROTATE_SECONDS + _timer.one_shot = false + _timer.timeout.connect(_advance) + add_child(_timer) + + +func start(label: Label) -> void: + _label = label + _advance() # show a phrase immediately, don't wait ROTATE_SECONDS + _timer.start() + + +func stop() -> void: + _timer.stop() + if _label != null: + _label.text = "" + _label = null + + +func _advance() -> void: + if _label != null: + _label.text = _phrases.next() +``` + +- [ ] **Step 4: Run to verify it passes** + +Run: `cd client && godot --headless --import >/dev/null 2>&1; ./run_tests.sh -gtest=res://tests/unit/test_considering_indicator.gd` +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add client/scripts/ui/considering_indicator.gd client/tests/unit/test_considering_indicator.gd +git commit -m "feat(client): ConsideringIndicator — rotates think-lines on a Timer" +``` + +--- + +## Task 5: Wire the indicator into both harnesses + +**Files:** +- Modify: `client/scripts/harness/narrate_harness.gd` +- Modify: `client/scripts/harness/npc_harness.gd` +- (No unit test — throwaway harnesses, verified by running.) + +**Interfaces:** +- Consumes: `ConsideringIndicator` (Task 4). + +- [ ] **Step 1: Wire `narrate_harness.gd`** + +It already has a `_status: Label`. Add the indicator field, build it in `_ready`, and wrap the await. + +Add the preload near the other consts: + +```gdscript +const ConsideringIndicator = preload("res://scripts/ui/considering_indicator.gd") +``` + +Add the field near the other vars: + +```gdscript +var _indicator: ConsideringIndicator +``` + +At the end of `_ready()` (after the status label is created), add: + +```gdscript + _indicator = ConsideringIndicator.new() + add_child(_indicator) +``` + +Replace the body of `_on_narrate_pressed()` with (the change: `start`/`stop` around the await instead of the static "…narrating…" text): + +```gdscript +func _on_narrate_pressed() -> void: + _button.disabled = true + _indicator.start(_status) + var r := await _service.narrate(_log) + _indicator.stop() + _label.text = r.display_text + for f in r.facts: + _log.add_fact(f) + _status.text = "facts harvested: %d%s" % [r.facts.size(), " (degraded)" if r.degraded else ""] + _button.disabled = false +``` + +- [ ] **Step 2: Wire `npc_harness.gd`** + +It has no status label. Add one, add the indicator, and wrap the await. + +Add the preload near the other consts: + +```gdscript +const ConsideringIndicator = preload("res://scripts/ui/considering_indicator.gd") +``` + +Add the fields near the other vars: + +```gdscript +var _status: Label +var _indicator: ConsideringIndicator +``` + +In `_ready()`, after the Speak button is added to the vbox, add a status label and the indicator: + +```gdscript + _status = Label.new() + vbox.add_child(_status) + + _indicator = ConsideringIndicator.new() + add_child(_indicator) +``` + +In `_on_speak()`, wrap the service call — start after appending "You:", stop before rendering Fenn's reply: + +```gdscript + _append("[b]You:[/b] %s" % utterance) + + var disposition := int(_game_state.npc_dispositions.get(NPC_ID, 0)) + var available := NpcContent.available_moves(_content.npc(NPC_ID), _game_state, _canon_log) + + _indicator.start(_status) + var r: NpcResult = await _service.speak( + NPC_ID, utterance, _canon_log, disposition, available) + _indicator.stop() +``` + +(The rest of `_on_speak()` — MoveApplier, fact harvest, appends, `ends_conversation` — is unchanged.) + +- [ ] **Step 3: Verify the project imports and the suite is green** + +Run: `cd client && godot --headless --import >/dev/null 2>&1; ./run_tests.sh` +Expected: all unit tests PASS (the harness changes must not break import/parse or any existing test). + +- [ ] **Step 4: Run the harness by hand (proxy + Ollama up)** + +In the Godot editor, run `npc_harness.tscn` (F6). Confirm: on Speak, the status line rotates through the authored think-lines (~2s cadence) while waiting; it clears and Fenn's prose appears on return. Then stop the proxy and Speak again: the line rotates, then after ~35s the harness shows the authored fallback (not an infinite spin). + +- [ ] **Step 5: Commit** + +```bash +git add client/scripts/harness/narrate_harness.gd client/scripts/harness/npc_harness.gd +git commit -m "feat(client): harnesses show the considering-state around the call" +``` + +--- + +## Self-Review + +**Spec coverage** — every spec section maps to a task: +- Decision 1–2 (reframe; loading is content) → Task 1 (`ConsideringPhrases` + `considering.json`). +- Decision 3 (rotate ~2s; pure core / node shim) → Task 1 (pure) + Task 4 (shim, `ROTATE_SECONDS`). +- Decision 4 (index is instance state, not reset) → Task 1 (running `_index`, test `test_index_persists…`) + Task 4 (one persistent instance). +- Decision 5 (timeout fix, default 35, reject ≤0) → Task 2 + Task 3. +- Decision 6 (no server change) → honoured; no server files in any task. +- Decision 7 (both harnesses) → Task 5. +- Components list → Tasks 1–5 one-to-one. +- Error handling (hang → timeout → degrade; missing file → LAST_RESORT) → Task 3 (relies on existing `test_transport_failure_degrades`, noted in constraints, not duplicated) + Task 1 (`test_missing_file_returns_last_resort`). +- Testing section → each task's tests; the Timer-fire path is explicitly run-verified in Task 5 Step 4, not unit-tested (matches the spec). + +**Placeholder scan** — no TBD/TODO. Task 5 has no unit test *by design* (throwaway harness), verified by Step 3 (suite) + Step 4 (by-hand run); every code step shows complete code. + +**Type consistency** — `ConsideringPhrases.next() -> String` and `.LAST_RESORT`/`.PATH` used identically across Tasks 1/4; `ConsideringIndicator.new(phrases)` / `start(label: Label)` / `stop()` match between Task 4 and Task 5; `ProxyConfig.request_timeout_seconds()` / `TIMEOUT_SETTING` / `TIMEOUT_DEFAULT` match between Tasks 2 and 3; `_status` is a `Label` in both harnesses (narrate already has it; npc adds it) so `start(_status)` type-checks.