7 Commits

Author	SHA1	Message	Date
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