27 Commits

Author	SHA1	Message	Date
Patrick Buckley	7a06f5e8bc	refactor(session): make ModelLane the provider boundary (#979) (#989) ... * refactor(session): make ModelLane the provider boundary (#979) ## Summary This closes the model-lane ownership gap left by #832: `ChatSession` no longer stores raw provider/client handles. `ResolvedModelBinding` now carries the provider, client, model, capabilities, registry generation, and backend-auth configuration as one coherent snapshot. - Atomically rebind existing sessions after model-registry changes while pinning each in-flight send, fallback, judge, output guard, task agent, title, compaction, perception, and voice operation to its initiating principal and binding. - Fence UI publication, canonical trajectory folds, durable writes, streams, retries, child scopes, and judge work by generation. Stop can hand off to a successor without accepting late state; cancelled tools retain typed effect receipts, and concurrent approval batches resolve by exact cycle or call. - Make create, fork, open, close, and delete race-safe with hidden `creating` reservations, incarnation-aware state tails, and an ACL-rechecked transaction that clones checkpoint-bounded history, configuration, project/persona state, and attachment references. - Extend REST/OpenAPI and Python/TypeScript SDK contracts for create/fork inputs, routed-create metadata, live-workstream probes, targeted approvals, and structured cancellation results. - Update architecture, storage, authentication, judge, channel, console, API, and SDK documentation, including regenerated architecture diagrams and OpenAPI artifacts. ## Validation - SQLite suite: 11,188 passed, 9 skipped, 10 deselected - PostgreSQL suite: 11,195 passed, 2 skipped, 10 deselected - Live backend: 3 passed - SSE recovery: 6 passed; browser recovery harness passed all scenarios - Ruff: clean; 595 files correctly formatted - mypy: 243 source files clean - TypeScript: typecheck/build and 35 tests passed - OpenAPI artifacts fresh; all 14 changed diagrams reproduce byte-for-byte - `git diff --check` and Git LFS integrity clean Closes #979. * fix(deps): update nanoid for GHSA-2v37-7h3g-55p8 Refresh the transitive lock entry admitted by PostCSS so the TypeScript security gate no longer resolves the vulnerable custom-generator implementation. Validation: - npm ci - npm audit --audit-level=moderate: 0 vulnerabilities - TypeScript typecheck and build - TypeScript tests: 35 passed * fix(test): assert canonical model registry URLs Replace prefix checks with exact canonical base URL assertions so the tests do not model incomplete URL validation. Validation: tests/test_model_registry.py (185 passed); Ruff check/format; mypy.	2026-08-08 16:13:35 -07:00
Patrick Buckley	0d52b63b50	feat(coordinator): nudge a coordinator that goes idle holding unfinished work ... Two nudge classes can fire from one IDLE event, tasks first, each asserting only its own domain. idle_tasks (advice) fires when open (pending/in_progress) tasks exist. The body is a counts opener, the open task ids with statuses, and typed branches that each end in a runnable tasks(...) or wait_for_workstream(...) call populated with real server-minted ids. Everything it says about children is governed by one observed fact: live children present adds the caveat sentence and the blocked-on-a-child branch; affirmatively none says nothing about children at all. Any needs_user row parks the class entirely — at the fire gate and the drain predicate — because with no task graph an open task may be gated on a parked one's unanswered question; the operator's answer is the re-arm. Gated on memory.nudges and on the persona actually exposing the tasks tool; carries the per-class cooldown as well as the per-bracket cap. The tasks tool itself gains the needs_user status and a note field — the typed escalation the body's branches point at. idle_children (liveness) fires when children are in a live state — the wake that lets an idle coordinator collect a finished child's results. The body is a roster of workstream id prefixes and states, never names: a child's name is model-authored text and does not enter a system turn. Cap-only and cooldown-free by design, not gated on memory.nudges, and it survives an operator Stop. Fail-closed, event-wide: if any storage read fails while the observer handles an IDLE event, neither nudge is queued and neither cap is charged. Both paths run as side-effect-free plans; the commit tail is storage-free, so no read can fail past the veto point; both drain predicates drop on a failed read. A path's own fault (a generic raise) still costs only that path's fire, so one class's bug cannot strand the other. Task text is stored verbatim and projected per audience at render: the model-facing projection deletes angle brackets, the operator projection keeps them, and both strip newlines and bidi/zero-width runs. Idle cards render what the model was told, formatted for the operator, never augmented with content the model did not receive.	2026-07-29 22:11:38 -07:00
Patrick Buckley	3803feb008	fix(console): uniform not_found wait-entry keys + ws_ids param precision ... PR #652 review follow-ups: - not_found snapshot entries now carry the full key set (updated/name empty) so results[ws_id] is shape-uniform across states; pinned by a key-set assertion in the sentinel test - ws_ids param text now distinguishes malformed (fails before any waiting) from well-formed-but-unobservable (first-tick abort) at unchanged length — per-param descriptions stay lean by policy	2026-06-10 22:03:08 -07:00
Patrick Buckley	d0e9aa3dbe	fix(console): coordinator ws-ref validation, did-you-mean recovery, wait fail-fast ... Field incident: the coordinator LLM hand-copied a child ws_id and collapsed its aaa run to a, producing a 30-char id. inspect said "not found", wait called it "denied", neither offered recovery, and the model concluded the child was dead and dropped the lane — silent report degradation while the child kept working. - validate model-supplied ws_id args at the tool boundary (send/close/cancel/delete/inspect/wait): full 32-hex ids pass through at unchanged storage cost; a child's exact legacy id still resolves; anything else fails fast with a did-you-mean (capped Levenshtein <=3 over the coord's own children) plus a child roster. Near-misses never auto-resolve; display names are not addresses (mutable, non-unique) — a name ref errors with a pointer at the right id - wait_for_workstream: rename per-entry state "denied" -> "not_found" with an honest sentinel; malformed refs error before any waiting (invalid_ws_ids); a well-formed id that is foreign, missing, or hard-deleted mid-wait aborts the wait on the tick that observes it instead of burning the timeout (mode=all was unsatisfiable) or riding along to complete=True (silent lane loss); mode=all completes only when every id is real-terminal; entries carry the child display name - one not-found payload across all verbs: foreign and nonexistent stay byte-identical (no existence oracle), hints reference only the coord's own children, echoed refs clipped in error strings; invalid_ws_ids and not_found share one per-ref shape with the roster hoisted top-level - inspect ownership now requires user_id parity via _row_in_own_subtree, matching the wait/mutating gates (#506) — closes the forged-parent cross-tenant read - session exec serializes the structured recovery payload (results + did_you_mean + children) on unresolvable-id wait errors instead of collapsing to the bare error string - tool JSON descriptions + coordinator docs updated to the new contract; incident regression test pins the captured aaa-collapse ids	2026-06-10 22:03:08 -07:00
Patrick Buckley	5bd7af6b73	feat(session): path-key /rewind + /retry into shared verb handlers (#549) ... Lift the conversation-modifying /rewind and /retry verbs out of the body-keyed POST /v1/api/command into path-keyed POST /v1/api/workstreams/{ws_id}/rewind ({turns:N}) and /retry, as make_rewind_handler/make_retry_handler in SharedSessionVerbHandlers (template: make_close_handler/make_cancel_handler), wired on both interactive and coordinator kinds. Closes the last unlifted conversation-modifying surface — coordinator workstreams gain rewind/retry where they had none — and removes the surviving exception to the post-#422 path-keyed URL convention. Handler shape: auth gate (coord -> admin.coordinator via permission_gate; interactive -> conversation.modify via accepted_permissions) -> busy-gate -> session.rewind(n)/retry() -> always emit clear_ui (incl. rewind-to-zero, carries #503) -> audit (conversation.rewind/retry on both kinds). Retry re-dispatch reuses the shared session_worker.send via a per-kind dispatch_retry closure (hard-reject on busy), not a third hand-rolled thread. The web /command handler now rejects /rewind+/retry with a pointer to the path-keyed endpoint (BREAKING; 1.6.0aN-tolerant); session.handle_command's branches stay for the terminal CLI. auth.py adds the verbs to both write suffix-sets; Python + TS SDKs, OpenAPI (RewindRequest + server/console specs), the /route/ proxy mounts + audit actions, and coordinator_client all gain them. Interactive frontend (app.js): the 3 /command POST sites + the hand-typed-slash reroute now hit the path-keyed endpoints; the bare .msg.user rewind selector is kept (matches the server's _find_turn_boundaries, which counts system-nudge user turns). The coordinator frontend rewind UX lands in a follow-up commit (browser-verified). Tests: route-walk mount/order, /route/ audit rows, required_scope, OpenAPI catalog, SDK body-inspection, and HTTP-level handler behavior (busy-gate, turns validation, clear_ui emit, retry dispatch, audit invocation + swallow).	2026-05-28 21:02:51 -07:00
Patrick Buckley	e1c2a05467	fix(coord): strip intent-judge verdicts from inspect_workstream output (#580) ... * fix(coord): strip intent-judge verdicts from inspect_workstream output Coordinator LLMs repeatedly misread `user_decision="policy"` (the label meaning "auto-approved by an admin policy allow rule") as "blocked, waiting for policy review" — combined with `recommendation="review"` (the heuristic judge's risk class, not a workflow state) the verdict fields read end-to-end as "stuck on policy review" and produced incorrect cancel-and-respawn reasoning against healthy children. The blocking signal already lives on `state` (`"attention"`) and the `live.pending_approval` block, both still in the result. Verdict history remains queryable through admin / audit surfaces — only the LLM-facing inspect surface drops them. Also drops `verdict_count` / `verdicts_by_risk` from the tier-3 skeleton fallback, deletes the now-dead `_serialize_verdicts` helper, and clears the now-stale `"verdicts": []` keys from 10 fixture sites that fed `_format_inspect_tiered` test cases. * fix(coord): correct comment pointer — inline comment, not docstring	2026-05-23 19:23:58 -07:00
Patrick Buckley	471d48abd9	feat(skills): unify skill + list_skills into dual-kind action-multiplexed tool ... Replaces the legacy `skill` (load + search) and `list_skills` tools with a single `skills(action=...)` tool serving both interactive and coordinator sessions. Stacks on the model.skills.write permission introduced in PR 1. Tool surface - `find`: filter by category/tag/risk_level/enabled_only/limit with optional BM25 query ranking; auto-approved on both kinds; kind-scoped at the storage filter (interactive sees interactive+any, coord sees coordinator+any). - `get`: fetch a single skill including content; cross-kind misses collapse to "not found" so a model can't enumerate the other surface by name-probing. - `load`: activate a skill in the current session (interactive-only; coord sessions get an explicit hint pointing at spawn_workstream). - `create`/`update`/`enable`/`disable`: require approval AND model.skills.write; permission re-checked at exec time to catch a revocation between approval and write. - No `delete` — hard-delete stays admin-UI exclusive; tool description documents the soft-delete-via-disable pattern. Defenses on the write surface - Approval cards surface projected risk_level (scanner re-run against the proposed final state) and warn explicitly when allowed_tools + auto_approve combine (auto-fire-on-load consequence is spelled out, not just shown as raw field values). - Toggle preview surfaces existing risk_level + allowed_tools count so re-enabling a critical-tier skill is never a one-click bypass. - Update path now re-fetches the row at exec to catch a readonly flip between approval and write, filters updates back to the runtime-only set if so, refuses if no fields survive. - Update path rejects empty content (hollow-out via emptying bypassed the soft-delete-via-disable invariant), non-list tags, and empty category — failures are loud rather than silent. - Permission denials audit `skill.write_denied` with actor_source=model so probing the permission state leaves a trail. Audit failures log at error (not warning) — a successful write without a row is the exact gap the trail exists to surface. - `_skill_hint` routes both message and system_reminder through escape_wrapper_tags so caller-controlled values can't close the <system-reminder> envelope and let the model fabricate directives in its own future context. Shared validation - `parse_skill_session_config` lifted from console/server.py to turnstone/core/skill_field_validation.py; both the HTTP admin path and the model-tool path consume it. Single source of truth so field rules can't drift between layers. - `SKILL_RUNTIME_CONFIG_FIELDS` lifted similarly (was duplicated as _SKILL_RUNTIME_CONFIG_FIELDS in server.py and _SKILLS_READONLY_FIELDS on ChatSession). - `notify_on_complete` validator now accepts list input from the JSON schema's `array` type — previously rejected because str() of a list yields Python repr that json.loads then refuses. Performance - Update prepare skips the projected-risk scan when neither content nor allowed_tools is changing (storage re-scans on write authoritatively). Metadata-only updates no longer pay the ~25 regex-pass scan cost. Cleanup - CoordinatorClient.list_skills deleted (-91 lines); model-tool path talks to storage directly via list_skills_filtered. - Roles admin UI gains a Model section exposing model.skills.write. - tests/test_load_skill.py renamed to tests/test_skills_tool.py and rewritten for the new tool — 48 tests covering registration, prepare dispatch, permission gating (including TOCTOU-revoked exec deny), audit actor_source on create + disable + permission-denied probe, BM25 ranking, invalid-kind branches, audit-failure swallow, and <system-reminder> envelope injection resistance.	2026-05-22 16:25:52 -07:00
Patrick Buckley	d2f5db092d	feat(coord): three-tier compression for inspect_workstream output ... A coord doing a fan-out wave of inspect_workstream calls against tool-heavy children could blow the context budget on raw output alone (one child with a 100 KB bash result × N children). The previous safety net was ``_truncate_output``'s head+tail strategy, which silently drops *middle* messages — exactly the wrong shape for a coordinator trying to understand a child's trajectory (the LAST message tells the model what the child concluded; the FIRST sets the brief; the middle is the connective tissue). Three-tier degradation modeled on the search tool's pattern at ``session.py:_format_search_results``: Tier 1 (full): every message verbatim — used when size fits. Tier 2 (compact): per-message head/tail-snipped content (600/300 chars) plus snipped ``tool_calls.arguments`` (300/100 chars). When content snipping alone doesn't fit, fall through a message-list trim ladder ((20,30) → (10,20) → (5,10)) that keeps head + tail messages and elides the middle as ``{"_omitted": N}``. Tier 3 (skeleton): no messages — counts + role distribution + verdicts-by-risk + last assistant preview. Budget 32 KB (matches ``_SEARCH_OUTPUT_BUDGET``). First emission whose JSON serialization fits the budget wins. ``_tier`` lands on every non-error emission so the coordinator LLM and audit readers can see which compression rung was selected; ``_tier_note`` carries actionable advice (re-call with a smaller ``message_limit`` etc.). Error-shape results bypass tiering — they're already small. Bug fixes caught during review: - ``_compact_message`` now preserves the assistant-side ``tool_calls`` list with snipped ``function.arguments``; the pre-fix shape left audit readers with tool-result orphans against invisible calls. - The intermediate Tier-2 list-trim ladder fixes a size-monotonicity bug where Tier-2 with un-snippable content (per-message body under the 964-char threshold) plus the added ``_tier_note`` came out STRICTLY larger than Tier-1, falling through to skeleton when a head+tail trim would have preserved dozens of messages. - ``_inspect_skeleton`` reads ``result["skill_id"]`` (production storage row key) with a ``skill`` fallback; pre-fix it read ``skill`` only and emitted ``null`` for every real workstream.	2026-05-18 19:35:25 -07:00
Patrick Buckley	99a309ed70	fix(coord): omit empty allowed_tools in list_skills + clarify semantics ... A skill with `allowed_tools=[]` in the coordinator's `list_skills` response read as "no tools are usable by this skill" to a model that didn't know the semantics — but the actual meaning is "no tools are pre-approved for auto-approval (auto-approve exemption list)". Real misdiagnosis incident: a code-review child appeared to have been spawned with zero tool access when in fact the skill simply hadn't declared an auto-approve allowlist. Two-part fix: - `coordinator_client.list_skills` omits the `allowed_tools` key from the per-skill dict when empty. Absence now carries the unambiguous meaning "no tool is pre-approved for this skill"; presence (with a non-empty list) keeps the standard Claude Code skill-spec shape. - `turnstone/tools/list_skills.json` description rewrites the field doc so the LLM sees: "tool names exempt from the operator approval gate ... the field is OMITTED when empty: a skill without `allowed_tools` still has access to every tool in its session's toolset; absence of the field means no tool is pre-approved for this skill, not that the skill has no tools." Field name stays `allowed_tools` — matches the upstream Claude Code skill frontmatter (`allowed-tools` hyphenated, stored as `allowed_tools` internally per `skill_parser.py:241-242`). Parser, storage column, admin UI, and SDK unchanged.	2026-05-18 19:33:28 -07:00
Patrick Buckley	023606b968	feat(console): event-driven wait_for_workstream + idle cleanup via ChildEventBus ... Retire two polling patterns in coord that have clean event sources. PR 2 of 3 in the coord-completion stack; sits on top of PR #505 (reactive node discovery via PG LISTEN/NOTIFY). `wait_for_workstream` (coord's block-wait tool) polled storage every 0.5 s in a worker thread regardless of whether anything had changed — a 600 s wait incurred ~2400 round-trips. Now subscribes to a new in-process `ChildEventBus` (`turnstone/core/child_event_bus.py`) and blocks on `threading.Event.wait(min(remaining, WAIT_HEARTBEAT_INTERVAL))`: - `CoordinatorAdapter` owns the bus; `_dispatch_child_event` calls `bus.notify(child_ws_id)` after each `_enqueue_on_ui` for the state-class branch (cluster_state, ws_closed, ws_rename, intent_verdict, approval_resolved, approve_request). - Wait loop clears the Event BEFORE the storage snapshot to close the subscribe/check race; a notify between clear and the next `wait()` leaves the Event set so the loop re-reads without losing the wake-up. - 2 s heartbeat cap preserves the existing `wait_progress` SSE cadence for the sidebar UI while cutting SSE traffic ~4x vs the pre-bus 500 ms cadence in the quiescent case. - Worst-case completion latency is 2 s (vs pre-bus 0.5 s) because `set_state` buffers non-ERROR writes through `StateWriter` (async-flushed) while `emit_state` fans out immediately — a bus-driven wake can beat the flusher and read pre-transition state, then re-block until heartbeat. Deliberate trade-off; the SSE-traffic reduction outweighs the regression on the most common terminal transition. - Defense-in-depth: ownership-filter `cleaned` to own-subtree before `register_waiter` so a foreign ws_id passed by an untrusted coord LLM (prompt injection) can't observe wake-up timing as a side channel. Predicate (`_row_in_own_subtree`) requires both `parent_ws_id == coord_ws_id` AND `user_id == coord_user_id` parity — same gate strength as the existing `_is_own_subtree` mutating-op guard, so a corrupted / cross-tenant `parent_ws_id` alone can't satisfy it. Shared with `_snapshot_all` so the snapshot's `denied` shape stays in lockstep with the bus filter (Copilot review on #506). Coord idle-cleanup thread polled the storage scan every `check_every` seconds (~30 s on default 2 h timeout) even when no coord was anywhere near idle. Now subscribes to `SessionManager._state_subscribers` with a `tick_now` event and blocks on `tick_now.wait(check_every)` — any state change wakes the sweeper without waiting a full interval, AND the timeout still fires the periodic sweep for the DB-orphan-only case. A `min_sweep_interval=5 s` floor bounds DB-call traffic at ~0.2/s under sustained activity so the loop can't tight-spin `close_idle` at the rate of its own DB latency (6x improvement over the pre-refactor fixed 30 s cadence under any activity, and prompt state-change-driven wakes when below the floor). `CoordinatorClient` constructor takes `child_event_bus` as a required kwarg — there's no external SDK shape to preserve and keeping it optional would silently mask a wiring bug in any future caller. Tests construct their own `ChildEventBus()` per fixture. Tests: 16 unit tests for `ChildEventBus` (register / unregister symmetry, multi-waiter fan-out, multi-child waiter, subscribe/check race, concurrent register / notify smoke); 7 new adapter tests (bus notify fires for all 6 state-class events, drops for unknown child / wrong ws_id); 7 new coord-client wait tests (subscribe- after-terminal, notify wakes, unrelated notify doesn't wake, heartbeat fires without notify, unregister on exit, multi-waiter independence, cross-tenant denial via the user_id-parity filter); 8 idle-cleanup tests (initial sweep, heartbeat cadence, exception swallowing, stop_event clean exit, state-change wake, subscriber cleanup, mid-sweep wake, `min_sweep_interval` floor). All pass; ruff + mypy clean. Full non-live suite: 6227 passed (+2 vs prior baseline).	2026-05-11 01:06:47 -07:00
Patrick Buckley	47cf6dea24	feat(coord): expose healthy model aliases per node on list_nodes (#466) ... * feat(coord): expose healthy model aliases per node on list_nodes Surfaces a `model_aliases` field on each `list_nodes` row so a coordinator can discover which model aliases each cluster node will accept on `spawn_workstream(model=...)` without an HTTP fan-out. Each server projects its registry into a `models` entry on `node_metadata` (`{alias, provider, healthy}` per alias) at lifespan startup, on every 30s heartbeat tick, and after `internal_model_reload`. The publish helper short-circuits on a payload-equality cache so a stable cluster doesn't pay UPSERT churn — exposed via the new `turnstone_node_models_publish_total{outcome="written|skipped"}` Prometheus counter so operators can graph cache hit-rate. Coord client filters the per-alias rows to healthy aliases only and drops the provider-side model identifier (`cfg.model`) — coords kept reaching for it when they should pass the local alias. * fix(coord): address Copilot+CodeQL feedback on list_nodes models work - internal_model_reload: reuse a single get_storage() local across the registry load and the metadata publish (Copilot:3047) - _collect_node_models_metadata: iterate sorted aliases so two structurally identical registries built in different insertion orders serialize to the same JSON — directly improves the publish-cache hit rate exposed via turnstone_node_models_publish_total (Copilot:3105) - tests: drop mixed turnstone.server import style flagged by CodeQL — hoist _metrics into the from-import block, and use sys.modules in the shutdown-race regression test instead of `import as srv`	2026-05-02 19:54:13 -07:00
Patrick Buckley	7d6b31e18a	fix(coord): close gaps an operator's harness shakedown surfaced (#444) ... * fix(coord): close gaps an operator's harness shakedown surfaced Operator-driven shakedown of the coordinator tool surface flagged five issues; this commit addresses all of them plus the review findings against the initial fix. 1. Cancelled-mid-stream partial assistant content now carries a "[generation cancelled before completion]" marker. Without it, ``inspect_workstream`` / ``wait_for_workstream`` callers and the next coord-LLM turn read the truncated text as a complete answer. ``_cancelled_partial_msg`` no longer ships ``_provider_content`` (Anthropic would otherwise read that lane verbatim and bypass the marker; partial tool_use blocks could also leak through). 2. ``spawn_workstream`` / ``spawn_batch`` no longer surface the routing-proxy ``status`` field (always HTTP 200 on the success path). The tool description claimed it was "lifecycle state at creation"; code that did ``if result["status"] == "idle"`` silently never matched. Lifecycle state lives on the workstream row — ``inspect_workstream`` is the read. Tool JSON descriptions plus docs/coordinator-skills.md and docs/bulk-endpoints.md examples updated to match. 3. ``inspect_workstream`` not-found error string is bare ("workstream not found"); the structured ``ws_id`` field carries the queried id. Pre-fix the error STRING echoed the id back at the caller who just sent it — redundant and out of step with the rest of the surface. Cross-tenant + missing rows still return the same shape, preserving the existence-leak guarantee. 4. ``tasks(...)`` is now rejected when called in a parallel tool batch. The prior shape relied on a docstring warning ("a list paralleled with writes can reflect pre-write state") that put cognitive overhead on every model invocation; turning the silent footgun into an explicit error means the model only thinks about the rule the moment it actually breaks it. Warning dropped from the tasks tool description. ``_PARALLEL_INCOMPATIBLE_TOOLS`` constant in session.py is the extension point for any future tool with the same read-after-write hazard. Plus the multi-stage code review's findings against the initial fix (q-1 / q-2 docs drift, q-3 idiom, q-4 keys-assertion, q-5 duplicate guard) — all addressed in the same pass. Tests: 4752 pass, +6 net since the pre-fix baseline. Ruff + mypy clean. Three new tests pin the parallel-batch-rejection behaviour on tasks (rejected when batched, runs alone, sibling tools unaffected); existing cancel + spawn + inspect tests updated to match the new shape. * fix(coord): close two copilot review gaps on PR 444 Copilot review on PR 444 flagged two follow-ups: 1. Empty-content cancel divergence — when ``GenerationCancelled`` races BEFORE the first content token, the prior shape skipped ``save_message`` and only appended an empty-content msg in memory. In-memory and storage diverged: a rehydrate would see nothing in storage but the session would carry an empty assistant turn. Both branches now persist; on the empty-content shape the marker becomes the entire message ("[generation cancelled before completion]") so storage matches the in-memory history. 2. Test stub cleanup — three new tests injected ``ui.approve_tools`` via ad-hoc ``lambda + type: ignore[attr-defined]``. Replaced with a permissive ``approve_tools`` method on ``_StubUI`` so the stub matches the SessionUI surface the dispatcher actually reads. Tests that exercise approval pathways can still override per-instance. Tests: 4752 pass. Ruff + mypy clean.	2026-04-28 12:52:36 -07:00
Patrick Buckley	9a30530d41	feat(coord): surface child errors, isolate tool exceptions, add memory tool (#443) ... * feat(coord): surface child errors, isolate tool exceptions, add memory tool Closes four coordinator gaps identified during operator triage: 1. Child workstream errors now surface in inspect/wait. Worker-thread exception text is sanitized (URL userinfo masked, sk-/Bearer/ghp_/ github_pat_/AKIA tokens redacted, capped at 1024 chars) and persisted to workstream_config.last_error before _emit_state("error") fires, so coord polling never sees state=error with a missing cause. The row is cleared on recovery transitions (idle/running) so a once-leaked exception body doesn't outlive the failure. inspect_workstream and wait_for_workstream return last_error for state=error rows; the wait surface prefers it over the assistant-tail walk. 2. Tool exceptions now return as tool_results with sibling-aware guidance. ChatSession._safe_prepare_tool wraps every per-call _prepare_tool invocation; a buggy preparer becomes an error item for that call only — sibling parallel tool_calls keep going, never orphaning the assistant message's tool_calls block. run_one's runtime exception path includes the exception class and a short note that other tool calls in the batch completed independently so the model can recover. 3. Memory tool exposed to coordinator with a coord-only scope. memory.json gains coordinator: true + interactive: true + per-kind kind_variants. Coord sessions see scope enum ["coordinator"] and an orchestration-flavored description; IC sessions see ["global", "workstream", "user"] and the existing flavor. Coord-scope rows are private to the coordinator session (children cannot read or write them), closing the cross-session prompt-injection lane that an adversarially-steered child would otherwise have. Coord visibility is also restricted to coord-scope only — coords no longer see global / workstream / user memories that belong to the user's interactive sessions. 4. Per-call exception isolation in tool batches. _safe_prepare_tool was previously the implicit shield; now it's an explicit method with documented invariants. KeyboardInterrupt / GenerationCancelled re-raise so the cooperative cancel path still works. Other notable changes: - LAST_ERROR_CONFIG_KEY + persist_last_error / clear_last_error / load_last_error / sanitize_error_text moved to turnstone.core.memory (the storage facade hub) — readers in coordinator_client.py import the constant. - Memory scope tuples extracted to module constants _VALID_MEMORY_SCOPES and _IMPLICIT_SCOPE_WALK; seven inline duplicates collapsed. - tools.py grows _apply_kind_variant for the per-kind tool surface; tools without kind_variants pass through unchanged (no spurious deep-copies). - Session adds _coordinator_scope_id, _default_memory_scope, _implicit_scope_walk, and _record_fatal_error chokepoints so the worker-thread fatal path is one site rather than three. - Removed duplicate on_error / on_state_change emits from session_routes.py and coordinator_adapter.py — session.send()'s _record_fatal_error owns the sequence now. Tests: 4742 pass (no live), +30 net since the baseline. Ruff + mypy clean on every modified production file. * fix(coord): redact secrets in tool error paths via output_guard Copilot review flagged two paths where ``str(exc)`` flowed back into the model-facing tool_result without going through the credential- redaction the new fatal-error path applies: - ``ChatSession._safe_prepare_tool``: a preparer-side exception becomes an error item whose ``error`` field embedded the raw exception text. - ``ChatSession._execute_tools.run_one``: a runtime tool exception became an ``Error executing X: <e>`` tool_result, again with the raw exception text. Both now route through ``sanitize_error_text`` (sanitised log line + sanitised tool_result), and ``sanitize_error_text`` itself was refactored to delegate to ``output_guard.redact_credentials`` instead of carrying its own parallel regex catalog — the audit log + post-tool guard already use that pattern set, so the credential definition stays in one place. Also extended ``_RE_CONNECTION_STRING`` in ``output_guard`` to cover ``http(s)://user:pass@host`` so a misconfigured ``OPENAI_BASE_URL`` that lands in an httpx ``ConnectError.__str__`` is redacted by every caller of ``redact_credentials`` (audit details, close-reason persistence, last_error, the two tool error paths). The host (useful for triage) survives; only the password is replaced with the standard ``[REDACTED:password]`` marker. Tests: full suite (4745 pass), ruff + mypy clean. Two new tests pin the redaction behaviour in both tool error paths so a future refactor can't drift back to leaking ``str(exc)`` verbatim.	2026-04-28 12:19:34 -07:00
Patrick Buckley	dea2729292	refactor(coordinator): rename task_list → tasks, doc/prompt sweep (#437) ... Four themes from a coordinator-feature shakedown: 1. Correctness fixes (return shapes / examples / behavior) - tools_coordinator.md: drop fake skill names from spawn examples; fix wrong kwarg ``node_id=`` → ``target_node=``. - wait_for_workstream.json: document ``message`` + ``truncated`` per-ws fields (always enriched in the client; the JSON shape lagged the docstring). - cancel_workstream.json: document the conditional ``dropped`` payload — ``was_running`` always present when ``dropped`` is, ``pending_approval`` and ``queued_messages`` conditional sub-shapes. - spawn_workstream.json: document full return shape including ``routing_strategy ∈ {rendezvous, target_node, resume}`` and ``status``. - close_all_children.json: clarify ``skipped`` covers BOTH hard-deleted children AND already-closed-and-evicted children (wire shape doesn't distinguish); drop incorrect "echoed back in response" claim — server returns ``{status, closed, failed, skipped}``, never echoes ``reason``. - console/server.py: comment in ``_fanout_on_children`` clarifying that the 400 "No session" branch fires for cancel-cascade callers and is unreachable from close_all_children (close handler 404s instead). - coordinator_client._utc_now_iso(): switch to bare ISO format matching the rest of the storage row format used in the codebase. 2. Tightened the 11 longest tool descriptions (~23% cut on the coord set). Removed ALL-CAPS emphasis, normalised em-dashes, dropped informal phrasing. No new claims. 3. Removed static approval annotations from descriptions. Approval is governed at runtime by the unified ``approve_tools`` body and admin-defined ``tool_policies`` (#436); static "Auto-approved" / "Approval required" / per-action approval tags become a stale signal. Field names (``pending_approval``) and operational verb behaviour ("cancel unblocks pending approvals") stay. 4. Renamed ``task_list`` coord tool → ``tasks``. The previous name compounded the bare word ``task`` (which collides with chat-template channels on local models — same reason ``task_agent`` carries the suffix); the plural form sidesteps the collision and reads more accurately, since the tool acts on the whole list rather than a single task. Sweep covers tool JSON, Python methods (5 client methods + 2 session methods + 1 helper + 1 constant), audit event name (``task_list.update`` → ``tasks.update``), log tag (``task_list.corrupt_envelope`` → ``tasks.corrupt_envelope``), frontend SSE event matcher, prompts, docs, and tests. CHANGELOG entry added. Plus: dropped the ENV block (Output Environment / Available rendering / Formatting principles) from coordinator system prompts. Coordinators orchestrate rather than render rich output to the user, so the rendering capability matrix is not actionable for them. Coord prompt drops ~29% (6309 → 4493 chars). SDK regeneration via ``generate-types.py`` updates both ``openapi-console.json`` (the rename's downstream change) and ``openapi-server.json`` (PR #436 drift — its merge added ``pending_approval_detail`` + ``recent_auto_approvals`` fields to the Python schemas but didn't regenerate the JSON artifact). ## Behavior changes (operator-visible) - Audit event name: ``task_list.update`` → ``tasks.update``. Audit dashboards / SIEM filters / log greps that pinned the old prefix should update. - SSE ``tool_result`` events now ship ``name="tasks"`` for the scratchpad tool. The bundled coord-tree UI is updated atomically; external consumers reading SSE events by tool name need to update. - Existing task envelopes in production storage have ``+00:00`` timestamps from the old ``_utc_now_iso``. New writes are bare; old rows are not backfilled. Within an envelope you may briefly see mixed formats until each row is re-touched. No code path string-compares timestamps within an envelope, so this is cosmetic. ## Validation - ``ruff check`` + ``ruff format --check`` clean - ``mypy turnstone/`` clean (175 source files) - ``pytest -m "not live"`` — 4679 passed, 3 deselected	2026-04-27 22:51:31 -07:00
Patrick Buckley	91b07aaf4b	feat(coord): bundle child last-message inline in wait_for_workstream ... Each per-ws snapshot now carries `message` + `truncated` so the coordinator LLM doesn't need a follow-up `inspect_workstream` round-trip per child to read what came back. idle/error states return the last assistant turn (capped at 6 KiB UTF-8 bytes, truncated from the end); closed/denied return a sentinel; running children carry null. Storage reads for idle/error parallelize across an 8-worker thread pool so a 32-child fan-out lands in 4 batches instead of 32 sequential round-trips.	2026-04-27 13:56:16 -07:00
Patrick Buckley	3b92c96b31	fix(coord): align coord client routes with post-#422 path-keyed mounts ... #422's legacy URL adapter removal deleted the body-keyed /v1/api/route/{verb} endpoints (with ws_id in JSON body) but turnstone/console/coordinator_client.py still pointed at them. The coord LLM's close_workstream / close_all_children tools 404'd; send / approve / cancel were equally broken though exercised less often. _ROUTE_PATHS now uses {ws_id}-templated path-keyed forms: send → /v1/api/route/workstreams/{ws_id}/send approve → /v1/api/route/workstreams/{ws_id}/approve cancel → /v1/api/route/workstreams/{ws_id}/cancel close → /v1/api/route/workstreams/{ws_id}/close _post() interpolates {ws_id} at call time when the template has the slot; body-keyed paths (delete, close_all_children) still work via the same code path. Each affected caller (send, approve, cancel, close_workstream, close_all_children) was updated to pass ws_id as the kwarg and drop ws_id from the body. Added test_route_paths_match_actual_console_mounts: walks the real Starlette app's routes and asserts every _ROUTE_PATHS entry corresponds to an actually-mounted route. Catches the next URL unification drift before runtime. Updated the existing literal assertions + path-checking tests for the new shape. Pre-existing bug surfaced while testing inline-child-approvals.	2026-04-27 11:41:14 -07:00
Patrick Buckley	df7c0c2f44	refactor(server): delete legacy /v1/api/coordinator/ URL tree ... Stage 2 Priority 0 Steps 0.4–0.7 — collapses the four migration steps into one commit since they have to land together. The legacy ``/v1/api/coordinator/`` URL prefix never shipped in a stable release (it appeared in 1.5.0aN experimental), so there's no compat carry- forward — just rip and replace. What moves: - Step 0.4: deletes the eighteen ``Route("/api/coordinator/...")`` entries from ``console/server.py``. Coord traffic now flows exclusively through the unified ``/v1/api/workstreams/`` shape mounted via ``register_session_routes`` + ``register_coord_verbs`` (Steps 0.2 and 0.3). - Step 0.5: rewrites the OpenAPI spec (``console_spec.py``) and schemas (``console_schemas.py``, ``server_schemas.py``) to document the new paths. ``test_openapi.py`` parity assertions swap with them. - Step 0.6: mechanical URL sweep across the frontend (``console/static/app.js`` — 9 sites; ``coordinator/coordinator.js`` — 16 sites; ``index.html`` — 1 comment). - Step 0.7: same sweep across the test suite (``test_coordinator_endpoints.py``, ``test_coordinator_end_to_end.py``, ``test_coordinator_governance.py``, ``test_coordinator_close_all_children.py``, ``test_coordinator_client.py``, ``test_phase6_endpoints.py``). Also touched: - Server-side ``CoordinatorClient`` (``coordinator_client.py``) — the coord agent's HTTP path for ``close_all_children`` updates to the new shape. - Handler docstrings in ``console/server.py`` say ``POST /v1/api/workstreams/...`` not ``/coordinator/...`` so a ``grep`` for a verb's URL lands on the right line. - ``settings_registry.py`` setting descriptions, ``server.py`` cross-process error message, migration 042 docstring — all updated to the unified shape. The handler functions stay named ``coordinator_*`` until the body-convergence follow-on lifts them into ``session_routes`` with kind branching behind ``SessionRouteConfig`` flags. URL surface is the only thing that changes here. The ``test_session_routes`` route-walk now asserts the legacy paths are GONE — previously it asserted both shapes coexisted. A future accidental remount of ``/api/coordinator/`` would fail that test.	2026-04-24 16:56:06 -07:00
Patrick Buckley	58d20f4012	chore(coord): remove spawn-quota subsystem (#403) ... * chore(coord): remove spawn-quota subsystem The quota gate was operator-level safety per its own comments, not a security boundary, and never fired in a week of heavy use. Runaway coordinator spawns are already bounded by max_active slot exhaustion, which surfaces to the coord LLM as a tool error — same operational shape, one fewer moving part. Precedes the Stage 1 SessionManager unification so the coord tool doesn't inherit quota bookkeeping. Upgraded deployments with the three removed settings persisted will log three "Skipping invalid setting" warnings on startup and otherwise degrade cleanly; a follow-up migration to delete the rows would silence that noise. * chore(migrations): drop stale coord spawn-quota settings rows (047) Clears persisted rows for the three ConfigStore keys removed in the previous commit so upgraded deployments don't log "Skipping invalid setting" warnings on every startup. Downgrade is a no-op — the rows were operator-set values, and a rollback to pre-1.5.0 code falls back to the registry defaults for any key not present.	2026-04-23 20:09:12 -07:00
Patrick Buckley	67aaa236e8	feat(coordinator): phase 8 PR B — spawn budget + rate limit + /quota endpoint (#387) ... * feat(coordinator): phase 8 PR B — spawn budget + rate limit + /quota endpoint Adds two complementary controls so a runaway coordinator can't saturate a cluster's max_active without anyone noticing: - **Spawn budget** (hard quota) — cap on concurrently active children. Default 20 per coord. spawn_workstream returns a tool error guiding the model to close idle children; spawn_batch routes overflow rows to `denied[]` with partial-success semantics. - **Spawn rate limit** (soft pacing) — classic token bucket, defaults 5 tokens/minute with burst 10. A rate-limited spawn surfaces a tool error carrying `retry after Ns` so the model paces itself. Zero refill rate is honoured as "disable refill" (bucket still honors the initial burst). Shipped infra: - `turnstone/core/spawn_quota.py` — thread-safe `SpawnBudget` + `TokenBucket`. 15 unit tests. - `turnstone/core/session.py` — coord-only state built from settings at __init__. Shared `_eval_spawn_quota(active)` helper drives both the single-spawn path (wraps the denial reason in `_coord_tool_error`) and the batch path (annotates `spec["_error"]`). `_count_active_children` routes through `coord_client.list_children(include_closed=False)` and fails *open* on lookup error (budget is operator-safety, not security). - `POST/GET /v1/api/coordinator/{ws_id}/quota` — partial-update admin endpoint mirroring the /trust + /restrict shape. Accepts either the nested `spawn_rate` object or flat aliases — supplying both for the same field returns 400 so the admin UI can't half-migrate silently. Overrides are in-memory only (die on session reopen). Audits via `coordinator.quota.updated` with before/after snapshots. - Settings: `coordinator.spawn_budget`, `coordinator.spawn_rate.tokens_per_minute`, `coordinator.spawn_rate.burst` with ranges 1..500 / 0..600 / 1..500. The range bounds are the single source of truth — the endpoint validators and Pydantic schema both import from `settings_registry.SETTINGS` so bumping a cap in one place lights up everywhere. - OpenAPI: `CoordinatorQuotaRequest` / `CoordinatorQuotaResponse` / `CoordinatorSpawnRateState` schemas + endpoint specs. TS SDK regenerated. Tests: +15 unit (SpawnBudget + TokenBucket), +17 endpoint (GET + POST happy paths, range edges, mixed-body rejection, non-object spawn_rate, service-token refusal), +11 session-side (budget blocks single spawn, budget batch partial-success, rate batch partial-success, empty-body reject, mutator live-update, non-coord session has no quota state). Deferred (not this PR): per-skill scoping via migration 047 + `prompt_templates.spawn_budget` column. Count-only storage helper (opportunistic — list_children at budget ≤ 500 is fine behind a human-gated approval flow). * fix(coordinator): address PR #387 copilot review - Budget undercount: _count_active_children used list_children's LIMIT-then-Python-filter path, so a fan-out with many recently-closed children could push live rows past the SQL LIMIT and silently undercount, leaking spawn slots past the budget. Replace with a new CoordinatorClient.count_active_children that uses storage.count_workstreams_by_state (SQL aggregate, no pagination, sums non-terminal states). Tenant-guarded; fails open on storage error (budget is operator-safety, not a security gate). New client tests cover the non-terminal count, the closed/deleted exclusion, the foreign-parent guard, and the fail-open path. - Service-token bypass on /quota: both GET and POST used the default allow_service_bypass=True, so a service token whose user_id matched the coord owner could read or *raise* spawn capacity without the explicit admin.coordinator grant. Flip both to allow_service_bypass=False for consistency with /restrict, /stop_cascade, and /close_all_children. - OpenAPI contract leak: CoordinatorSpawnRateState was used for both the request and response shapes, which let generated SDKs imply clients could POST tokens_available (a read-only bucket reading the handler ignores). Split into CoordinatorSpawnRateInput (request: tokens_per_minute + burst only) and CoordinatorSpawnRateState (response: adds tokens_available). No runtime behaviour change; SDKs regenerate with two distinct types. Drops the _ACTIVE_COUNT_SLACK / _ACTIVE_COUNT_MIN_LIMIT constants in session.py — no longer needed since the new helper takes no limit argument. Updates the 5 session-side quota tests to stub count_active_children instead of list_children.	2026-04-19 10:14:26 -07:00
Patrick Buckley	7d61f9a37c	feat(coordinator): phase 8 PR A — spawn_batch + close_all_children batch tools (#386) ... * feat(coordinator): phase 8 PR A — spawn_batch + close_all_children batch tools Adds two model-facing batch tools so a coordinator can fan out without burning one approval per child: - `spawn_batch` — create up to 10 child workstreams in a single approval. Serialised spawns so sibling ordering (by created_at) stays deterministic. Returns `{results: {idx: {ws_id, name, node_id, status}}, denied: [{idx, reason}]}`. Per-item validation / spawn failures surface in `denied[]`; the batch hard-errors on >10 rather than silent truncation. - `close_all_children` — soft-close every direct child in one approval. Server-side Sem(16) fan-out via `coord_client.close_workstream`; `reason` propagates to every closed child's audit + workstream_config. Response mirrors `stop_cascade`'s cascade idiom: `{closed, failed, skipped}` where `skipped` is upstream-404 / already-gone. Shipped infra: - New console endpoint `POST /v1/api/coordinator/{ws_id}/close_all_children` (gated `admin.coordinator`, `allow_service_bypass=False`, 512-char reason cap, `coordinator.closed_all_children` audit). - Shared `_fanout_on_children` helper — both `stop_cascade` and `close_all_children` now delegate to it (one place to own the snapshot → semaphore-gather → bucket-split skeleton). - `CoordinatorClient.close_all_children(reason)` plus a `_post_url` seam that `_post` now reuses (no more duplicated transport-error handling). - `_emit_batch_event` — best-effort SSE emitter modelled on `_emit_wait_event`. Emits `batch_started` / `batch_ended` pairs keyed by call_id. Throttled `batch_progress` deferred to a follow-up. - OpenAPI request + response schemas, endpoint spec entry, TS SDK regenerated. - Persona doc (`tools_coordinator.md`) covers the two new patterns. Bulk-endpoint shape policy (codified in PR C later): split by semantic category — `{results, denied, truncated}` for bulk-read / bulk-create-with-payload (cluster/ws/live, spawn_batch), `{<bucket>, failed, skipped}` for cascade-mutation (stop_cascade, close_all_children). No retrofit needed on stop_cascade. Tests: new `test_coordinator_close_all_children.py` (8 endpoint tests), expanded `test_coordinator_tools.py` (session-side prepare/exec, coord_client=None guards, batch SSE events), expanded `test_coordinator_client.py` (route map, client method, transport errors), tool-count assertions updated. Deferred (not this PR): per-item selective-deny approval UI, throttled batch_progress SSE, coordinator-skills doc + bulk-endpoints doc (PR C), spawn budget / rate limit (PR B). * fix(coordinator): address PR #386 copilot review - coordinator_client.close_all_children: pass the unformatted path template as log_path so telemetry aggregates don't fragment per session (ws_id still lives in the real URL). - session.py: drop dead spawned_ids accumulator in _exec_spawn_batch — leftover from an eager-register path that got removed earlier. - close_all_children tool JSON: document the 512-char server-side cap on reason and that reason is echoed back in the response payload. Added maxLength:512 on the schema property so the LLM sees the constraint. - CoordinatorCloseAllChildrenRequest: add Field(max_length=512) so the OpenAPI schema reflects the runtime 400-on-overflow constraint.	2026-04-18 23:50:25 -07:00
Patrick Buckley	9826ea15c5	feat(coordinator): phase 7 — governance + skill metadata + cross-cutt… (#383) ... * feat(coordinator): phase 7 — governance + skill metadata + cross-cutting invariants Combines three stacked sub-PRs into a single coordinator phase-7 shipment against the phase-7 plan doc. The sub-PR structure (0 / A / B) preserved on individual branches for reviewer drill-down; this branch is the one reviewers should merge. ## Sub-PR 0 — service-auth boundary invariants Shared helpers and contracts that lock the console ↔ node service-auth boundary so later authz surfaces use them by construction. - ``_effective_user_filter(request)`` in both ``turnstone.console.server`` and ``turnstone.server`` with a shared ``DENY_EMPTY_SUB`` sentinel on ``turnstone.core.auth``. Three-way return — admin/service bypass, scoped caller uid, or fail-closed sentinel on blank sub. Four callsite migrations (``_coordinator_rows``, ``coordinator_children``, ``coordinator_metrics``, ``cluster_ws_live_bulk``). - ``StorageBackend`` class docstring codifies the tenancy contract (every list/count/aggregate method must accept ``user_id: str | None = None`` and push ``WHERE user_id = :user_id`` into SQL) and the ``_mapping`` row-access contract. New ``turnstone.testing.row_contract`` ships ``assert_row_like()``. - ``_verify_collector_service_scope`` probes an upstream node at boot with ``expected_node_id=_scope-probe_``; a 409 proves the scope gate was passed, a 403/401 sets ``collector_scope_error`` and causes ``cluster_snapshot`` / ``cluster_events_sse`` to return 503 with a remediation hint. Probe URL allowlist rejects non-http(s) schemes and 169.254.0.0/16 hosts. - 4xx log-level floor on ``_NodeDashboardCache.get``, ``_fetch_live_block``, and ``_proxy_sse`` — dotted-hierarchy prefixes with bounded body previews. ``_bounded_body_preview`` and ``_bounded_stream_preview`` strip control chars. ## Sub-PR A — coordinator governance core Mid-session governance surface for coordinator workstreams. - **Trusted-session mode.** New ``coordinator.trust.send`` permission (migration 042). ``ChatSession.set_trust_send`` / ``revoke_tools`` methods with a ``_governance_lock``. ``POST /v1/api/coordinator/{ws_id}/trust {send: bool}`` double-gated on ``admin.coordinator`` AND ``coordinator.trust.send`` with ``allow_service_bypass=False`` so service tokens can't escalate. ``_prepare_send_to_workstream`` auto-approves sends whose target is in the coordinator's own subtree; foreign ws_ids still require approval. ``_is_own_subtree`` checks both ``parent_ws_id`` AND ``user_id`` to defend against cross-tenant row corruption. - **Audit-layer credential redaction.** ``record_audit`` walks ``detail`` (dicts, lists, tuples, sets, frozensets; keys too) and routes every string through ``redact_credentials`` + a C0 control-char scrub. New kw-only ``raw_detail=True`` opt-out. ``_has_any_string`` fast-path. Audit action registry extended with the four new governance sub-prefixes. - **Mid-session revocation + cascading stop.** ``POST /v1/api/coordinator/{ws_id}/restrict {revoke: [...]}`` caps 256 entries / 128 chars; ``_prepare_tool`` short-circuits with a tool-error. ``POST /v1/api/coordinator/{ws_id}/stop_cascade`` cancels the coord's in-flight generation then dispatches ``cancel_workstream`` for every direct child in parallel via ``asyncio.gather`` bounded by ``Semaphore(16)``. Per-child outcomes split into ``cancelled`` / ``failed`` / ``skipped`` (404 = already-gone rather than dispatch-broken). Both endpoints apply ``allow_service_bypass=False`` on the admin gate. - **Shared plumbing.** ``_resolve_coord_session`` helper collapses the handler prelude three endpoints shared. ``_emit_coord_audit`` wraps ``record_audit`` in a dedicated ``ThreadPoolExecutor`` (``app.state.audit_executor``) so audit bursts don't starve cancel dispatches. ``_require_json_object`` guards body parsing so non- object JSON returns 400 instead of 500. ## Sub-PR B — skill metadata governance - **Description validator (migration 043).** ``prompt_templates`` rows now require a non-empty ``description``. Existing empty rows get backfilled with a ``"Skill: <name>"`` placeholder on upgrade. The installer (``admin_skill_discover``) and MCP prompt sync both synthesise a placeholder when the upstream description is blank so non-admin write paths satisfy the invariant. - **Skill kind classifier (migration 044).** New ``prompt_templates.kind`` column (``interactive`` / ``coordinator`` / ``any``; defaults to ``any``). New ``turnstone.core.skill_kind.SkillKind`` StrEnum is the single source of truth; Pydantic schemas type ``kind`` as ``SkillKind`` (OpenAPI advertises the enum) and the handler validator catches the ValueError. ``list_skills_filtered`` gains a ``kinds: list[str] | None = None`` SQL filter. ``CoordinatorClient.list_skills`` defaults to ``kinds=["coordinator", "any"]`` so interactive-only skills are hidden from the orchestrator. - **``scan_status`` → ``risk_level`` rename (migration 045).** Lossless column rename to align with ``IntentVerdict.risk_level`` terminology. Swept storage (both backends + schema + protocol), handlers, API schemas, tool JSON, generated OpenAPI specs, TypeScript SDK types, frontend (``governance.js``), tests, and English prose in ``docs/judge.md`` + ``docs/tools.md``. The user-facing on-load warning now reads ``has risk level: {risk_tier}``. Tool JSON's ``risk_level`` enum corrected to the scanner's actual taxonomy (``safe / low / medium / high / critical``; was the never-shipped ``clean / flagged / unscanned / pending``). Historical migration 021 left untouched. ## Migrations 042 (``coordinator.trust.send`` perm — PR A) 043 (description backfill — PR B) 044 (``kind`` column add — PR B) 045 (``scan_status`` → ``risk_level`` rename — PR B) All four use position-anchored permission strings / host-side parse-filter-rejoin on downgrade where SQL ``REPLACE`` could corrupt prefix-overlapping values. ## Verification - ``ruff check turnstone tests`` clean. - ``mypy turnstone`` clean on 165 source files. - ``pytest -m "not live"``: 4431 passed (+85 over the phase-6 baseline). Includes +32 tests in ``tests/test_service_auth_boundary.py`` and +38 in ``tests/test_coordinator_governance.py``; shared fixtures extracted to ``tests/_coord_test_helpers.py``. - Generated OpenAPI JSON (``sdk/typescript/openapi-{console,server}.json``) regenerated via ``sdk/typescript/scripts/generate-types.py``; zero ``scan_status`` occurrences remaining outside the historical migration 021 and the rename migration 045. ## Security reviews Both reviews flagged by the phase-7 plan (items 1 + 5, plus 0a's refuse-to-serve gate) ran through the multi-stage ``/review`` pipeline twice per sub-PR; all confirmed findings landed in-branch. * fixup(phase-7): CI lint + PR #383 review fixups Addresses the lint CI failure (ruff format) plus 12 findings from the two automated PR reviewers. Copilot: - ``_sqlite.list_installed_skill_urls`` / ``_postgresql.list_installed_skill_urls`` used positional row indexing (``r[0]``/``r[1]``/``r[2]``) while this same PR's ``StorageBackend`` class docstring forbids it. Switched both to ``r._mapping["..."]`` access. - ``list_skills.json`` previously advertised ``risk_level=""`` as a filter for unscanned skills, but the implementation treats empty strings as "no filter". Clarified the tool description to say omit the filter entirely to include unscanned rows, and added an explicit ``enum`` on the parameter restricting it to the scanner tiers. ``_prepare_list_skills`` keeps the ``strip() or None`` normalisation — unscanned filtering now has an unambiguous contract. - ``test_storage_skills_filtered.test_risk_level_filter`` used the legacy ``clean`` / ``flagged`` values from the pre-rename column. Rewritten with the scanner's actual taxonomy (``safe`` / ``high``). github-code-quality (CodeQL): - ``test_deny_sentinel_is_singleton`` previously asserted ``cs.DENY_EMPTY_SUB is cs.DENY_EMPTY_SUB`` — an identical-expression comparison. Rewritten as two separate ``from ... import ... as`` aliases (``FIRST_READ`` / ``SECOND_READ``) so the identity check is between distinct bindings. - ``test_restrict_empty_revoke_is_noop_but_audits`` unpacked ``state`` without using it. Renamed to ``_state``. - Mixed import styles in ``test_service_auth_boundary.py`` — the file previously used both ``import turnstone.console.server as cs`` and ``from turnstone.console.server import ...`` for the same module (same story for ``turnstone.core.auth`` and ``turnstone.server``). Consolidated to the ``from X import Y`` style used elsewhere in the file; the ``_fetch_live_block`` test now patches via pytest's ``monkeypatch`` fixture instead of a manual rebind through a module alias. CI: - ``ruff format`` reformatted one line in ``tests/test_coordinator_endpoints.py``. Verification: ruff check + mypy clean (166 files); 4459 non-live pytest pass. * fix(tests): swap asyncio marker for anyio in service-auth boundary tests PR #383 CI caught that the 13 ``@pytest.mark.asyncio`` decorators I added in ``test_service_auth_boundary.py`` are an off-convention choice — the rest of the repo uses ``@pytest.mark.anyio`` (148 sites vs my 13). The CI environment pulls in ``anyio`` but not ``pytest-asyncio``, so every async test in this one file was failing with "async def functions are not natively supported". It passed locally by accident — my dev venv happens to have pytest-asyncio installed ambiently. Swapped all 13 marker sites to ``@pytest.mark.anyio``. No functional change; the tests run under the same default asyncio backend anyio provides. Verification: ruff + mypy clean (166 files); 4459 non-live pytest pass.	2026-04-18 10:20:19 -07:00
Patrick Buckley	bd6670d748	feat(coordinator): phase 6 — polish, observability, active-coords via SSE, frontend cleanup (#381) ... * feat(coordinator): phase 6 — polish, observability, active-coords via SSE, frontend cleanup Squashed from two working commits: 1. phase-6 backend polish + active-coords SSE 2. phase-6 frontend cleanup (legacy chat-view classes + designer nits) Both tier-A/B observability items and tier-C frontend consolidation ship together — the shared-vocabulary migration touches surfaces the backend polish already had its hands in, so one combined commit keeps the diff reviewable as a coherent phase. Observability ------------- - **Coordinator-side wait dashboard** — `_exec_wait_for_workstream` emits `wait_started` / `wait_progress` / `wait_ended` SSE events via a new `progress_callback` hook on `CoordinatorClient.wait_for_workstream`; coordinator.js renders a "⧗ waiting · N ws · Ts" header indicator keyed by call_id so overlapping waits coexist. Progress throttled to emit only on snapshot-diff or 5s heartbeat; full results dict attached only on transitions so a 600s wait doesn't flood SSE listener queues. Indicator only attaches when a proper header host exists (no floating document.body fallback) and is cleared on SSE reconnect so a dropped `wait_ended` can't pin the badge. - **`cancel_workstream` forensics** — `server.cancel_generation` captures `ui._pending_approval` tool names + `session._queued_messages` count / preview before invoking `session.cancel`, returning the snapshot as `dropped`; routing proxy passes it through to the tool result. Preview runs through `redact_credentials` before the 120-char truncate so pasted secrets / connection strings don't land verbatim in the coordinator's conversation history. - **Per-coordinator metrics** — `GET /v1/api/coordinator/{ws_id}/metrics` returns `spawns_total` / `spawns_last_hour` / `child_state_counts` / `judge_fallback_rate` (substring match on verdict.tier) plus zero placeholders for wait_* pending dedicated instrumentation. Derived from new `storage.count_workstreams_by_state` + `count_workstreams_since` aggregate helpers — no 10k-row hydrated-select to compute a histogram. Ownership 404-mask matches `coordinator_detail`. - **Coordinator skill in inspect** — `CoordinatorManager.create` resolves `skill` → `template_id` / `applied_version` via `get_skill_by_name` + new `storage.count_skill_versions` (replacing the SELECT-all-for-COUNT anti-pattern) and persists them on the workstreams row. `/new` handler dispatches via `asyncio.to_thread` so blocking storage calls don't stall the event loop. - **`wait_for_workstream(since=…)`** — optional prior-snapshot hint; when supplied, the wait loop diffs each polled ws_id that IS in `since_map` and exits on any change, independent of mode. ws_ids absent from `since_map` fall through to the normal mode condition — a disjoint since dict no longer silently exits the wait on tick one. - **`task_list.child_ws_id` referential cleanup** — `CoordinatorClient.cleanup_dead_task_child_refs(ws_id)` holds the same per-ws `_task_lock` as `task_list_*` so a close racing a task_list write can't lose the mutation. `CoordinatorManager.close` delegates. Final save-failure logs at `warning` instead of `debug`. Home view live-updates ---------------------- - **Active-coordinators via SSE** instead of a 5s poll — `ClusterCollector.ensure_console_pseudo_node` + `emit_console_ws_created / _closed / _state / _rename` plumbing; `CoordinatorManager.create / open / close / eviction` + `ConsoleCoordinatorUI.on_state_change / on_rename` all fan out through the collector. The pseudo-node is exempt from the discovery-loop eviction; rehydrate-path eviction now also emits `console_ws_closed` for the evicted row so other tabs drop it live. `app.js` reads coordinators from `clusterState.nodes["console"]`; poller + back-compat shims deleted (9 call sites). Overview / nodes list skip the pseudo-node so it doesn't inflate cluster totals. Tenant-filtering preserved by excluding the pseudo-node from `collector.get_workstreams` so `/v1/api/cluster/workstreams` still uses the existing tenant-filtered `_coordinator_rows` path. `CoordinatorManager.NODE_ID` bound from `ClusterCollector.CONSOLE_PSEUDO_NODE_ID` so the two literals can't drift. Frontend perf ------------- - **Bulk cluster-ws live endpoint** — `GET /v1/api/cluster/ws/live?ids=` returns `{results, denied, truncated}` (cap 50); coordinator.js batches visible-row live-badge fetches into one bulk request per ~250ms window (replaces per-row /detail polling). Ownership check routes through the empty-string-safe pattern (non-admin with empty `caller_uid` doesn't match empty-owner rows). Legacy chat-view class cleanup ------------------------------ - Drop the `.msg` / `.msg-user` / `.msg-assistant` / `.msg-tool` / `.msg-error` / `.msg-info` / `.approval-block` / `.approval-tool` / `.approval-btn` / `.approval-badge` / `.approval-prompt` / `.approval-feedback-input` / `.approval-actions` / `.pane-input` / `.pane-input-area` / `.pane-input-row` / `.pane-attach` / `.pane-attach-chip` / `.coord-msg` / `.coord-body` / `role-*` / `btn-approve` / `btn-deny` / `btn-always` / `verdict-glow-*` legacy dual-class names left over from the phase-4 migration. Every JS className concatenation + querySelector + CSS selector now uses the `ts-*` vocabulary from `shared_static/chat.css` (and `ui/static/style.css` where the interactive-page extensions live). Feature-specific class names that don't map to `ts-*` stay — `msg-queued` / `msg-editing` / `msg-actions` / `msg-edit-*` / `msg-user-attach*` / `msg-user-text` / `queued-badge` / `queued-dismiss` / `tool-name` / `tool-cmd` / `tool-diff` / `tool-header` / `tool-preview`. Designer nits ------------- - **`.ui-btn--icon:focus-visible`** — new rule matching `.ui-btn`'s `outline: 2px solid var(--accent); outline-offset: 1px` so the compact icon variant gets the accent ring instead of the browser-default outline. - **Dropped speculative 701-880px composer wrap rule** — the flex math at ≥701px fits comfortably in every desktop viewport, so the mid-zone break rule was forcing a 2-line layout where the browser wouldn't have wrapped naturally. The existing `<700px` full-stack covers the original wrap observation. - **`.verdict-badge` border-top** + **`.ch-row.highlight` prefers-reduced-motion** — confirmed already in main; no additional code change needed for phase 6. Follow-up designer-review findings ---------------------------------- - Dropped `border-top` from `.ts-approval-badge` + `.ts-approval-body` (chat.css's max-content width / flex-gap made them read as truncated / floating lines). - `var(--muted)` → `var(--fg-dim)` on denied tool names (undefined token was silently failing). - Dropped 3 dead `.ts-approval-badge.badge-*` rules + duplicated `.ts-approval-btn:focus-visible` + dead `.reasoning` CSS rule + `contains("reasoning")` JS guard. - Dropped `tool-row` / `approval-header` / `btn-row` / `label` dead legacy classes in the coordinator. - Added `:focus-visible` to `.ch-row a.ws-link` + `.task-row` so keyboard users get the accent ring on sidebar rows. Cleanups -------- - `_WAIT_REAL_TERMINAL_STATES` / `_WAIT_TERMINAL_STATES` / `_WAIT_MAX_*` / `_WAIT_POLL_INTERVAL` hoisted to module level on `coordinator_client` so `session.py` no longer reads a class internal; ClassVar aliases kept for back-compat. - `ConsoleCoordinatorUI` state/rename observers typed as `Callable[[str], None] | None` instead of `Any`. - SSE error renderer verified end-to-end (coordinator.js already handles `case "error"` → `appendText`; no code change). Tests ----- - 26 new test cases: 11 for `_diff_since` + `cleanup_dead_task_child_refs`, 15 for `cluster_ws_live_bulk` + `coordinator_metrics`. Full suite 4345 passing (4319 base + 26 phase-6). Gate: ruff + mypy + pytest -m "not live" (4345 passed) all clean. * fix(coordinator): address PR #381 review feedback Copilot comments: - Cross-tenant aggregate leak in coordinator_metrics — the new count_workstreams_by_state / count_workstreams_since aggregates took parent_ws_id but not user_id, so a non-admin caller could observe drifted / forged child rows that share parent_ws_id with their coord but whose user_id drifted to another tenant. The 404-mask on coord ownership (_resolve_coordinator_or_404) is the primary defense; this is defense-in-depth inside the aggregate queries. Pass filter_user_id (None for admin, caller_uid for non-admin) — matches coordinator_children's tenant-push-into-SQL pattern. - wait_for_workstream(since=…) docstring + tool schema were stale — claimed "A missing entry counts as changed on first observation" but the implementation ignores ws_ids absent from since_map to prevent a disjoint since dict from silently exiting on tick one. Rewrote both doc sites to match the actual semantics: only ws_ids present in `since` participate in the diff-exit check; others fall back to the normal mode-based completion condition. - WAIT_TERMINAL_STATES comment drift — the comment claimed it was "used by the resolved-count summary" but the summary counts only WAIT_REAL_TERMINAL_STATES (denied is a rejection, not a resolution). Rewrote the comment to describe the real usage: mode='any' pure-denied short-circuit + mode='all' settle check. github-code-quality (CodeQL): - coordinator.js — dropped the dead typeof _renderWaitIndicator guard + the typeof activeWaits guard around the reconnect clear. Both symbols are defined in the same IIFE; the onopen handler fires strictly AFTER IIFE execution finishes, so the guards always evaluated to true. Removing the dead branching also removes a CodeQL nit. - Protocol-method `...` statements — the bot flagged the three new methods (count_workstreams_by_state / count_workstreams_since / count_skill_versions) with "statement has no effect". Left as `...` to match the file's universal convention (216 `...` bodies / 0 `pass` bodies pre-change); swapping just the new methods to `pass` would introduce inconsistency with every other Protocol method. Resolved as non-actionable. Test: new test_metrics_tenant_filter_excludes_forged_cross_tenant_child covering the aggregate-query tenant filter with both a legitimate alice child and a forged bob child sharing parent_ws_id. Non-admin alice sees 1; admin sees 2. Gate: ruff + mypy + pytest -m "not live" (4346 passed) all clean.	2026-04-18 03:49:56 -07:00
Patrick Buckley	553d73109b	feat(coordinator): phase 5 — harness-test polish + wait_for_workstrea… (#378) ... * feat(coordinator): phase 5 — harness-test polish + wait_for_workstream + judge fix Closes the bug list surfaced by the 2026-04-17 coordinator harness test plus the post-phase-4 wait_for_workstream ask, and folds in three adjacent cleanups that landed in the same window. Tightens defense-in- depth on the model-invoked mutating ops, fixes the LLM judge silent no-op, kills the inspect-poll token burn, and rounds out a handful of observability / docstring / spec gaps. The session-factory pre-resolve at console/session_factory.py and server.py was rewriting `judge.model` from an alias (e.g. `judge-mini`) to the resolved underlying id (e.g. `gpt-5-mini`). IntentJudge then checked `model_registry.has_alias(config.model)`, found nothing, and fell back to the SESSION's provider/client with that bare model id — silent `llm_fallback / "did not return a verdict"` whenever the coordinator and judge alias resolved to different providers. Pass the alias through unchanged; IntentJudge's existing alias- resolution path picks up the matching client + provider. Validate the alias exists so an obvious typo still surfaces, but don't replace the model field. Regression: `test_alias_uses_registry_provider_not_session_provider` constructs an alias whose provider differs from the session's and asserts the judge picks up the alias's provider/client/model; `test_coordinator_tool_call_returns_llm_verdict_not_fallback` asserts the verdict tier is `llm` (not `llm_fallback`) on the happy path. New `cancel_workstream` tool (approval required, primary_key=ws_id) — cancels in-flight generation, unblocks any pending approval / plan, moves the child to idle, leaves the row in storage so a fresh send_to_workstream lands cleanly. Re-uses the existing `/v1/api/route/cancel` route + `route.cancel` audit namespace; no new server endpoint. `CoordinatorClient.cancel/close_workstream/delete/send` now enforce a tenant guard inline (`_is_own_subtree`) — only the coordinator itself or one of its own children is targetable. Foreign ids return the same 404-shape inspect/wait_for_workstream use, so the model can't distinguish foreign from missing (no existence oracle). Defense-in-depth — the upstream node enforcement is the perimeter, this is the second line. `list_workstreams` advertised `state="deleted"` and an `include_closed=true` that surfaced deleted rows. Hard-deletes cascade the workstream + conversation rows out of storage, so deleted is unreachable in normal operation. Doc-only fix; the synthetic-test path that registers `state="deleted"` rows still works (terminal-state filter still excludes them via `_terminal_states = {"closed", "deleted"}` in list_children). Documented that the 120s service-registry heartbeat window means a node returned by list_nodes can drop out before a follow-up `spawn_workstream(target_node=…)` lands — the spawn fails with "No available node for routing" rather than falling back. Two-line clarification on each tool. No code change (a code fallback is a bigger discussion deferred to 1.6). `close_workstream` accepts `reason`; the upstream server handler now persists it to `workstream_config.close_reason` (capped at 512 BYTES, sliced on UTF-8 not code points so a CJK / emoji-heavy payload can't 4× the documented budget). `CoordinatorClient.inspect()` reads it and surfaces as `close_reason` in the result dict — only for terminal-state children (closed/error/deleted) so the live-child hot path doesn't pay a per-inspect DB round-trip. Tests: server-side persistence covers success / no-reason / length-cap / non-string / storage-failure / multi-byte-utf8 paths; client-side surface covers terminal vs. live workstreams. For idle children whose node-dashboard live counter is 0 (the live block only surfaces in-flight token counters), fall back to `SUM(prompt_tokens + completion_tokens)` from `usage_events` so the inspect output reflects cumulative spend. New `storage.sum_workstream_tokens(ws_id) -> int` on the protocol + both backends. The fallback is folded INTO `_fetch_cluster_live` so the merged live block (with persisted total applied) is what gets cached — back-to-back inspects of an idle child amortize through the existing 2s LRU cache instead of each firing a fresh aggregation. `CoordinatorClient.list_skills()` now projects `allowed_tools` per skill — capped at 20 with a `+N more` sentinel so a skill that whitelists a wide MCP surface doesn't bloat the per-row payload. Reads the existing `prompt_templates.allowed_tools` column; no storage change. Coordinators no longer have to guess what tools a skill brings. `route_create` now sets `routing_strategy: "hash_ring" | "target_node" | "resume"` on the spawn response so the coordinator's spawn response (and the `spawn_workstream` tool output) carries why a given node was chosen. 3 lines + 3 covering tests in test_console_routing_proxy.py. New coordinator tool `wait_for_workstream(ws_ids, timeout=60, mode='any'|'all')` that absorbs the wait into a single tool call — the model sees one call + one result regardless of how long the children take. Kills the busy-poll inspect loop that burned 20+ turns on a 3-child fan-out. Storage-poll loop with batched primitives — `get_workstreams_batch` + `sum_workstream_tokens_batch` issue exactly two storage calls per tick regardless of N. At the cap (32 ws_ids / 600s / 0.5s tick) that's ~2400 round-trips for a full wait, down from ~38k under the naive per-id shape. Validation single-source-of-truth: the client owns mode whitelist, ws_ids dedup + cap, timeout coerce + clamp. The session preparer is a thin pass-through that builds the header + dispatches; bad input surfaces at exec time as a tool error via `result.get("error")`. Tenant-isolation collapse: missing-row and cross-tenant cases both return `state="denied"` so wait can't be used as an existence oracle (matches the 404-mask contract `inspect` uses). Prompt-side: tools_coordinator.md adds a `wait_for_workstream` pattern + an explicit "PREFER wait_for_workstream OVER a loop of inspect_workstream" line in the workflow-shape section. Replaces the quote-bracketed substring LIKE/ILIKE pattern with proper JSON-array containment. The previous shape effectively did `LOWER(tags) LIKE '%"<lower-tag>"%'`, which broke for tag values containing `"` (the JSON encoder escapes it to `\"` and the literal- substring search misses), `\` (encoded as `\\`), or non-ASCII characters that the encoder rendered as `\uXXXX`. Also exposed a small spoofing surface — `tags=["foo\","bar"]` would have matched a query for `bar`. Real-world tag values are alphanumeric+dash today so it hadn't fired in production, but the fix is small. - SQLite: `EXISTS (SELECT 1 FROM json_each(prompt_templates.tags) WHERE lower(value) = lower(:tag))` (JSON1 extension; SQLite 3.38+). - PostgreSQL: `EXISTS (SELECT 1 FROM jsonb_array_elements_text( prompt_templates.tags::jsonb) AS jat(elem) WHERE lower(jat.elem) = lower(:tag))`. Three new tests prove the substring pattern was broken for quoted / backslash / unicode tag values; the existing case-fold + wildcard tests continue to pin the contract. Phase 1 added the coordinator workstream API; phase 2 added only `/open` to the OpenAPI catalog and missed every other coordinator endpoint plus phase 3's `/children`, `/tasks`, and the `/cluster/ws/{ws_id}/detail` aggregator. SDK consumers + operators browsing `/docs` couldn't discover the surface. Doc-only addition: 12 endpoints + 9 new Pydantic models, all under the `Coordinator` OpenAPI tag so /docs groups them together. Sidebar re-fetches `GET /tasks` on every `task_list` `tool_result` SSE event. A model that runs `add → list` (or any back-to-back mutation pair) double-fetches the same envelope. Coalesced into one fetch per 150ms window via a new `loadTasksDebounced` wrapper; direct UI actions (refresh button, page load) keep calling `loadTasks` directly so user clicks aren't delayed. - `ruff check turnstone tests` — clean - `mypy turnstone` — clean (157 source files) - `pytest -m "not live"` — 4284 passed, 3 deselected (was 4226 on main; +58 new tests across coordinator client, tools, judge, storage, console routing proxy, server close-handler, storage_skills_filtered, OpenAPI catalog, server close-reason persistence) - New tools added: 2 (cancel_workstream, wait_for_workstream) — TOOLS count 28 → 30; coordinator subset 9 → 11; auto_approve adds wait_for_workstream; primary_key adds cancel_workstream - New OpenAPI endpoints: 12 (every phase-1/2/3 coordinator route + the cluster-inspect aggregator) - New storage protocol methods: 3 (sum_workstream_tokens, sum_workstream_tokens_batch, get_workstreams_batch) All phase 1 / 2 / 3 / 4 invariants preserved: COORDINATOR_TOOLS / INTERACTIVE_TOOLS disjoint; coordinator sessions have no MCP surface; list-style tools return {items, truncated}; route-proxy emits route.<action> audit on 2xx; 404-mask on ownership failures; tenant filters pushed into SQL; per-coordinator JWT carries scope context. * fix(coordinator): address Copilot review on PR #378 Three valid Copilot findings on the wait_for_workstream surface: 1. ``wait_for_workstream.json`` description claimed the tool returns a top-level mapping ``ws_id -> {state, tokens, updated}`` plus elapsed/complete/mode at the same level, but the actual shape is ``{results: {ws_id: {...}}, elapsed, complete, mode}``. Description now matches the implementation. Also adds ``deleted`` to the advertised terminal-state list (it's in ``_WAIT_REAL_TERMINAL_STATES``; the doc and runtime now agree). 2. ``CoordinatorClient.wait_for_workstream`` docstring listed ``idle / error / closed`` as the real terminal set but the constant includes ``deleted``. Same fix — list ``deleted`` with a parenthetical noting it's unreachable in normal operation (hard-delete cascades the row). 3. Storage protocol docstring math: ``sum_workstream_tokens_batch`` claimed "from ~38k to ~1200" round-trips per wait at the cap, but ``wait_for_workstream`` issues TWO storage calls per tick (``get_workstreams_batch`` + this one), so 1200 ticks × 2 = ~2400. Updated to "~2400" with the math spelled out. Also a clean rebase onto today's main (PR #377 — the rebalancer node_id snapshot doc — landed since phase 5's last push). Single conflict in ``inspect_workstream.json`` resolved by keeping both notes (rebalancer node_id binding semantics + the new ``close_reason`` surface from phase 5); ``spawn_workstream.json`` auto-merged. The github-code-quality bot also flagged three items on ``_protocol.py`` asking to replace ``...`` with ``pass`` in Protocol method bodies. Refuted: ``...`` is the canonical PEP 544 idiom for Protocol method bodies and the rest of the file uses it consistently. The bot's lint rule misfires for ``Protocol`` classes. Verification: - ``ruff check turnstone tests`` clean - ``mypy turnstone`` clean (158 source files) - ``pytest -m "not live"`` — 4308 passed, 3 deselected (no test count change; pure doc/comment edits)	2026-04-17 23:56:10 -07:00
Patrick Buckley	37ed6bbf5b	feat(core): WorkstreamKind enum + list_workstreams user_id filter (#374) ... Foundation PR for the multi-stage-review follow-up. Introduces a single source of truth for workstream kind values and pushes tenant scoping into the storage protocol so list callers can't forget to filter client-side. - WorkstreamKind(StrEnum) replaces bare "interactive" / "coordinator" literals across 17 production modules. Strict mypy narrows every internal call site; raw strings still work at wide boundaries (HTTP body, DB row) via WorkstreamKind(raw) parse at the edge. - StorageBackend.list_workstreams(..., user_id=None) adds a SQL-level WHERE user_id = :user_id gate on both sqlite and postgres impls. Memory wrapper forwards the new filters. - register_workstream now validates kind at the storage edge so SDK / restore / internal callers can't silently corrupt the NOT NULL column with empty / mis-cased / unknown values. - WebUI.__init__ normalizes empty-string parent_ws_id to None, matching the storage-edge and WorkstreamManager invariants. - POST /v1/api/workstreams/new parses body["kind"] through the enum and returns 400 on unknown kinds instead of silent coercion. Absorbs bug-1, bug-2, bug-4/q-6, q-1, q-8, and partial q-2 (wrapper signature forwards the new filters; full deletion of the unused wrapper stays in the cleanup PR).	2026-04-17 22:10:57 -07:00
Patrick Buckley	c397668d21	feat(coordinator): tree-view UI, cluster-wide live inspect, dashboard… (#370) ... * feat(coordinator): tree-view UI, cluster-wide live inspect, dashboard grouping — phase 3 Closes out the 1.5 coordinator UX surface: a right-sidebar tree view at /coordinator/{ws_id} showing spawned children + task list, a new cluster-wide live inspect endpoint that powers the tree's live badges, and 2-level dashboard tree grouping that nests spawned children under their coordinator parent. ## Cluster-wide live `inspect_workstream` New `GET /v1/api/cluster/ws/{ws_id}/detail` on the console, gated by a new `admin.cluster.inspect` permission (unassigned to any builtin role; operators opt in). Aggregates `storage.get_workstream` with a short-timeout (2s) HTTP fetch against the owning node's `/v1/api/dashboard`. Coordinator-hosted workstreams get their `live` block from the in-process `CoordinatorManager` instead of a proxy hop. Response shape `{persisted, live, messages}` — `live: null` on node unreachability / 5xx / missing-entry with status 200 so the UI can degrade gracefully without an error state. Correlation-id masks unexpected exceptions. 404-masks cross-tenant reads (non-admin callers see only their own workstreams). `CoordinatorClient.inspect()` best-effort merges the `live` block onto its storage snapshot so the model-facing `inspect_workstream` tool gains a `live` key without any schema change. Model-facing tool schema stays identical. ## Tree-view UI New right sidebar at `/coordinator/{ws_id}` with a 2-level children tree + the phase-2 task list. Backend: - New `GET /v1/api/coordinator/{ws_id}/children` returns `{items, truncated}` — identical row shape to the `list_children` tool — filtered via `storage.list_workstreams(parent_ws_id=..., kind=None)`. - New `GET /v1/api/coordinator/{ws_id}/tasks` returns the `{version, tasks}` envelope via the shared module-level `load_task_envelope` decoder (extracted from `CoordinatorClient` so both the tool path and the UI read share corruption semantics). Corrupt envelopes return an empty list for UI resilience — the `task_list` tool remains the authoritative write + error path. - `CoordinatorManager` subscribes to the `ClusterCollector`'s listener channel from the console lifespan and dispatches filtered `child_ws_created / child_ws_state / child_ws_closed / child_ws_rename` events onto each coordinator's SSE stream. Filter authoritative on the server via a per-coordinator child-ws_id registry populated lazily on `open()` from storage and incrementally on `ws_created` events; cleared on `close()` / eviction. One SSE connection per client, no client-side filtering. Frontend: - DOM-method-only child-row rendering (no innerHTML of user content). - State glyph vocabulary (● running / ◐ thinking / ⚠ attention / ✗ error / ○ idle) plus text labels — WCAG 1.4.1 carries info in both glyph and label. - Live badges (tokens + pending-approval pip) fetched via `/cluster/ws/{ws_id}/detail` with a 5s TTL cache and 250ms debounce per child. One request per state change, not per second. - SSE child events update in place; renderChildren() re-sorts. - Mobile (<700px) sidebar collapses to an accordion above the chat with a toggle button flipping aria-expanded; a `.highlight` flash marks task→child scroll targets; `prefers-reduced-motion` respected. - Deep-link child rows to `/node/{node_id}/?ws_id=<child>` via `<a target="_blank" rel="noopener">` with encodeURIComponent on regex-validated ids. ## Dashboard tree grouping Cluster dashboard rows now group by `parent_ws_id`. Coordinator rows (`kind == "coordinator"` or children present) get an expand/collapse caret (button with `aria-expanded`); collapsed shows "(N children)". Expanded renders children indented as sibling rows with a left-border gutter. Orphaned children (parent missing or closed) render at top level with a muted "orphan" badge. Expansion state persisted in `localStorage` keyed per coordinator ws_id so operator preference survives reloads. Coordinator rows deep-link to `/coordinator/{id}`; node-backed workstreams keep their existing proxy deep-link. Per-node `ws_created / ws_state / ws_activity` SSE event payloads gained `parent_ws_id` + `kind` so the collector can propagate them through its fan-out to browser clients without a second lookup; `_build_node_snapshot` and `/v1/api/dashboard` rows include the same. Coordinators (which don't live on cluster nodes) merge into `/cluster/workstreams` via a new `_coordinator_rows` helper that threads them through the collector's `get_workstreams(extra_rows=...)` parameter — extras share the filter / sort / paginate pipeline with node-backed rows. ## Tests - `tests/test_coordinator_endpoints.py` — 19 new cases covering children (empty / populated / ownership 404 / admin bypass / invalid ws_id / truncation), tasks (empty / round-trip / corrupt / ownership), and cluster-inspect (auth gates / 400 / 404 / ownership / coordinator self-path / unloaded-live-null / message-limit clamp). - `tests/test_coordinator_manager.py` — 8 new cases covering registry bootstrap on create + open, dispatch for each event type, unrelated-parent filtering, shutdown idempotency. - `tests/test_console.py` — existing `cluster_workstreams` assert updated for the new `extra_rows` kwarg. ## Verification - `ruff check turnstone tests` clean. - `mypy turnstone` clean. - `pytest -m "not live"` — 4184 passed, 3 deselected. * fix(coordinator): race in dispatch + ui_factory kwarg filtering — PR #370 review Addresses feedback from the GitHub Copilot + code-quality bot review passes on PR #370. ## Race in _dispatch_child_event ws_created branch Copilot flagged a TOCTOU where the lock-free read of ``self._active_coords`` (line 912) could see the parent coordinator, then ``close()`` / eviction pops ``_children[parent]`` + drops the coord from ``_active_coords`` before we acquire ``_children_lock``, and then ``setdefault(parent, set())`` resurrects the entry — leaking the registry key forever and fanning events to a closed UI. Fix: re-check ``parent in self._active_coords`` inside ``_children_lock``. The reference swap is still atomic; holding ``_children_lock`` and re-reading the snapshot catches the race without serializing back through ``self._lock``. Regression test: create → close → dispatch a ws_created → assert neither ``_children`` nor ``_active_coords`` regained the entry. ## ui_factory kwarg filtering via inspect.signature code-quality bot flagged that the previous ``try ui_factory(…, kind=, parent_ws_id=) except TypeError`` dance fired on every call with legacy test factories (``lambda wid: WebUI(ws_id=wid)``) — wasteful and masks real signature mismatches. Fix: inspect the factory's signature and only pass kwargs it actually accepts (explicit param name OR ``**kwargs`` absorber). Keep a conservative ``except TypeError`` fallback for C-callables and odd signatures ``inspect`` can't introspect. Copilot also flagged a comment mismatch (the old comment said "KeyError on **kwargs" — it's ``TypeError``, which is what the code caught). The rewritten comment is correct. ## Nit: side-effect in assert code-quality bot flagged ``assert mgr.close(ws.id)`` in test_coordinator_manager.py. Split into two statements. ## Verification - ``ruff check`` clean. - ``mypy turnstone`` clean. - ``pytest -m "not live"`` — 4223 passed, 3 deselected, 0 failed.	2026-04-17 14:38:13 -07:00
Patrick Buckley	8650370790	Feat/coordinator phase2 audit (#369) ... * feat(coordinator): audit middleware on routing proxy — phase 2 Adds per-tool-call audit attribution to the multi-node routing proxy handlers so coordinator → server hops land observable rows in ``audit_events``. Phase 1 preserved the ``src="coordinator"`` claim through ``_proxy_auth_headers``'s upstream re-mint; this commit closes the recording side. Was the last real security gap from phase 1 — an enterprise deployment with ``admin.coordinator`` granted got only the three console-side ``coordinator.{create,close,cancel}`` rows; per-tool-call attribution was missing. ## Action-naming scheme route.workstream.create POST /v1/api/route/workstreams/new route.workstream.send POST /v1/api/route/send route.workstream.close POST /v1/api/route/workstreams/close route.workstream.delete POST /v1/api/route/workstreams/delete route.approve POST /v1/api/route/approve route.cancel POST /v1/api/route/cancel route.command POST /v1/api/route/command route.plan POST /v1/api/route/plan Action-name conventions documented in ``turnstone/core/audit.py`` module docstring alongside the existing namespaces — the docstring is now ``<resource>.<verb>`` shaped (non-exhaustive) rather than trying to enumerate every prefix. ## Recording rules - ``record_audit()`` fires only on a 2xx upstream response. 4xx/5xx are observable via ``_record_route``'s metrics path; doubling the audit-events table size for failure rows would dilute signal without giving operators much extra value. - ``detail`` JSON carries ``{src, node_id, coord_ws_id?}`` — ``src`` lands verbatim from ``auth.token_source`` so non-coordinator origins (``"jwt"``, ``"console-proxy"``) also get attribution; ``coord_ws_id`` only appears when the inbound JWT carried it. - Wrapped in ``try/except`` + ``log.debug("route.audit_failed", ...)`` defence-in-depth. ``record_audit`` itself is fire-and-forget; the outer try guards against a programmer error in the call site. ## Routing-proxy specifics - ``route_create``: emits at the post-multipart/JSON convergence ``if resp.status_code == 200`` block. Both branches set ``audit_ws_id`` correctly — multipart from the query-string ws_id, JSON from ``body["ws_id"]`` (post-503-retry) or ``body["resume_ws"]``. - ``route_proxy``: emits the URL-method-mapped action. ``ref`` is reassigned to ``new_ref`` after a successful 404→cache-refresh retry so audit attribution uses the retried node, not the failed first node. - ``route_workstream_delete``: emits on 2xx using the ws_id from the request body. - ``route_attachment_proxy``: out of scope (upstream attachment endpoints emit their own ``workstream.attachment.*`` rows; auditing here would double-count). ## Tests 16 new tests in ``tests/test_route_proxy_audit.py`` covering: - Coordinator-origin emission with full detail payload. - 502 / 400 / 503-retry-final-node-id paths. - Parametrised method→action mapping for the 6 ``route_proxy`` URLs. - Plain-JWT origin (no ``coord_ws_id`` in detail). - Delete handler 2xx + 502. - Audit-storage exception swallowed (proxied response unchanged). - ``auth_storage`` absent → no-op (existing route-handler tests unaffected). Verification: ``ruff check`` clean, ``mypy turnstone`` clean (156 source files), ``pytest -m "not live"`` 4087 passed, 3 deselected (live-backend), 0 failed. * feat(coordinator): discovery tools and /open parity — list_nodes, list_skills, POST /coordinator/{ws_id}/open Adds the read-side surface coordinators need to make informed orchestration decisions plus an explicit rehydration endpoint matching the server's ``POST /v1/api/workstreams/{ws_id}/open``. ## list_nodes (auto-approved) ``list_nodes(filters={key: value, ...})`` reads ``node_metadata`` via ``storage.filter_nodes_by_metadata`` + ``get_all_node_metadata`` — one query each, no N+1. Each row carries its full metadata dict so the coordinator has both auto keys (``arch`` / ``cpu_count`` / ``fqdn`` / ``hostname`` / ``os`` / ``os_release`` / ``python``; always present) and operator-supplied user keys (``capability`` / ``region`` / ``tenant`` / ``role``) without a second round-trip. Tool description enumerates the auto keys explicitly so the model knows what's always available vs deployment-specific. Storage stores metadata values as JSON-encoded strings (the write path in ``server.py`` / ``admin.py`` / ``console/server.py`` all go through ``json.dumps``). The client re-encodes filter values before the stored-text comparison and decodes stored values before returning them to the model — so ``{"capability": "gpu"}`` is the natural form the model uses, not ``{"capability": "\"gpu\""}``. Ints round-trip as ints. Returns ``{nodes, truncated}``; ``truncated=True`` when the page was full. ## list_skills (auto-approved) ``list_skills(category?, tag?, scan_status?, enabled_only?, limit?)`` surfaces the skill registry so coordinators can discover worker profiles. New storage protocol method ``list_skills_filtered(...)`` on both SQLite and PostgreSQL backends pushes filters into SQL. ``tag`` filter matches against the JSON-array ``tags`` column with quote-bracketed substring (``%"tag"%``) — quote-safe against ``foo`` vs ``foobar`` collisions on both backends. Returns ``{skills, truncated}`` with ``name`` / ``category`` / ``tags`` (decoded to list) / ``version`` / ``description`` / ``model`` / ``enabled`` / ``scan_status`` / ``activation`` — the discovery projection, not the full row. ## POST /v1/api/coordinator/{ws_id}/open Explicit rehydration endpoint. Lazy ``GET`` rehydration works for the UI; this gives SDK callers and operators a way to warm a coordinator without browsing to it. Same ownership / 404-on- mismatch / correlation-id-masked error semantics as ``coordinator_detail``. Returns ``{ws_id, name, already_loaded?}``. Registered in ``turnstone/api/console_spec.py`` with a dedicated ``CoordinatorOpenResponse`` Pydantic model so the OpenAPI schema matches the wire shape. ## Tests - ``tests/test_storage_skills_filtered.py`` — 8 cases validated on BOTH SQLite and PostgreSQL backends (``pytest --storage-backend postgresql``). Covers no-filter ordering, category exact-match, tag quote-safety (``"foo"`` matches ``["foo","bar"]`` but not ``["foobar"]``), scan_status, enabled_only, limit, AND semantics, empty result. - ``tests/test_coordinator_client.py`` — 11 new cases covering node/skill shape decoding, JSON-encoded filter round-trip (the ``"gpu"`` vs ``'"gpu"'`` case), int filter encoding, truncation, no-match empty, no N+1 (``get_prompt_template`` / ``get_node_metadata`` call counts asserted zero). - ``tests/test_coordinator_tools.py`` — 11 new cases for ``_prepare``/``_exec`` dispatch, filter type-drop, limit clamping (``limit=0`` falls back to 100, negatives clamp to 1), truncation-signal summary. - ``tests/test_coordinator_endpoints.py`` — 8 new cases for ``/open``: ``already_loaded`` on in-memory hit, 404 on ownership mismatch, lazy rehydrate on miss, admin bypass, unknown ws_id, 503 on ``coord_mgr`` unavailable, 500 with correlation-id mask on factory failure, 503 passthrough on ``ValueError``. - ``tests/test_workstream_kind.py`` / ``test_tools_schema.py`` updated to include ``list_nodes`` and ``list_skills`` in the disjoint-namespace regression guard and the tool-count check. Verification: ``ruff check`` clean, ``mypy turnstone`` clean (156 source files), ``pytest -m "not live"`` 4122 passed, 3 deselected (live-backend), 0 failed. Postgres backend storage tests green (``pytest --storage-backend postgresql tests/test_storage_skills_filtered.py`` 8 passed). * feat(coordinator): task_list tool — persistent planning state Adds a coordinator-only ``task_list`` tool persisted on the coordinator's own ``workstream_config`` row. Gives coordinators a scratch surface for work decomposition that survives restarts so the UI can render planned-vs-done state once the tree view lands. ## Tool surface ``task_list(action, ...)`` with five actions: - ``list`` auto-approved read. Returns ``{tasks, truncated}``; truncated=True when the list exceeded the 200-row page cap. - ``add`` needs approval. ``title`` required; optional ``status`` and ``child_ws_id``. Title clamped at 200 chars. Capacity cap at 500 tasks — hitting the cap is an explicit signal to prune done/blocked rows. - ``update`` needs approval. Mutate by ``task_id``; fields ``title`` / ``status`` / ``child_ws_id`` optional. - ``remove`` needs approval. Drop by ``task_id``. - ``reorder`` needs approval. Pass ``task_ids``; validated as an exact permutation of the current set (rejects partial, extra, or substituted ids — prevents silent task loss). Status enum: ``pending`` / ``in_progress`` / ``done`` / ``blocked``. ``child_ws_id`` links a task to the child workstream spawned for it (no enforcement; the coordinator owns the relationship). ## Persistence Stored as a single JSON-envelope value on ``workstream_config`` — ``{"version": 1, "tasks": [...]}``. No new table; the kanban v2 work will supersede this row via a format migration keyed on ``version``. ``_save_task_list`` writes only the ``tasks`` key so concurrent writers to other ``workstream_config`` keys (e.g. the admin Settings UI updating ``reasoning_effort``) aren't clobbered by a read-modify-write on the full row. ## Corrupt-envelope safety A hand-edited or legacy config row that doesn't parse as the expected shape logs a warning and returns an empty envelope from ``task_list_get``. Mutators refuse to overwrite corrupt data — they detect the sentinel and return a clear error so the operator can inspect or clear the row rather than losing work silently. ## Concurrency Per-(ws) ``threading.Lock`` cached on the client. The worker thread is single-threaded for tool execs so this is mostly defence-in-depth against future maintenance-script call sites. Cache never grows beyond one entry per coordinator session because the scope guard short-circuits foreign ``ws_id`` before the lock is acquired. ## Malformed-JSON recovery ``_prepare_tool`` fallback-1 regex-extract allowlist extended with ``action`` / ``status`` / ``task_id`` / ``title`` (alphabetized) so slightly-malformed ``task_list`` calls get the same self-correction behaviour as the other coordinator tools. ## Tests - ``tests/test_coordinator_client.py`` — 15 new cases covering: fresh-envelope shape, add/get roundtrip, empty-title + invalid- status rejection, 200-char title clamp, update by id + missing id, remove semantics, reorder permutation validation (partial + extra + wrong id + valid), cross-ws scope violation, corrupt- JSON read recovery, corrupt-envelope write refusal (all four mutators), 500-task capacity cap, workstream_config key preservation across ``_save_task_list``. - ``tests/test_coordinator_tools.py`` — 12 new cases covering the dispatch layer: list auto-approved, each mutating action needs approval, unknown-action / missing-required-arg errors, list returns tasks, page-cap at 200 with truncated signal, add dispatches to client, reorder surfaces permutation error, remove-not-found. - ``tests/test_tools_schema.py`` / ``tests/test_workstream_kind.py`` extend the tool-count + disjoint-namespace + primary-key regression guards with ``task_list``. Verification: ``ruff check`` clean, ``mypy turnstone`` clean (156 source files), ``pytest -m "not live"`` 4148 passed, 3 deselected (live-backend), 0 failed.	2026-04-16 23:44:28 -07:00
Patrick Buckley	e42add1b77	feat(coordinator): coordinator workstream kind — phase 1 (#368) ... * feat(coordinator): coordinator workstream kind — phase 1 Adds a new ``kind="coordinator"`` workstream that runs inside the ``turnstone-console`` process (first ChatSession hosted on the console) with a dedicated tool set for spawning and driving child workstreams. Supersedes the external ``turnstone-coordinator`` MCP side-car for new installs; the extension is marked deprecated in ``examples/mcp-cluster-ops/README.md`` but still works on 1.4-and-earlier clusters. Phase 1 ships: the workstream class, 6 lifecycle tools, console hosting, 9 HTTP endpoints, per-user audit attribution, and a one-pane web UI at ``/coordinator/{ws_id}``. Node/skill discovery tools, task-list tool, tree-view UI, and routing-proxy audit middleware follow in a later PR. ## Schema Migration 039 adds ``kind`` / ``parent_ws_id`` columns + indexes to ``workstreams``. Both SQLite and PostgreSQL backends take the new kwargs on ``register_workstream``; empty-string ``parent_ws_id`` normalises to ``NULL`` at the storage edge. PostgreSQL uses ``INSERT ... ON CONFLICT DO NOTHING`` to match SQLite's ``OR IGNORE`` and close a pre-existing SELECT-then-INSERT TOCTOU window. ``list_workstreams`` gains optional ``parent_ws_id`` / ``kind`` filters; new ``get_workstream(ws_id)`` returns the full row (the existing ``get_workstream_metadata`` stays untouched for back-compat). ## Core session + kind routing - ``ChatSession.__init__`` accepts ``kind`` / ``parent_ws_id`` / ``coord_client``. On ``kind="coordinator"`` it swaps ``_tools = COORDINATOR_TOOLS`` and zeros sub-agent tool lists. - ``Workstream`` dataclass extended with ``user_id`` / ``kind`` / ``parent_ws_id``. Both ``WorkstreamManager`` and the new ``CoordinatorManager`` use the same type — no parallel hierarchy. - ``_SessionFactory`` Protocol + server / cli factory closures thread the new kwargs. ``POST /v1/api/workstreams/new`` rejects ``kind != "interactive"`` with 400; ``POST /v1/api/workstreams/{ws_id}/open`` refuses coordinator rows so a server node can't accidentally rehydrate one. ## Coordinator tool set Six tools (``spawn``, ``inspect``, ``send``, ``close``, ``delete``, ``list_workstreams``) with a ``coordinator: true`` metadata flag, scoped to coordinator-kind sessions only. ``inspect`` and ``list`` are auto-approved reads; the four mutators need approval. ``list`` returns ``{"children": [...], "truncated": bool}`` so the model can detect post-filter under-fill and paginate. ## CoordinatorClient (in-process, sync) Mutating ops HTTP-POST to the console's own ``/v1/api/route/*`` on the local bind URL so every existing middleware (auth, rate-limit) runs. Read ops hit ``storage.list_workstreams`` / ``get_workstream`` / ``load_messages`` directly — the routing proxy doesn't expose list/inspect paths. URL paths are a validated constant table (avoids an httpx ``base_url``-merge trap). A new ``/v1/api/route/workstreams/delete`` proxy handler joins the existing route-proxy endpoints. ## Per-session coordinator JWT ``CoordinatorTokenManager`` mints short-lived JWTs with ``sub=<real user>`` (attribution preserved), ``src="coordinator"``, ``aud="turnstone-console"``, ``coord_ws_id=<ws>`` custom claim. ``_proxy_auth_headers`` preserves ``src`` + ``coord_ws_id`` across the upstream re-mint so server-side middleware sees coordinator-origin, not ``console-proxy``. ``AuthResult.extra_claims`` carries non-reserved claims through validate→remint; ``create_jwt``'s reserved-claim set (now including ``nbf`` / ``jti``) is symmetric with ``validate_jwt``. ## Console hosts the ChatSession - New ConfigStore settings: ``coordinator.model_alias`` (required), ``reasoning_effort``, ``max_active`` (default 5), ``session_jwt_ttl_seconds``. - Console lifespan builds a ``ModelRegistry`` + ``CoordinatorManager``. Missing / unresolvable alias returns **503** with remediation text — never 500. - ``CoordinatorManager``: placeholder-slot reservation under lock, rollback on factory failure, per-ws_id rehydration lock to serialise concurrent lazy-opens, ``max_active`` enforced via ``close_idle`` eviction semantics. - ``ConsoleCoordinatorUI`` is a thin ``SessionUI`` implementation — no global broadcast, no per-node metrics, shared ``_APPROVAL_WAIT_TIMEOUT`` constant across approval + plan paths. - No eager startup rehydration: persisted coordinator rows load lazily on first ``GET /v1/api/coordinator/{ws_id}``. ## Console coordinator API Nine endpoints under ``/v1/api/coordinator/*`` gated by ``approve`` scope + new **``admin.coordinator``** permission (added to ``_VALID_PERMISSIONS``; not in any builtin role — operators opt in explicitly). Ownership failures return **404, not 403** and use strict equality so empty-owner rows don't leak across tenants. Correlation-id masking on every factory-raising path (``coordinator_create`` + ``coordinator_detail`` lazy rehydrate) — no stack traces to the client. ## Audit attribution Three console-side events (``coordinator.create`` / ``.close`` / ``.cancel``) with the real creator's ``user_id`` plus ``detail={coord_ws_id, src="coordinator"}``. No schema migration required. Per-tool-call audit across the routing proxy is deferred (needs either a ``source`` column on ``audit_events`` or ``record_audit`` calls wired into the route-proxy handlers). ## Web UI (``/coordinator/{ws_id}``) One-pane chat served by the console. Reuses ``shared_static`` (``base.css``, ``auth.js``, ``theme.js``, ``toast.js``, ``utils.js``, ``kb.js``) and the server UI's ``renderer.js`` pipeline (KaTeX, Mermaid, highlight.js already bundled). - SSE to ``/v1/api/coordinator/{ws_id}/events`` with exponential- backoff reconnect; status line carries a leading glyph (● / ○ / ⚠) so state isn't conveyed by colour alone. - Renders content, reasoning (dimmed italic ``.role-reasoning``), tool_result, approve_request, intent_verdict, output_warning. - Child ws_id references auto-wrap to ``/node/{node_id}/?ws_id={child}`` links — both ids regex-validated before interpolation, everything else HTML-escaped. - Non-modal approval bar (``role="region"``) with a batch header ("Approve N tool calls"), initial focus on the approve button, buttons disabled during the in-flight POST, red-bordered deny. ``aria-live`` flips to ``off`` during streaming. - "New coordinator" button on the dashboard header — permission-gated on the UI side, matching the backend 403. - Mobile composer capped under ``@media (max-width: 700px)``. ## Tests ~120 new tests across 8 files: workstream-kind storage + dataclass semantics, CoordinatorClient URL map + token minting + storage reads + truncation signalling, tool prepare/exec dispatch and approval gating, CoordinatorManager create / rollback / eviction / lazy rehydration + concurrency, HTTP endpoint auth + 404-on-ownership + 503-on-misconfig, proxy-auth ``src`` preservation, full lifecycle end-to-end, coordinator page HTML-injection guard. ``test_tools_schema.py`` widened to 25 tools (19 existing + 6 coordinator). Verification: ``ruff check`` clean, ``mypy turnstone`` clean (156 files), ``pytest`` 4054 passed (5 pre-existing failures unrelated to this change — confirmed against ``main``). * polish(coordinator): address PR review + CI + tool-namespace isolation CI: - `ruff format`: two files reformatted, matches the in-repo pre-commit config. - `wheel-completeness`: add `turnstone/console/static/coordinator/*.html` + `*.js` to the hatch wheel-include list. Without this the coordinator UI was missing from published wheels. - `test (3.11/3.12/3.13)` + `test-postgres`: three `TestExecReadImage` tests were masking a real bug — my 6 new tool JSONs pushed tool count 19→25, crossing the default `tool_search.auto` threshold (20), which made `ChatSession.__init__` construct a `ToolSearchManager` and cache `_cached_capabilities` during init. Tests that later patched `session._provider.get_capabilities` saw the cached value instead. Root-cause fix: the tool-search threshold code path now reads capabilities through `_resolve_capabilities(...)` directly — no cache populate — so the patch takes. Tool-namespace isolation (bigger fix than CI symptoms suggested): - `TOOLS` was the union of all loaded tool JSONs including the 6 new coordinator tools. Interactive sessions were getting coordinator tools in their function-calling surface (which is nonsense — they require a console-hosted `coord_client`), and coordinator sessions counted against the interactive tool-search threshold. Fix: - New `INTERACTIVE_TOOLS` / `INTERACTIVE_TOOL_NAMES` in `turnstone/core/tools.py` exclude anything with `coordinator: true` metadata. `TOOLS` stays as the union for schema introspection + eval catalog. - `ChatSession.__init__` selects tool set by kind: coordinator gets fixed `COORDINATOR_TOOLS` (no MCP merge, no listeners registered); interactive gets `INTERACTIVE_TOOLS` (+ MCP if configured). Coordinators are meta-orchestrators that spawn child workstreams; MCP tools / resources / prompts live on the children, not on the coordinator's own surface. - `_on_mcp_tools_changed` no-ops for coordinator sessions (defence-in-depth in case listeners were registered). - `always_on_names` on `ToolSearchManager` is now the set of builtin tools actually present in the session (kind-aware) rather than the full `BUILTIN_TOOL_NAMES` frozenset. - `turnstone/eval.py` uses `INTERACTIVE_TOOLS` (coordinator tools aren't in scope for the eval harness which tests interactive agent behaviour). - Regression tests in `tests/test_workstream_kind.py`: - `INTERACTIVE_TOOLS ∩ COORDINATOR_TOOLS == ∅` and their union is `TOOLS`. - Interactive `ChatSession._tools` does not include any coordinator tool name. - Coordinator `ChatSession._tools` contains `spawn_workstream` but not `bash` / `edit_file` / `memory`; sub-agent lists are empty. - Coordinator `ChatSession` with an MCP client attached does NOT merge MCP tools and does NOT register any MCP listeners. PR review findings: - **#10 / #11** (Copilot): coordinator UI claimed to reuse the server renderer pipeline but loaded none of its JS. Mirrored `turnstone/ui/static/renderer.js` into `turnstone/console/static/coordinator/renderer.js` (flagged in-file as a cleanup candidate to promote into `shared_static/`), added `katex.min.js` / `highlight.min.js` / `renderer.js` script tags to `coordinator/index.html`. `coordinator.js` now buffers raw markdown via `textContent` during streaming, then swaps to `renderMarkdown` + `postRenderMarkdown` on `stream_end`. - **#7** (Copilot): N+1 query pattern in `CoordinatorClient.list_children()` — per-row `storage.get_workstream` just to read `skill_id`. Pushed `skill_id` + `skill_version` into the `list_workstreams` SELECT projection on both backends; the client reads them from `row._mapping` directly. New `test_list_children_skill_filter_avoids_n_plus_one` pins the behaviour (asserts `storage.get_workstream` call count is 0). - **#8 / #9** (Copilot): `spawn_workstream` tool JSON said "if empty, the workstream is created idle" but the prepare method rejected empty and the field was marked required. Resolved by allowing empty end-to-end: removed from `required`, prepare builds a "spawn idle workstream" header + empty preview when empty, updated `test_spawn_prepare_allows_empty_initial_message`. - **#1–#5** (github-code-quality): five asserts with side-effecting method calls in `test_coordinator_manager.py` (`mgr.close`, `mgr.open`, `mgr.create` in a dead `_c = ...`). Extracted each call to a local variable so `python -O` can't strip the side effect. Verification: - `ruff check turnstone tests` clean. - `mypy turnstone` clean (156 source files). - `pytest -m "not live"` — 4063 passed, 3 deselected (live-backend tests), 0 failed. The 3 image tests that were failing on this branch now pass; wheel + lint both green locally. * polish(coordinator): address Copilot re-review findings Two findings from the re-review of #368 after the first polish commit. **user_id wired into `mgr.create()` at the server handlers.** Phase 1 added ``user_id`` to the ``Workstream`` dataclass and ``WorkstreamManager.create()`` signature, but the two call sites in ``turnstone/server.py`` forgot to pass the authenticated caller through. Result: interactive workstreams created via ``POST /v1/api/workstreams/new`` (including coordinator-spawned children, which route through this handler) were landing with blank ``user_id``, defeating ownership-based access control on subsequent sends / approvals / closes (``_require_ws_access`` treats blank owners as legacy/allowed). Two changes: - ``server.py:create_workstream`` forwards ``user_id=uid`` — the same ``uid`` already resolved from the auth result (with trusted-service forwarding preserved). - ``server.py:open_workstream`` prefers the persisted owner on the workstream row over the rehydrating caller so reloading someone else's workstream doesn't silently re-parent it. Falls back to the authenticated caller when the stored row has no owner recorded (pre-phase-1 rows). Regression test in ``tests/test_workstream.py`` pins ``WorkstreamManager.create(user_id=X)`` → ``ws.user_id == X`` so the manager seam can't regress silently on a future refactor. **Malformed-JSON recovery allowlist expanded for coordinator args.** ``_prepare_tool()`` has a two-stage salvage path for models that emit malformed JSON: a regex-extract (fallback 1) and a bare-string → primary_key wrap (fallback 2). The fallback-1 key list didn't include coordinator argument names, so a slightly malformed ``spawn_workstream`` / ``send_to_workstream`` / etc. call would hard-fail instead of salvaging into a minimal-args dict for retry. Added ``ws_id`` / ``message`` / ``initial_message`` / ``parent_ws_id`` to the allowlist (kept alphabetised) so the coordinator tools get the same model-self-correction behaviour as the interactive tools. Fallback 2 already covers the ``ws_id``-primary-key tools via ``PRIMARY_KEY_MAP``; the regex path matters when the model emits ``{"ws_id": "abc", "message": "..."}`` with a trailing syntax error. Verification: ``ruff check`` clean, ``mypy turnstone`` clean (156 source files), ``pytest -m "not live"`` → 4065 passed, 3 deselected (live-backend), 0 failed. * fix(coordinator): address ultrareview findings on coordinator workstream kind Security - Cross-tenant leak: CoordinatorClient.inspect/list_children now constrain to the coordinator's own ws_id + direct children; an LLM coerced via prompt injection can no longer exfiltrate other tenants' workstreams. - Empty-owner short-circuit bypass: strict equality at coordinator.py ownership gate and at the storage-fallback branch in coordinator_history; orphan/system-owned coordinator rows can no longer be rehydrated by arbitrary holders of admin.coordinator (DoS + history disclosure vector). - Closed coordinators no longer silently resurrect on subsequent GET — the Close button is now actually durable across URL revisits and tab refreshes; rows with state in {closed, deleted} refuse rehydration. Correctness - ChatSession.close() now releases the CoordinatorClient httpx.Client pool; previously every closed/evicted coordinator dropped a connection pool on the floor until non-deterministic GC. - open_workstream rehydration now forwards parent_ws_id + kind, so coordinator-spawned children survive node restart / idle eviction with their parent link intact instead of becoming silent orphans. - list_children truncated flag now signals whenever the SQL fetch hit the page cap (previously permanently False in the no-filter case, causing confident-but-incomplete summaries from the coordinator). - ConsoleCoordinatorUI.approve_tools: per-tool auto-approve now checks auto_approve_tools independently of the blanket auto_approve flag, so 'Always approve this tool' actually works on the next invocation. Concurrency - _spawn_worker no longer falls through to start a second concurrent worker thread on the same ChatSession when queue.Full fires; instead send() returns False and the endpoint surfaces HTTP 429. - _open_locks entries are now refcounted under self._lock and only popped when the last waiter releases — eliminates the race where a rehydration-failure path lets two threads serialize on different lock instances for the same ws_id and trip the "already tracked" guard. Tests: +6 regression cases covering closed-coordinator refusal, empty-owner non-admin refusal, queue.Full no-duplicate-worker, inspect/list_children cross-tenant rejection, and truncated semantics.	2026-04-16 21:40:50 -07:00