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) <noreply@anthropic.com>
20 KiB
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— NOTJSON.parse_string()(the project's.gutconfig.jsonfailure_error_typesturns 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 existingHttpTransportresult != RESULT_SUCCESS → DmResponse.failed()path (already covered bytest_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/ConsideringIndicatorinstance persists on the harness;next()advances a running index thatstart()/stop()never reset. - Branch: all work on
feature/considering-state, branched offdev. Never commit todev/masterdirectly (CLAUDE.md §18); the human owns thedevmerge and push. - Client tests:
cd client && ./run_tests.sh(full) or./run_tests.sh -gtest=res://tests/unit/test_X.gd(one file). A newclass_namemay needcd client && godot --headless --importonce 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: addrequest_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_RESORTwhen the pool is empty/missing). ConstantsPATH,LAST_RESORT. -
Step 1: Write the failing test
# 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:
{
"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:
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
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(aProjectSettingskey, defaultTIMEOUT_DEFAULT := 35.0; a non-positive/invalid value falls back to the default so a hang can never be re-enabled). ConstantsTIMEOUT_SETTING,TIMEOUT_DEFAULT. -
Step 1: Write the failing test
Add to client/tests/unit/test_net_primitives.gd:
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:
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
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)setshttp.timeoutto the configured value. Closes the_http.timeout=0gap; a hang now completes withRESULT_TIMEOUT, which the existing code maps toDmResponse.failed(). -
Step 1: Write the failing test
Add to client/tests/unit/test_net_primitives.gd:
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 preloads ProxyConfig), set the timeout:
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
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 freshConsideringPhrases);start(label: Label)shows the first phrase immediately and starts a repeatingTimer(ROTATE_SECONDS := 2.0) that rotates the label;stop()stops the timer and clears the label. ExtendsNode(needs aTimerin the tree). Holds oneConsideringPhrasesfor its lifetime. -
Step 1: Write the failing test
# 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
# 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
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:
const ConsideringIndicator = preload("res://scripts/ui/considering_indicator.gd")
Add the field near the other vars:
var _indicator: ConsideringIndicator
At the end of _ready() (after the status label is created), add:
_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):
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:
const ConsideringIndicator = preload("res://scripts/ui/considering_indicator.gd")
Add the fields near the other vars:
var _status: Label
var _indicator: ConsideringIndicator
In _ready(), after the Speak button is added to the vbox, add a status label and the indicator:
_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:
_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
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, testtest_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.