275 Commits

Author	SHA1	Message	Date
Patrick Buckley	fa09f7c9c0	fix: apply /review feedback on legacy URL cleanup ... Reviewer caught real misses on the consumer-swap claim: - TypeScript SDK still defined and re-exported `CloseWorkstreamRequest` (types.ts + index.ts) — drop both. Now matches the Python-side removal. - Four `tests/test_auth.py` cases (`test_write_full_token_ok`, `test_approve_full_token_ok`, `test_bearer_takes_precedence_over_cookie`, `test_cookie_full_on_write_ok`) were tautological after the legacy URL removal: they posted to `/api/send` / `/api/approve` and asserted `allowed is True`, but those paths now classify as `read` so a read token would also pass — they no longer tested the write/approve scope enforcement. Swap to path-keyed URLs to restore the original intent. - `is_public_path("/api/send")` test renamed + retargeted to a path-keyed URL. Doc-table drift the previous commit missed: - `docs/security.md` path-to-scope mapping rewritten for the path-keyed verb family (write set, DELETE-on-/send dequeue, per-ws_id approve). - `docs/architecture.md` scope-model row text swap from `/api/send` / `/api/approve` to the path-keyed equivalents. - `docs/diagrams/01-system-context.puml` channel→server edge label swap. - `docs/diagrams/15-auth-architecture.puml` scope class swap. Cosmetic comment-only stragglers: - `tests/test_session_worker.py` module docstring URL update. - `tests/test_ratelimit.py` ~11 `/api/send` fixture-key strings retargeted to `/api/workstreams/abc/send` so the URL fixtures reflect the post-1.5 surface (rate limiter is path-agnostic; the swap is purely cosmetic). 4557 tests still passing under -m "not live"; ruff + mypy clean.	2026-04-26 21:06:25 -07:00
Patrick Buckley	347dc22c0b	docs: mark 1.5.0 legacy URL surface removal ... CHANGELOG [Unreleased] / Removed (BREAKING — 1.5.0) block calling out the legacy URL family removal with the swap table. Doc passes on api-reference.md (per-endpoint sections rewritten with path parameters and slimmer body shapes), architecture.md (handler-list diagram and console-proxy URL example), console.md (URL-rewriting JS shim docstring + SSE proxy example), and the two PlantUML diagrams (11-console-data-flow, 16-channel-architecture). Also picks up two test-side stragglers from step 5 that referenced the legacy adapters in a docstring + a stale /v1/api/events SSE test: turn into path-keyed equivalents. OpenAPI JSON dump regenerated to reflect the catalog edits from step 3. After this commit: - 4557 tests passing under -m "not live" - ruff + mypy clean on turnstone/ tests/ sdk/ - grep for "/v1/api/send", "/v1/api/approve", "/v1/api/cancel", "/v1/api/workstreams/close" returns zero hits across turnstone/ sdk/ docs/ tests/ (excluding CHANGELOG.md, which intentionally documents the old shape). - grep for make_legacy_body_keyed_adapter, make_legacy_query_keyed_adapter, _make_method_dispatch, close_legacy returns zero hits.	2026-04-26 20:57:35 -07:00
Patrick Buckley	bf55bf54ff	chore(tests): refresh fixtures for path-keyed URL family ... Mechanical updates across the test suite to swap legacy /v1/api/{send,approve,cancel,events,workstreams/close} URLs for the path-keyed equivalents under /v1/api/workstreams/{ws_id}/<verb>, and to drop ws_id from request bodies (the path provides it now). Per file: - test_session_routes.py: deletes test_close_legacy_mounts_when_handler_provided (the close_legacy slot is gone); test_send_mounts_post_and_delete_when_dequeue_provided (added in PR commit 1) stays. - test_openapi.py: expected-paths set swaps to path-keyed shape; test_send_endpoint_has_request_body now asserts the OpenAPI for /v1/api/workstreams/{ws_id}/send. - test_auth.py / test_auth_identity.py: required_scope and check_request fixtures swap to path-keyed shape; new tests cover write/approve/read scope assignment for the path-keyed verbs + the /node/* proxy mirror. - test_sdk_server.py / test_sdk_console.py: mock-transport URL keys swap; bodies drop ws_id. - test_server_attachments_endpoints.py: ~17 send sites migrated to /v1/api/workstreams/<ws>/send (a small Python script ran the bulk rewrite — body ws_id stripped, URL rebuilt). - test_server_authz.py: cross-tenant approve/close/cancel/events tests retargeted to path-keyed URLs; test_events_legacy_query_keyed_url_still_resolves_to_404_for_unknown_ws renamed to test_events_path_keyed_url_resolves_to_404_for_unknown_ws with the docstring updated to note the legacy adapter is gone. - test_close_reason_persistence.py: 7 close sites all swap. - test_console_routing_proxy.py: route-proxy tests swap to /v1/api/route/workstreams/{ws_id}/<verb>; the upstream-URL assertion now reads from .request (route_proxy uses client.request(method, url, ...) for method passthrough); _wire_proxy helper installs both .post and .request mocks for compatibility. - test_route_proxy_audit.py: parametrized URLs migrated; _make_proxy now also exposes a .request side-effect that delegates to .post for the same compatibility surface. - test_api_versioning.py: openapi.json path assertion swaps to the path-keyed shape. 4557 passing under -m "not live"; ruff + mypy clean.	2026-04-26 20:49:49 -07:00
Patrick Buckley	ae4a84e552	refactor(routes): wire DELETE on path-keyed workstreams/{ws_id}/send ... Pre-flight for the legacy URL adapter removal: the path-keyed `/v1/api/workstreams/{ws_id}/send` route only mounted POST today; the dequeue handler was reachable only via the legacy `DELETE /v1/api/send` body-keyed URL through `_make_method_dispatch`. Add a new `dequeue: Handler | None = None` slot on `SharedSessionVerbHandlers` next to `send`, mounted as a second `Route` on the same path with `methods=["DELETE"]` (two distinct Routes rather than collapsing methods on one Route — different handler callables, and collapsing would force the same method-dispatch wrapper this cleanup is tearing out). Wire `dequeue=dequeue_handler` in `turnstone/server.py`'s `SharedSessionVerbHandlers(...)` call so DELETE on the path-keyed shape works in the same merge as the legacy mount removal. Adds a regression-locking test covering both the POST+DELETE and the dequeue-alone cases.	2026-04-26 20:17:52 -07:00
Patrick Buckley	6572437c5d	refactor(server): rename dashboard row id → ws_id for v1 row-shape consistency ... The /v1/api/dashboard endpoint was the last workstream-listing surface keyed on `id` rather than `ws_id`. The Stage 2 list-verb lift converged the active list (`/v1/api/workstreams`) and saved list (`/v1/api/workstreams/saved`) on `ws_id` but explicitly left dashboard alone to keep that PR's diff focused. This lands the same rename on the remaining endpoint so v1 row shape is consistent across the family. Scope kept narrow: - Pydantic `DashboardWorkstream` and TS SDK `DashboardWorkstream` interface both rename `id: str/string` → `ws_id`. - The bundled web UI (`turnstone/ui/static/app.js`) is the only consumer reading `dashboard.workstreams[].id` and is updated atomically. - Console `_fetch_live_block` (cluster-inspect's projection over a remote node's dashboard payload at `turnstone/console/server.py`) flips its `entry.get("id")` lookup to `entry.get("ws_id")`. - Drive-by: stale `id` example in `docs/api-reference.md` for the earlier `/v1/api/workstreams` rename also fixed. `_build_node_snapshot` (the global-events SSE node_snapshot payload consumed by the cluster collector) deliberately stays on `id` — it's part of a separate cluster-row family (collector → cluster_workstreams → console UI) that is internally consistent on `id` and would need its own coordinated sweep. CHANGELOG documents the bounded blast radius. Tests: 4554 passing (-m "not live"). ruff + mypy clean.	2026-04-26 20:00:45 -07:00
Patrick Buckley	3abd2c441b	feat(console): coord rich ws_state payload + live activity broadcast (#420) ... * feat(console): coord rich ws_state payload + live activity broadcast (Stage 2 follow-up) Pre-lift coord's cluster broadcast was state-only — the dashboard's coord rows showed the state column flipping but ``tokens`` / ``context_ratio`` / ``activity`` / ``content`` were all hardcoded to zero / empty. The lift makes coord populate the same per-ws metric fields interactive does and broadcasts them through the cluster collector with the rich kwargs. **Architecture changes:** - Lift ``on_status`` / ``on_content_token`` / ``on_thinking_start`` / ``on_thinking_stop`` / ``on_stream_end`` / ``on_tool_result`` / ``on_reasoning_token`` / ``on_tool_output_chunk`` / ``on_info`` / ``on_error`` from ``WebUI`` to :class:`SessionUIBase` as base implementations. Coord inherits the bodies; the per-ws metric fields it had at the base but never populated now flow. - ``WebUI`` keeps overrides for ``on_status`` / ``on_tool_result`` / ``on_error`` to layer Prometheus ``_metrics.record_*`` calls on top of ``super()`` (node-only — the console isn't a node). ``WebUI._broadcast_state`` now uses the new :meth:`SessionUIBase.snapshot_and_consume_state_payload` helper for the rich-payload snapshot read. - ``ConsoleCoordinatorUI`` adds a ``_broadcast_activity`` override that calls the new :meth:`ClusterCollector.update_console_ws_activity` (in-memory pseudo-node row update; named ``update_*`` rather than ``emit_*`` to flag the no-fanout asymmetry vs. the rest of the ``emit_console_ws_*`` family). - ``coord_adapter.emit_state`` reads ``ws.ui``'s snapshot under ``_ws_lock`` and passes the rich kwargs to the extended :meth:`ClusterCollector.emit_console_ws_state`. Defensive when ``ws.ui is None`` mid-eviction (broadcasts state-only). - ``coord_endpoint_config`` wires a new ``_coord_spawn_metrics`` hook so per-spawn ``_ws_messages`` / ``_ws_turn_tool_calls`` bookkeeping fires on coord too. - ``_MAX_TURN_CONTENT_CHARS`` moved from ``turnstone.server`` to ``turnstone.core.session_ui_base`` so coord enforces the same per-turn content cap. **Three observable behaviour changes** (CHANGELOG-callout-worthy): - Coord persists ``usage_event`` storage rows on every status emission (governance dashboards / token-spend queries gain coord visibility). - Coord broadcasts live activity transitions to the cluster collector (dashboard's coord rows show activity ticks between state changes the same way interactive does), with last-emitted dedup so a tool-heavy turn's repeated ``activity=""`` clears don't hammer the collector lock. - Cluster ``cluster_state`` events for coord rows now carry non-zero ``tokens`` / ``content``. Frontend rendering that conditionally hid these on coord can drop the branch. **Tests:** 23 new tests in ``tests/test_coord_rich_ws_state_payload.py`` (per-ws metric writes, snapshot helper drain semantics + single-lock-acquisition, adapter rich-payload pass-through + None-UI defensive handling, activity broadcast wire + dedup + failure swallow + no-op-when-collector-unset, spawn_metrics hook, concurrent-writes-during-snapshot stress with reader cycling through running/idle/error so drain branches actually run, on_stream_end activity-clear pin). Plus WebUI override regression tests confirming ``_metrics.record_*`` still fires on top of the lifted bodies. Existing ``tests/test_webui_content.py`` updated to import ``_MAX_TURN_CONTENT_CHARS`` from its new home; ``tests/test_coordinator_adapter.py`` updated to expect the rich-payload kwargs (default zeros) on ``emit_console_ws_state``. Total: ``4491 → 4514``. ``ruff check`` clean, ``mypy`` clean on touched files. **/review pipeline** (4 finders → verify → dedupe) caught 14 findings → 12 unique (3 collapsed as duplicates of the lockless ``on_content_token`` writer): - bug-1 Minor: ``on_status`` regressed coord's defensive ``usage.get(...)`` indexing → restored ``.get(..., 0)`` for ``prompt_tokens`` / ``completion_tokens`` on both base + WebUI override. - bug-2 Nit: concurrent-snapshot reader only used ``"running"`` → cycled through ``("running", "idle", "error")`` so drain branches run; also captures + re-raises thread exceptions instead of silently passing. - bug-3 + sec-2 + perf-3 Nit (merged): ``on_content_token`` mutated ``_ws_turn_content`` lockless while the snapshot drained under lock → wrapped the cap-check + append + size-update in ``_ws_lock``. - perf-2 Minor: collector lock contention from per-event activity broadcasts → cached last-emitted ``(activity, activity_state)`` on the UI; subsequent identical ticks return early without acquiring the collector lock. - perf-4 Nit: join-under-lock in snapshot helper → swap-then-join pattern (capture list reference under lock, reassign to empty, join the captured list outside the lock). Halves the lock hold and decouples the join walk from concurrent appenders. - q-1 Minor: ``emit_console_ws_activity`` was misleading (no ``_fanout`` call, unlike the rest of the ``emit_console_ws_*`` family) → renamed to ``update_console_ws_activity`` + docstring call-out for the asymmetry. - q-2 + q-3 Minor/Nit: stale docstrings on ``coordinator_ui.py`` (still claimed "no per-node metrics — Phase D") and ``_interactive_spawn_metrics`` (still claimed "counters live on WebUI only") → both updated to reflect the lifted base class + coord's new hook. - q-4 Nit: broken Sphinx cross-ref ``:meth:\`_snapshot_and_consume_state_payload\``` → dropped the leading underscore. - q-5 Nit: missing ``test_coord_on_stream_end_clears_activity`` → added. **Two findings explicitly deferred** (out-of-scope follow-ups, documented in CHANGELOG): - perf-1: synchronous ``record_usage_event`` INSERT on coord worker thread per status tick. Parity with WebUI is the lift's goal; if throughput becomes a concern, batch usage_event writes on a background flusher (would apply to both kinds). - sec-1: coord assistant content now flows on the cluster SSE stream, which has no per-user filter today. Pre-existing exposure for interactive ``cluster_state`` events; the lift extends to coord rows. Proper fix needs SSE auth gating (``admin.cluster.inspect``) or per-listener user_id filtering — separate security project, doesn't gate this lift. * fix(console): apply review feedback on PR #420 Three review findings, all confirmed against source: 1. **Copilot — dedup-state-vs-failure race in `_broadcast_activity`** (correctness bug): pre-fix ``self._last_broadcast_activity = current`` was assigned inside the ``_ws_lock`` block BEFORE the collector call. If the collector raised mid-broadcast, the exception was swallowed but the dedup state was already updated, so subsequent identical activity ticks would be deduped and never retried — leaving the dashboard's coord row stranded at the pre-failure activity until the activity actually changed. Fix: move the dedup-state update OUT of the lock and place it AFTER a successful collector call. On failure, ``_last_broadcast_activity`` stays unchanged so the next identical tick retries. Two new regression tests pin both the failure-recovery (``test_coord_ui_ broadcast_activity_failure_does_not_strand_dedup``) and the happy-path dedup behavior (``test_coord_ui_broadcast_activity_ dedup_skips_identical_after_success``). 2. **Copilot — stale `emit_console_ws_activity` reference in CHANGELOG**: the method was renamed to ``update_console_ws_activity`` per /review's q-1 finding before the original commit landed, but the CHANGELOG entry was written ahead of the rename. Updated to match the actual API + added the no-fanout asymmetry rationale inline so readers don't have to chase the method name. 3. **code-quality bot ×2 — `except BaseException` in test workers**: the concurrent-snapshot stress test caught thread-worker exceptions with ``except BaseException`` (with a noqa to suppress BLE001). ``BaseException`` is overkill for a thread worker — ``SystemExit`` / ``KeyboardInterrupt`` are main-thread signals and ``Exception`` is the right scope. Narrowed to ``except Exception`` on both workers; ``writer_exc`` / ``reader_exc`` types narrowed from ``list[BaseException]`` to ``list[Exception]``. Tests: ``4514 → 4516`` (+2 regression tests for the dedup race fix). ``ruff check`` clean, ``mypy`` clean. No code-path changes outside the dedup-state placement; the rich-payload broadcast surface is unchanged.	2026-04-26 17:32:44 -07:00
Patrick Buckley	d555816016	refactor(core): lift history + detail verb bodies across both kinds (Stage 2 verb lift) ... Last verb-shape lift before v1.5.0 stable can tag. Adds two new factories to ``turnstone/core/session_routes.py``: - ``make_history_handler(cfg)`` — body lifted from coord's ``coordinator_history`` near-verbatim. ``?limit=`` query param defaults to 100, clamps to [1, 500], malformed values fall back to 100. Storage operations (``get_workstream`` on the storage-fallback path, ``load_messages`` for the row read) now run via ``asyncio.to_thread`` (was inline pre-lift on coord). - ``make_detail_handler(cfg)`` — body lifted from coord's ``coordinator_detail``. Lazy-rehydrates a closed/evicted workstream via ``mgr.open()`` on miss; mirrors :func:`make_open_handler`'s exception envelope (``ValueError`` → 503 with the session-factory's remediation text; bare ``Exception`` → correlation_id'd 500 with the per-kind noun via ``cfg.audit_action_prefix``). NO new ``SessionEndpointConfig`` fields — the factories reuse ``permission_gate``, ``manager_lookup``, ``not_found_label``, ``audit_action_prefix``, and (for history's storage-fallback kind check) ``list_kind`` — all already wired by both production lifespans for the list/saved factories. Coord side: ``coordinator_history`` and ``coordinator_detail`` standalone handler bodies removed from ``console/server.py``; ``register_session_routes`` now wires ``history=make_history_handler(coord_endpoint_config)`` and ``detail=make_detail_handler(coord_endpoint_config)``. Interactive side: GAINS both endpoints as a feature gain. Pre-lift interactive had no ``GET /v1/api/workstreams/{ws_id}`` and no ``GET /v1/api/workstreams/{ws_id}/history`` — SDK consumers had to subscribe to ``/events`` SSE just to read display fields or message rows. The same lifted factories are wired with the interactive endpoint config; cross-kind isolation is preserved on both sides (history via ``cfg.list_kind`` storage-fallback gate + fail-loud-on-misconfig 500; detail via ``mgr.open()``'s internal kind check). Pydantic schemas: ``CoordinatorDetailResponse`` / ``CoordinatorHistoryResponse`` removed from ``console_schemas.py``; ``WorkstreamDetailResponse`` / ``WorkstreamHistoryResponse`` added to ``server_schemas.py`` (mirrors the list lift's pattern for ``WorkstreamInfo``). Both server and console OpenAPI specs reference the unified schemas; ``server_spec.py`` gains ``EndpointSpec`` entries for the new interactive endpoints. TS SDK gains both interfaces in ``sdk/typescript/src/types.ts``; ``openapi-{server,console}.json`` regenerated. Tests: 6 new coord regression/parity tests in ``test_coordinator_endpoints.py`` (limit clamping, cross-kind 404 on storage fallback, storage-only history, detail 503 on session-factory misconfig, detail 500 with correlation_id on unexpected rehydrate failure, history swallows ``load_messages`` exception → 200 with empty messages). 10 new interactive parity tests in ``test_workstream_endpoints.py`` (``TestHistoryInteractive`` + ``TestDetailInteractive``). 1 new openapi spec test pinning the server-side ``?limit=`` query param. Total: ``4490 → 4491`` after the new exception-swallow regression test landed. ``ruff check`` clean, ``mypy`` clean on touched files. /review pipeline (4 finders → verify → dedupe) caught 1 Minor defense-in-depth (bug-1/sec-1, merged: ``make_history_handler`` fail-closed gate when ``cfg.list_kind is None``, mirroring ``make_saved_handler``'s same gate) + 1 Minor test-helper rename (q-1: ``_interactive_history_cfg`` → ``_interactive_endpoint_cfg``) + 4 Nits (q-2 unused fixture parameter, q-3 CHANGELOG TS SDK mention, q-4 missing exception-swallow regression test, q-5 misleading test comment) — all addressed in the same commit.	2026-04-26 15:48:26 -07:00
Patrick Buckley	edf52016ac	refactor(core): lift list + saved verb bodies across both kinds (Stage 2 verb lift) ... New ``make_list_handler(cfg)`` and ``make_saved_handler(cfg)`` factories in ``turnstone/core/session_routes.py`` replace four pre-lift bodies (interactive ``list_workstreams`` + ``list_saved_workstreams``; coord ``coordinator_list`` + ``coordinator_saved``). Same factory + capability-flag pattern as the merged cancel / open / events / create lifts. Four new ``SessionEndpointConfig`` fields: - ``list_resolve_titles: ListResolveTitles | None`` — bulk lookup ``(ws_ids) -> {ws_id: title-or-None}``. Interactive wires ``get_workstream_display_names`` (new bulk helper added on the storage layer + memory.py); the lifted body resolves every active row in ONE ``SELECT ... WHERE ws_id IN (...)`` instead of the pre-lift N+1 (one SELECT per row). - ``list_kind: WorkstreamKind | None`` — explicit kind classifier for the saved-list storage filter. Replaces the initial draft's ``audit_action_prefix == "coordinator"`` string compare which would have silently leaked INTERACTIVE rows for any future kind whose audit prefix didn't match. Required when a kind mounts list/saved; misconfig surfaces as a 500 with a clear log line. - ``saved_state_filter: str | None`` — coord wires ``"closed"``; interactive wires ``None``. - ``saved_loaded_lookup: SavedLoadedLookup | None`` — coord-only defence-in-depth filter that excludes ws_ids in the warm pool. Behaviour changes (all observable in CHANGELOG): - **Active-list row shape converges on always-include** ``{ws_id, name, state, kind, parent_ws_id, user_id}``. Interactive renames ``id`` → ``ws_id``; both kinds populate every field (coord adds kind + parent_ws_id; interactive adds user_id). - **Top-level response key converges on ``"workstreams"``** on both endpoints. Coord ``coordinators`` key removed — coord is a 1.5.0aN-only surface (never shipped stable) so the convergence has no compat shim; SDK / frontend consumers swap once. - **Storage + manager-lock work moved off the event loop on interactive**. ``list_workstreams_with_history`` runs through ``asyncio.to_thread`` on both kinds (matches coord's pre-existing perf-2 pattern from the saved-coordinators review); ``mgr.list_all`` + per-row work also offloaded. - **N+1 storage round-trips on /v1/api/workstreams eliminated**. Pre-lift interactive resolved the alias for every active row in a separate SELECT (up to 50 round-trips per dashboard refresh on a saturated node). Lifted body issues one bulk SELECT. Pydantic schemas: ``WorkstreamInfo.id`` renamed → ``ws_id``, ``WorkstreamInfo.user_id`` field added. ``CoordinatorInfo`` and ``CoordinatorListResponse`` removed (folded into the unified ``WorkstreamInfo`` / ``ListWorkstreamsResponse``). OpenAPI spec snapshots regenerated. TS SDK types updated (``WorkstreamInfo`` interface gains ws_id + the always-include fields); TS test mock + assertion updated to match. ``GET /v1/api/dashboard`` is intentionally NOT in this PR's scope and still returns rows keyed on ``id``. Tracked as a separate cleanup PR (tombstone-note added at the dashboard handler). /review pipeline run; the four Major findings + one Minor + six nits all addressed in the same commit: - M1: TS SDK ``WorkstreamInfo`` interface stale (id: string) → renamed + fields added. - M2: TS SDK test masked the type-mismatch with stale mock → updated. - M3: N+1 alias resolution on active list → bulk ``get_workstream_display_names`` helper + ``list_resolve_titles`` bulk cfg hook. - M4: Missing interactive parity regression test for unified row shape → mirror of coord's added in test_server_authz.py. - Mi1: ``audit_action_prefix`` string-compare deriving kind → explicit ``cfg.list_kind: WorkstreamKind`` field. - Six nits: redundant inner asyncio import, forward-ref quotes on Awaitable, duplicated frontend comments, dashboard ``id`` field has no tombstone-note, empty-coord_mgr short-circuit on ``saved_loaded_lookup``. 4512 tests passing; ruff + mypy clean.	2026-04-26 13:11:07 -07:00
Patrick Buckley	c77b237033	refactor(core): defer emit_created on SessionManager.create + commit_create / discard pair (#417) ... * refactor(core): defer emit_created on SessionManager.create + commit_create / discard pair Eliminates the phantom create→close pair on coord rollback that was documented as a known limitation in PR #416. The pair surfaced on the cluster events stream when a multipart workstream-create request failed attachment validation: coord's ``mgr.create`` fired ``emit_created`` synchronously, then the rollback called ``mgr.close`` which fired ``emit_closed``. Cluster consumers had to reconcile via the collector's diff path. Post-fix, a rejected upload produces zero events. API changes on ``SessionManager``: - ``create(..., defer_emit_created: bool = False)`` — when True, skip the trailing ``emit_created`` so the caller can run additional post-create work (attachment validation in the lifted HTTP handler) before advertising the workstream. Default preserves the existing "advertise immediately" contract for direct callers (test fixtures, CLI REPL, channel adapters). - ``commit_create(ws)`` — fires the deferred ``emit_created`` event after the caller's post-create work confirms the workstream should be advertised. Synchronous; the wrapped work is in-memory and non-blocking on every kind (interactive: documented no-op stub; coord: dict updates under a lock + ``queue.put_nowait`` fan-out). - ``discard(ws_id)`` — releases the in-memory slot + cleans up the UI WITHOUT firing ``emit_closed``. Distinct from ``close`` which advertises the transition; ``discard`` is for the rollback case where the workstream's existence was never advertised. Storage-row deletion stays a separate concern (caller invokes ``delete_workstream``), mirroring ``mgr.create``'s split between slot reservation and ``register_workstream``. Caller-bug detection: ``Workstream._emit_created_fired`` is set inside ``create`` (non-deferred path) and ``commit_create``; ``discard`` logs ``session_mgr.discard.after_emit_created`` warning when invoked on an already-advertised workstream. Slot is still released so capacity isn't stranded. Lifted ``make_create_handler`` updated to use the deferred bracket: pass ``defer_emit_created=True``, validate uploaded attachments, then ``mgr.commit_create(ws)`` on success / ``mgr.discard(ws.id)`` on failure. Ordering invariants (``commit_create`` BEFORE ``audit_emit`` and ``post_install`` so any state events the worker fires reach the cluster collector for an already-known ws_id) are documented in the handler docstring. Tests: - 5 new ``SessionManager`` unit tests (defer skips emit, commit fires it, commit no-ops without emitter, discard releases without emit_closed, discard returns False on unknown id). - 2 caller-bug regression tests (commit_create after discard pins the silent re-emit behaviour; discard after non-deferred create asserts the warning fires + slot still releases). - 1 coord regression test asserting the cluster collector sees zero events when attachment validation fails. ``/review`` pipeline run; M1 (test gap on caller-bug paths) + Mi1 (no runtime guard for already-advertised) + Mi2 (``_make_manager`` event_emitter override) + Mi3 / N2 (duplicated comments + ordering invariant) + N1 (drop ``to_thread`` on ``commit_create``) all addressed. 4509 tests passing; ruff + mypy clean. * fix(core): apply Copilot + code-quality review feedback on PR #417 Copilot review: - ``Workstream._emit_created_fired`` comment claimed the flag was "set under the manager's _lock-protected emit", but the actual ordering set it OUTSIDE the lock. Comment updated to describe the real synchronization (non-deferred ``create`` sets it immediately before ``emit_created``; ``commit_create`` sets it under the manager lock alongside the tracked-ws check). - ``commit_create`` had no guard against duplicate calls, post-discard calls, or calls on workstreams not tracked by this manager — any of those would have fired duplicate or phantom ``ws_created`` events. Added a guard symmetric to ``discard``'s after-emit warning: under ``self._lock``, check ``_emit_created_fired`` + ``_workstreams.get(ws.id) is ws``, no-op + log a warning (``session_mgr.commit_create.already_fired`` / ``session_mgr.commit_create.untracked``) on either failure. The emit itself still runs outside the lock so coord's collector fan-out doesn't couple to the manager mutex. - ``test_commit_create_after_discard_is_caller_bug_no_op`` was internally inconsistent — name + docstring said "must not re-emit" but the assertion expected the re-emit. Renamed to ``test_commit_create_after_discard_is_no_op`` and updated to assert the new no-op + warning behaviour. New test ``test_commit_create_is_idempotent_on_duplicate_call`` pins the second-commit-call code path: exactly one ``ws_created`` event fires, second call short-circuits via the guard with a ``commit_create.already_fired`` warning. Code-quality bot review (3 findings, identical pattern): - Three test ``assert`` statements wrapped side-effecting calls (``assert mgr.discard(ws_id) is True/False``); under ``python -O`` the asserts strip and the side-effect strips with them. Refactored all three to assign the result to a local first, assert on the local. No behaviour change. 4510 tests passing; ruff + mypy clean.	2026-04-26 12:06:59 -07:00
Patrick Buckley	16916dc257	fix(core,console): coord create-time attachments coordination + Copilot review feedback on PR #416 ... Coord initial-message + create-time-attachments coordination: - ``CoordinatorAdapter.send`` gains optional ``attachments`` + ``send_id`` kwargs so the worker dispatched at create time can carry the uploaded files onto the first turn. Mirrors interactive's pre-existing worker-thread pattern. The ``send_id`` reservation token soft-locks the rows; the adapter's failure path unreserves so a worker crash returns them to pending. - ``_coord_create_post_install`` reserves any uploaded ``attachment_ids`` via the lifted ``reserve_and_resolve_attachments`` helper before dispatching through the adapter — closes the parity gap with interactive's create-with-attachments+initial_message flow. - ``_reserve_and_resolve_attachments`` lifted from ``turnstone/server.py`` to ``turnstone/core/attachments.py`` as ``reserve_and_resolve_attachments`` so both processes use one kind-agnostic implementation. Copilot review fixes on PR #416: - Skill lookup now calls ``storage.get_prompt_template_by_name`` directly rather than going through ``turnstone.core.memory.get_skill_by_name``; that helper swallows storage exceptions into ``None`` which would have masked outages as the 400 "Skill not found" branch. Calling storage directly lets exceptions bubble to the lifted body's correlation_id'd 500 path so operators chasing skill-related reports can distinguish real misses from registry outages. - ``_interactive_create_build_kwargs`` / ``_coord_create_build_kwargs`` thread ``skill_data["name"]`` (the canonical row name) into ``mgr.create`` instead of the raw ``body["skill"]`` value. Pre-fix a whitespace-padded request body ``"skill": " my-skill "`` would have persisted the dirty name even though the lookup ran on the stripped key. - ``make_create_handler`` docstring corrected: audit-emit failures return 200 (not 201). - ``_audit_workstream_created`` docstring corrected: factory keeps the successful 200 response on audit-emit failure (was 201). New regression test: ``test_create_with_multipart_attachments_and_initial_message_reserves`` asserts attachments are reserved (not pending) when both ``initial_message`` and uploads land in the same coord create request. Updated ``_SendSession`` stub in ``test_coordinator_adapter.py`` to match the new ``send`` / ``queue_message`` signatures. 4501 tests passing; ruff + mypy clean.	2026-04-26 04:07:39 -07:00
Patrick Buckley	9ed8b1e0b5	refactor(core): lift create verb body across both kinds (Stage 2 verb lift) ... New ``make_create_handler(cfg, *, audit_emit=None)`` factory in ``turnstone/core/session_routes.py`` consumes five new ``SessionEndpointConfig`` fields (``create_supports_attachments``, ``create_supports_user_id_override``, ``create_validate_request``, ``create_build_kwargs``, ``create_post_install``) and replaces both ``create_workstream`` and ``coordinator_create`` bodies. Same factory + capability-flag pattern as the merged cancel / open / events lifts. ``_validate_and_save_uploaded_files`` lifted to ``turnstone.core.attachments`` so both processes call one kind-agnostic implementation. Coord parity gains (§ Post-P3 reckoning item #1 + carry-forward): - Create-time attachments: multipart parsing, validate+save+rollback, ``attachment_ids`` on the response. Coord adapter ``send`` doesn't yet reserve attachments at create time, so the rows save as pending and the next ``/send`` picks them up via the standard send-with-attachments path. - Disabled-skill rejection (matches interactive's pre-lift gate). - Always-include response shape ``{ws_id, name, resumed, message_count, attachment_ids}`` populated with default ``False``/``0``/``[]`` on the fields coord doesn't fill. - 200 status (was 201). - Audit-emit failures swallow + warning log instead of 500. Both kinds converge on the manager-at-capacity 429, factory-misconfig 503, and correlation_id'd 500 for unexpected ``mgr.create`` failure (interactive lifted up to coord's safer error envelope). Three /review fixes folded in: - ``notify_targets`` malformed input gates at the validator (400) instead of bubbling out of post_install as a 500 — pre-fix the workstream had already been created + audited + broadcast by the time the validation raised. - Skill-lookup storage failures now share the correlation_id'd 500 path with ``mgr.create`` (was masquerading as 400 "Skill not found"). - Whitespace-only ``skill`` field treated as empty (matches pre-lift coord). CHANGELOG entry under [Unreleased] documents every observable behaviour change. OpenAPI spec regenerated. Three new coord regression tests (create-time-attachments save pending rows, always-include parity fields, disabled-skill rejection) plus one interactive regression test (notify_targets 400). 4500 tests passing.	2026-04-26 04:07:39 -07:00
Patrick Buckley	577ad2824f	refactor(core): lift events verb body across both kinds (Stage 2 verb lift) (#415) ... * refactor(core): lift events verb body across both kinds (Stage 2 verb lift) The interactive ``GET /v1/api/events?ws_id=...`` and coord ``GET /v1/api/workstreams/{ws_id}/events`` SSE handlers now share one body via ``make_events_handler(cfg)``. Per-kind divergence captured by two new ``SessionEndpointConfig`` fields: * ``events_replay: EventsReplay | None`` — Protocol-typed callback that yields the kind-specific initial replay payload. Interactive wires ``_interactive_events_replay`` (connected + status + history + pending_approval + cached intent verdicts + pending_plan_review); coord wires ``_coord_events_replay`` (just pending_approval + pending_plan_review). The lifted body iterates the callback before starting the live event loop. * ``sse_executor_lookup: SseExecutorLookup | None`` — per-kind executor for the live loop's blocking ``client_queue.get``. Interactive returns the dedicated 200-thread ``sse_executor`` from app state so SSE polling stays isolated from every other ``asyncio.to_thread`` caller in the process; coord returns ``None`` and the lifted body falls through to the default executor. Also adds ``make_legacy_query_keyed_adapter(handler)`` (sister to ``make_legacy_body_keyed_adapter`` from earlier lifts): reads ``ws_id`` from the query string and splices into ``request.path_params`` before delegating to the lifted body. Preserves the ``GET /v1/api/events?ws_id=...`` legacy URL shape so any 1.x SDK consumer keeps working. Old ``events_sse`` (server.py) + ``coordinator_events`` (console/server.py) bodies deleted. Two convergence wins for coord: * **SSE connect/disconnect metrics** — pre-lift coord didn't record per-stream metrics; the lifted body always calls ``metrics.record_sse_connect()`` / ``record_sse_disconnect()``, giving the cluster dashboard the same per-stream observability interactive's had since 1.0. * **Both kinds now check ``request.is_disconnected()`` AND the ``ws_closed`` event** to terminate. Pre-lift interactive relied solely on ``ws_closed`` (which never fires if the client just goes away without closing the workstream); pre-lift coord relied solely on ``is_disconnected``. The lifted body uses both. One observable shape change for coord callers: the lifted body returns 409 ``"session has no UI"`` when ``ws.ui`` is missing (placeholder / build-failed UI), matching pre-lift coord. Pre-lift interactive returned 404 in this case; the lift converges on 409 because the workstream EXISTS (404 would imply it doesn't). Item #2 from § Post-P3 reckoning (rich ``ws_state`` payload parity for coord) split out during scoping — touches different files (``coordinator_ui.py`` + ``collector.py`` + ``session_ui_base.py``) with different reviewer concerns. Tracked as standalone follow-up ``feat/coord-rich-ws-state-payload``. Two /review fixes folded in: * **Dedicated SSE thread pool restored.** Initial draft used ``asyncio.to_thread`` (default executor, ~32 workers). Pre-lift interactive deliberately used a dedicated 200-thread ``sse_executor`` to avoid pool starvation; the ``sse_executor_lookup`` cfg field above restores that isolation. * **5s poll timeout restored.** Initial draft shortened to 1s, multiplying thread-wakeup rate 5x while the pool was already starving. ``is_disconnected()`` between polls covers cancel- detection latency. Plus minor cleanups: stale ``coordinator_events`` comment references in coordinator.js refreshed; ``TestInteractiveEventsLifted`` gets a ``_make_interactive_replay_mocks`` fixture so per-test intent stays clear; live-loop coverage gap documented in the test class docstring. Lint + mypy clean. 4497 tests passing (+8 new events tests). * fix(core): stream events replay from inside the generator instead of pre-building PR #415 review caught that ``make_events_handler`` pre-built the full replay payload (``connected`` + ``status`` + ``history`` + pending prompts) into a list before constructing the ``EventSourceResponse``. Two real costs: * **TTFB delay** — the client saw nothing until the heaviest replay event finished serialising (``_build_history`` on a long-running interactive workstream can take 10s of ms). With pre-build, the ``connected`` event was buried at the end of the materialisation pass instead of streaming first. * **Listener-queue accumulation** — registering the per-UI listener BEFORE building the replay let live events queue during the build window. On a chatty mid-generation workstream that window can fill the 500-slot listener queue and drop events before the live loop starts draining. Fix: iterate ``cfg.events_replay`` inside the async generator so each event ships as soon as the callback yields it. The observational-failure swallow semantics are preserved by wrapping the iteration in the same try/except + log.debug as before — partial replay is still acceptable; the live loop continues either way. Resolves the Copilot review thread on PR #415. Lint + mypy clean. 4497 tests passing (no test changes — the replay callbacks themselves are unchanged; only the lifted body's consumption pattern flipped from eager-build to lazy-stream).	2026-04-26 01:58:41 -07:00
Patrick Buckley	f9ed4d3071	refactor(core): lift open verb body across both kinds (Stage 2 verb lift) (#414) ... * refactor(core): lift open verb body across both kinds (Stage 2 verb lift) The interactive ``POST /v1/api/workstreams/{ws_id}/open`` and coord ``POST /v1/api/workstreams/{ws_id}/open`` handlers now share one body via ``make_open_handler(cfg, *, audit_emit=None)``. Per-kind divergence captured by two new ``SessionEndpointConfig`` fields: * ``open_resolve_alias: AliasResolver | None`` — interactive wires ``resolve_workstream`` so callers can pass user-friendly aliases in the path param. Coord wires ``None``. * ``open_post_load: OpenPostLoad | None`` — interactive wires ``_interactive_open_post_load`` (display-name sync + UI replay via ``clear_ui`` + history + handler-side ``ws_created`` enqueue onto the global SSE queue). Coord wires ``None`` and relies on the cluster collector fan-out from ``CoordinatorAdapter.emit_rehydrated``. Plus an optional ``audit_emit`` parameter (interactive wires ``_audit_workstream_opened``; coord wires ``None`` — coord doesn't audit open today). Old ``open_workstream`` (server.py) + ``coordinator_open`` (console/server.py) bodies deleted. **Load-bearing fix** (§ Post-P3 reckoning item #3 from the planning docs): pre-lift interactive's ``open_workstream`` called ``mgr.create(ws_id=resolved_id)`` + ``ws.session.resume(...)`` to rehydrate, bypassing ``mgr.open()`` entirely. After the lift both kinds route through ``mgr.open()`` — which makes ``InteractiveAdapter.emit_rehydrated`` reachable on interactive (it had been dead-by-routing) and gives the manager a single rehydrate code path to maintain. ``emit_rehydrated`` stays a documented no-op stub on the interactive adapter; the handler-side ``ws_created`` enqueue from the post-load callback is the load-bearing emission for the SSE consumers. Behaviour changes for interactive callers (documented in CHANGELOG): * **Cross-kind open returns 404** (was 400 with ``"Workstream is not an interactive kind"``). The lift consolidates on ``mgr.open()``'s single ``None``-return contract for missing / wrong-kind / tombstoned rows. Security boundary unchanged. * **Already-loaded response uses ``ws.name`` directly** (was ``get_workstream_display_name(resolved_id) or resolved_id``). The dashboard listing endpoint still resolves aliases on its own pass, so the user-visible name in the tab strip isn't affected. Two /review fixes folded in: * **Resume failures now return 5xx instead of broken-200.** ``SessionManager.open()`` previously caught and ``log.debug``- swallowed exceptions from ``ChatSession.resume``. Since ``ChatSession.resume`` assigns ``self.messages`` *before* the config-restore block, a partial-failure resume (corrupted ``workstream_config`` row, model-registry mismatch on a saved alias, malformed ``temperature`` / ``max_tokens``) would leave the session with history but with default config. Pre-lift the interactive open handler called ``ws.session.resume`` directly and let exceptions propagate as 500. Restored that behaviour: ``mgr.open()`` now re-raises resume exceptions after rolling back the slot (``cleanup_ui`` + ``_remove_locked``), so the lifted handler returns 500 with a correlation id and the storage row stays available for a retry. * **Bare ``except Exception`` documents intent.** A one-line rationale in the handler body explains why the catch is broad (no documented exception spec on ``adapter.build_session``; resume can propagate via the new contract above). Keeps a future contributor from narrowing it incorrectly. Test scaffolding: * ``tests/test_workstream_endpoints.py`` — fixture rebuilt to use ``make_open_handler`` + a minimal cfg with a lazy alias resolver so per-test ``@patch`` calls take effect. Added 5 new tests: already-loaded uses ws.name, alias resolution runs first, ``mgr.open`` is called (NOT ``mgr.create``), post-load callback fires with (request, ws) only on the load-from-storage path (not the already-loaded shortcut), post-load exception swallowed → 200. * ``tests/test_coordinator_endpoints.py`` — fixture imports updated to ``make_open_handler``. * ``tests/test_server_authz.py`` — ``TestOpenKindGate`` now expects 404 (not pre-lift's 400) for cross-kind open attempts. Docstring explains the consolidation. Two nit cleanups: dropped the unnecessary ``import secrets as _secrets`` aliasing in the exception handler; refreshed the stale ``open_workstream`` reference in the ``AliasResolver`` doc-comment. Lint + mypy clean. 4488 tests passing (was 4475; +13 new open tests). * fix(core): use cfg.audit_action_prefix for the per-kind noun in open's 500 error PR #414 review caught the hardcoded ``"failed to open workstream"`` in ``make_open_handler``'s 500 path: coord callers got misleading text (pre-lift coord said ``"failed to open coordinator"``). The fix derives the noun from ``cfg.audit_action_prefix`` ("workstream" interactive, "coordinator" coord) — a field both production lifespans already construct, and which the previous /review pipeline (q-5) flagged as dead config (set but read by no factory). Reusing it here both fixes the wording AND gives the field its first runtime reader. Pinned by a new test (``test_open_500_message_uses_kind_noun_from_cfg``) that wires a coord-shaped cfg, forces ``mgr.open`` to raise, and asserts the 500 body contains ``"failed to open coordinator"`` + the correlation id, without echoing the exception text. Lint + mypy clean. 4489 tests passing (+1 new).	2026-04-26 00:44:14 -07:00
Patrick Buckley	412c99f486	refactor(core): lift cancel verb body across both kinds (Stage 2 verb lift) (#413) ... * refactor(core): lift cancel verb body across both kinds (Stage 2 verb lift) The interactive ``/v1/api/cancel`` (body-keyed ws_id) and coord ``/v1/api/workstreams/{ws_id}/cancel`` (path-keyed) handlers now share one body via ``make_cancel_handler(cfg, *, audit_emit=None)`` in ``turnstone.core.session_routes``. Per-kind divergence captured by a new ``cancel_forensics: CancelForensics | None`` field on ``SessionEndpointConfig`` (interactive wires ``_capture_cancel_forensics``; coord wires ``None``) plus an optional ``audit_emit`` (coord wires ``_audit_cancel_coordinator``; interactive wires ``None`` — pre-lift interactive didn't audit cancel). Same factory + capability-flag pattern as P1.5's ``make_send_handler`` + make_attachment_handlers. Old ``cancel_generation`` body deleted from ``server.py``; old ``coordinator_cancel`` body deleted from ``console/server.py``. Behavior changes (documented in CHANGELOG): * **Coord gains the ``force`` flag.** Pre-lift coord ignored ``force``; the lifted body honours it on both kinds. Stuck-worker recovery becomes available on coord (parity gain — coord workers hang the same way interactive's can). * **Coord cancel response always includes ``"dropped"``.** Pre-lift returned bare ``{"status": "ok"}``; lifted returns ``{"status": "ok", "dropped": {}}``. Always-include parity with interactive so SDK consumers don't branch on kind. * **Coord cancel returns 400 ``"No session"``** on placeholder / build-failed workstreams (was a silent 200 no-op pre-lift). Parity with interactive's existing 400 branch. * **Coord ``coordinator.cancel`` audit detail now includes ``force``** so operator-driven recovery is distinguishable from routine cancels. Three /review fixes folded in: * **bug-1**: lifted body's ``resolve_approval`` is now gated on ``ui._pending_approval is not None``. Pre-fix, the unconditional call leaked a stale ``approval_resolved`` SSE event on every idle cancel — listener UIs that key on the event would dismiss prompts they didn't have. ``resolve_plan`` keeps its existing internal no-pending guard so the unconditional call is still safe there. * **bug-2**: force-cancel now clears ``_worker_running`` alongside ``worker_thread`` inside the same ``with ws._lock`` block. Prior half-state ``(_worker_running=True, worker_thread=None)`` routed follow-up sends through the queue-enqueue path onto the abandoned worker (whose cancel flag short-circuits the queue-drain seam, leaving messages orphaned until next spawn). Restores the ``(worker_thread, _worker_running)`` invariant ``session_worker.send`` documents. * **bug-3**: ``coordinator_stop_cascade._fanout_on_children`` now treats child cancel ``400 + "No session"`` as ``skipped`` (was ``failed``). Lifted coord cancel returns 400 on placeholder children; matches the pre-lift outcome where those children were silently no-op'd, so the cascade response's ``failed`` bucket stops firing spurious operator alerts. Test scaffolding: * ``tests/test_coordinator_endpoints.py`` — replace ``coordinator_cancel`` fixture with ``make_cancel_handler(...)`` wiring; add 6 new tests covering always-include shape, force-flag worker-abandon, 400-on-null-session, cancel_forensics swallowed-exception, audit_emit swallowed-exception, no-stale-approval-resolved-on-idle. * ``tests/test_server_authz.py`` — new ``TestInteractiveCancelLifted`` class with HTTP-level coverage of ``/v1/api/cancel`` for the dropped shape, force-flag + ``_worker_running`` clearing, and 400-on-null-session. Pre-lift ``cancel_generation`` had no HTTP-level test; this is the first. One observable change for interactive (pre-existing call site): ``resolve_approval`` / ``resolve_plan`` now run on every cancel regardless of ``was_running`` (was gated). Lifts coord's unconditional behaviour onto interactive — a stuck approval-pending state from a crashed worker can now be cleared via cancel without requiring close + rehydrate. Lint + mypy clean. 4484 tests passing (was 4475; +9 new cancel tests minus the moved one that became part of the new suite). * docs(core,changelog): correct cancel-lift behaviour description for resolve_approval Two review comments on PR #413 caught the same drift between the implementation and its documentation: my bug-1 fix gated ``resolve_approval`` on ``_pending_approval is not None`` (because it broadcasts ``approval_resolved`` unconditionally), but the ``make_cancel_handler`` docstring and the CHANGELOG entry still claimed both ``resolve_approval`` and ``resolve_plan`` "run on every cancel" and "the calls are idempotent and no-op when nothing is blocked". Reality: * ``resolve_plan`` does run on every cancel and its no-op-when- nothing-pending behaviour is real (the method has an internal ``_pending_plan_review is None`` short-circuit). * ``resolve_approval`` runs only when ``ui._pending_approval is not None``. Without the gate, every idle cancel would broadcast a stale ``approval_resolved`` SSE event and overwrite ``_approval_result``. Updated: * ``make_cancel_handler`` docstring (turnstone/core/session_routes.py in the "Behavior changes vs the pre-lift handlers" section) — splits the two methods into separate bullets, explains why ``resolve_approval`` is gated and ``resolve_plan`` isn't. * CHANGELOG.md ``[Stage 2 Verb Lift — cancel]`` entry — same split + rationale; the asymmetric coord pre-lift parity is still flagged as the recovery path that drove the lift. Docs-only change; lint + mypy clean; cancel test suite (59 tests) unchanged. * style(core): replace CancelForensics ellipsis stub with docstring github-code-quality bot flagged the ``...`` body of ``CancelForensics.__call__`` as "Statement has no effect". The ellipsis is the canonical Protocol method-body idiom (no real issue), but switching to a one-line docstring satisfies the bot AND adds a small piece of method-level documentation. The class- level rationale (why Protocol-typed instead of a plain Callable alias) moves from a wall of leading ``#`` comments into a proper class docstring at the same time. Style-only change; the Protocol semantics are identical.	2026-04-25 23:43:19 -07:00
Patrick Buckley	48c9ad2a40	refactor(core): split SessionKindAdapter Protocol into construction +… (#412) ... * refactor(core): split SessionKindAdapter Protocol into construction + emission (Stage 2 P3) The single ``SessionKindAdapter`` Protocol that ``SessionManager`` takes is split into two: * ``SessionKindAdapter`` — kind / build_ui / build_session / cleanup_ui. Required for every kind. The shared lifecycle manager always delegates here for construction + cleanup. * ``SessionEventEmitter`` — emit_created / emit_state / emit_rehydrated / emit_closed. **Optional**, wired through a new ``event_emitter: SessionEventEmitter | None = None`` kwarg on ``SessionManager``. Reserved for future kinds whose lifecycle transitions don't fan out anywhere; both production kinds wire one today. Both production adapters implement both Protocols. The interactive lifespan (``server.py``) and console lifespan (``console/server.py``) pass their adapter as both ``adapter`` and ``event_emitter`` — production behaviour is unchanged. Six lifecycle sites in ``SessionManager`` (create / open eviction / open rehydrate / close / set_state / close_idle / _reserve_and_install_locked unwind) now call ``self._event_emitter.emit_*(...)`` guarded by ``if self._event_emitter is not None``. InteractiveAdapter asymmetry preserved + documented: * ``emit_closed`` stays load-bearing — it's the **sole** transport path for ``ws_closed`` onto the process-wide global SSE queue (Stage 1 consolidated emission from the create handler here so there's exactly one emission point; ``name`` powers the frontend's eviction toast). * ``emit_created`` / ``emit_state`` / ``emit_rehydrated`` are documented no-op stubs (``del ws[, state]``). Those events fire from out-of-band paths — the create HTTP handler enqueues ``ws_created`` directly onto ``global_queue`` *after* attachment validation (so a rejected upload doesn't surface a phantom create→close pair); ``WebUI._broadcast_state`` emits the full ``ws_state`` payload (tokens + context_ratio + activity) via the ``SessionUI.on_state_change`` callback chain. The stubs exist solely to satisfy ``SessionEventEmitter`` Protocol so the adapter can be wired as the manager's ``event_emitter`` for the ``emit_closed`` path. Each stub has a 1-line inline rationale to match the in-repo convention (``coordinator_adapter.py:210``). Test scaffolding: * ``tests/test_session_manager.py`` — ``_make_manager`` and ``_make_with_writer`` wire ``FakeAdapter`` as both ``adapter`` and ``event_emitter`` for production parity; the standalone ``test_create_uses_configured_node_id`` does the same. ``FakeAdapter.emit_rehydrated`` now records as ``_Event("rehydrated", ...)`` rather than conflating with ``"created"``, and ``test_open_resurrects_closed_state`` asserts against ``events_of("rehydrated")`` so a regression where the manager fires the wrong call on the open path actually fails. * ``tests/_coord_test_helpers.py`` and ``tests/test_coordinator_end_to_end.py`` — wire ``CoordinatorAdapter`` as both args. * Six interactive test fixtures (``test_skills.py``, ``test_prompt_templates_runtime.py`` x2, ``test_model_registry.py``, ``test_server_authz.py``, ``test_server_attachments_on_create.py``) — wire ``event_emitter=adapter`` so they match the production wiring, removing the footgun where a future contributor adds a ``gq.get_nowait()`` assertion and silently loses the only ``ws_closed`` transport. * ``tests/test_interactive_adapter.py`` — drops the three tautological no-op-emit_* tests (``test_emit_created_is_noop``, ``test_emit_state_is_noop``, ``test_emit_rehydrated_is_noop``); keeps the four ``emit_closed`` tests (real behaviour). Lint + mypy clean. 4475 tests passing. * docs(core): correct SessionKindAdapter + SessionEventEmitter docstrings to match implementation Two Copilot review threads on PR #412 caught the same real discrepancy: my P3 docstrings on ``SessionKindAdapter`` and ``SessionEventEmitter`` described an *intent* — "interactive doesn't implement ``SessionEventEmitter``; the manager skips emit calls when no emitter is wired" — that doesn't match the actual wiring. ``InteractiveAdapter`` does implement both Protocols and ``server.py`` does pass it as ``event_emitter``; only the three no-op stubs (``emit_created`` / ``emit_state`` / ``emit_rehydrated``) are dead, while ``emit_closed`` is load-bearing. Updated both docstrings to: * State that both production adapters implement both Protocols. * Explain the asymmetry is in *which* emit methods carry real bodies (coord: 4; interactive: 1, with 3 documented stubs because the out-of-band paths — create handler ``ws_created`` after attachment validation, ``WebUI._broadcast_state`` carrying the richer ``ws_state`` payload — fire those events). * Clarify the ``if self._event_emitter is not None`` guard exists for the kwarg-omitted case (tests that don't care about events, reserved for future kinds whose transitions don't fan out anywhere). Docstring-only change. Lint + mypy clean; the 75 tests in test_session_manager + test_interactive_adapter + test_coordinator_adapter pass. Resolves the two Copilot review threads on PR #412 (commits PRRC_kwDORcMomM67VyPD, PRRC_kwDORcMomM67VyPI).	2026-04-25 22:52:39 -07:00
Patrick Buckley	ad56192a96	fix(core,console): address /review feedback on Stage 2 P1.5 ... Six fixes from the local /review pipeline (find-bug + find-security + find-quality, all confirmed by verify): * **sec-1 (major)** — coord ``attachment_owner_resolver`` now resolves through ``coord_mgr.get(ws_id)`` only and does NOT fall back to storage. Without the kind-strict check, an ``admin.coordinator``-scoped caller could pass an *interactive* workstream ws_id to the new coord attachment endpoints; the generic ``get_workstream_owner`` storage call (kind-agnostic) would resolve and grant cross-kind read / write access to interactive attachments. New regression test ``test_coord_attachment_endpoints_404_on_interactive_ws_id`` pins the surface. * **bug-1 (minor)** — UI hook calls in the spawn-path ``_run`` closure are now wrapped per-hook (via ``_emit_ui``) so a failure in ``ui.on_error`` doesn't suppress the subsequent ``ui.on_stream_end`` / ``ui.on_state_change`` calls. Mirrors the pre-P1.5 coord_adapter.send per-hook defense. * **bug-2 (minor)** — ``make_dequeue_handler`` now 404s when ``ws.ui is None`` (preserves the pre-P1.5 ``_get_ws`` contract; a partially-constructed or close-window workstream shouldn't answer DELETE). * **bug-3 (minor)** — ``coordinator.js`` gains a ``case "message_queued":`` handler that surfaces the queueing as an info row. Coord wires ``emit_message_queued=True`` for parity with interactive but the dashboard had no router branch for these events, silently dropping them. * **bug-4 (minor)** — error-message format on coord regressed from ``f"{type(exc).__name__}: {exc}"`` to ``f"Error: {e}"`` (lost the exception class name, which coord operators rely on to triage failures). Restored. * **q-1 (major)** — duplicate ``_auth_user_id`` and ``_require_ws_access`` helpers in ``server.py`` and ``console/server.py`` now delegate to the lifted ``turnstone.core.web_helpers.auth_user_id`` / ``resolve_workstream_owner``. The lifted versions are the canonical implementations; the shims keep existing call sites working without a sweeping rename. CHANGELOG entry adds a Security section noting the kind-strict resolver fix and a behaviour callout for the cancel-state semantic.	2026-04-25 21:44:40 -07:00
Patrick Buckley	e0c78e2aec	test,docs: coord attachment + queue parity tests + spec regen + CHANGELOG ... Five new TestCoordinatorAttachments tests in ``tests/test_coordinator_endpoints.py`` exercising the lifted attachment surface end-to-end on coord: * upload → list round-trip * get_content returns raw bytes with text/plain forced for text * delete removes pending entries and clears them from the listing * send with attachment_ids consumes pending under the send_id token * send response carries attached_ids / dropped_attachment_ids even on plain-text sends (unified shape parity) The existing ``_coord_endpoint_config`` fixture grew capability flags to mirror the production console wiring, and ``_make_client`` now mounts the four coord attachment routes via ``make_attachment_handlers``. OpenAPI specs regenerated; TS SDK bumped to 0.5.0. CHANGELOG entry under [Unreleased] documents the verb-shape lift, the coord attachment surface coming online, the response-shape change for ``coordinator_send``, the unification of the three lifted classifier / lock helpers under ``turnstone.core.attachments``, and the new SDK helpers.	2026-04-25 21:44:40 -07:00
Patrick Buckley	a8cd9444b1	fix(server): apply Copilot + code-quality review feedback ... PR #410 review pass: * **session_worker**: ``except BaseException`` → ``except Exception`` in ``_runner`` (code-quality bot). Daemon threads don't receive SystemExit/KeyboardInterrupt, so the wider catch was unjustified defensive style. Same defense-in-depth for unexpected ``run()`` exceptions; doesn't widen scope to runtime signals. * **session_worker**: ``threading.Thread()`` construction moved inside the spawn branch under ``ws._lock`` (Copilot). The enqueue path no longer allocates and then discards a Thread object on each call against a busy workstream. Thread() construction is microsecond-cheap, so the lock-window growth is negligible vs. the saved allocation churn. * **lifespans**: ``state_writer.shutdown()`` (and the console equivalent) now run via ``asyncio.to_thread`` so the daemon- thread join + sync DB drain don't block the event loop and delay other teardown tasks (Copilot, ×2). * **tests**: five remaining ``writer._flush_once()`` calls switched to the public ``writer.flush()`` API across test_session_manager.py (4) and test_state_writer.py (1) (Copilot, ×5). Tests no longer depend on private internals.	2026-04-25 20:11:47 -07:00
Patrick Buckley	52e09e87d6	fix(core): address /review feedback on Stage 2 P1 ... Six fixes from the local /review pipeline (find-bug + find-perf + find-quality, all confirmed by verify): * **bug-1 (critical)** — ``StateWriter.record(flush_now=True)`` now drops any pending buffered transient for the same ws_id AND waits on the flush_lock before its sync UPDATE. Without this, an earlier buffered 'running' could flush AFTER the sync 'error' write and clobber the terminal state — same shape as the close-vs-buffered-transient race ``discard`` was already guarding. New regression tests cover both the drop and the in-flight wait. * **bug-3 (major)** — ``session_worker.send`` now assigns ``ws.worker_thread = t`` AND sets ``ws._worker_running = True`` under the same ``ws._lock`` acquisition. Previously ``worker_thread`` was assigned outside the lock, so a reader holding ``ws._lock`` could observe ``_worker_running=True`` paired with a stale (already-exited) ``worker_thread`` — defeating every ``ws.worker_thread is me`` identity check downstream. * **bug-2 (major)** — rewind/retry busy gate in ``server.py:command`` now reads ``ws._worker_running`` instead of ``ws.worker_thread.is_alive()``. The is_alive() gate could see a stale dead thread under ws._lock while a new worker was in the middle of starting (post bug-3 fix the window narrows but the gate-mismatch was independent — ``_worker_running`` is the canonical gate post-Stage-2-P1). * **perf-2 (major)** — ``StateWriter.discard`` now waits on ``_flush_lock`` with a 5s timeout (configurable). Without a bound, a stuck Postgres connection inside an in-flight flush would block ``close()`` and ``close_idle()`` indefinitely while they hold ``ws._lock`` — a system-wide hang on every close path. On timeout we log + proceed; the worst-case degrades to "buffered transient flushes shortly after sync 'closed'" (eventual consistency) rather than process hang. * **q-1** — inline comments on ``run_retry`` and ``_run_initial`` now explain why those two spawn sites don't go through ``session_worker.send``: retry-when-busy is a hard reject (no fallback queue), and init-on-create can't have a pre-existing worker by construction (enqueue branch is dead code). Both still set ``_worker_running`` + ``ws.worker_thread`` together under ws._lock for parity with the dispatcher. * **q-3** — ``state_writer.discard`` callsite comments in ``session_manager.py`` no longer reference 'bug-3' (which lived only in untracked working notes). Now describe the invariant inline by what it prevents. * **q-5** — ``StateWriter._flush_once`` promoted to public ``flush()``. Tests now drive flushes via the public API.	2026-04-25 20:11:47 -07:00
Patrick Buckley	8240e32704	test(core): regression tests for state_writer + close ordering ... Five new tests under ``TestSessionManagerWithStateWriter`` exercise the bug-3 invariant under write-behind: * set_state buffers via the writer (long flush_interval → no sync write until drain). * set_state(ERROR) flushes synchronously. * close after a buffered transient writes 'closed' as the final state — the buffered 'running' must NOT be flushed to storage AFTER close's sync 'closed' write. * close_idle exhibits the same invariant. * set_state arriving AFTER close short-circuits on ws._closed and never reaches the buffer.	2026-04-25 20:11:47 -07:00
Patrick Buckley	470a6af6a9	feat(core): add state_writer for buffered set_state persistence ... ``turnstone.core.state_writer.StateWriter`` buffers non-terminal ``update_workstream_state`` writes (last state per ws_id wins) and flushes them on a ~1s cadence (configurable). Terminal ERROR transitions and close()'s 'closed' write bypass the buffer. Bounded buffer (``max_buffer=10000`` default) evicts the oldest ws_id on insertion overflow — protects against unbounded growth when storage is unreachable. ``discard(ws_id)`` drops any pending buffered transition AND waits on a flush_lock for any in-flight write to complete; this is the close-path hook that preserves the bug-3 invariant (a closed row can't be resurrected by a buffered transient writing AFTER close's sync 'closed'). 13 unit tests cover coalescing, flush_now, bounded buffer, the discard / in-flight-flush wait, lifecycle (start/shutdown idempotence), wake-on-record latency, and resilience to storage errors poisoning subsequent flushes.	2026-04-25 20:11:47 -07:00
Patrick Buckley	4e791cfb15	refactor(server): swap interactive workers to session_worker.send ... Five spawn sites in turnstone/server.py now share the worker dispatch: * ``send_message`` (``POST /v1/api/send``) — the main path. Now uses ``session_worker.send`` with separate ``_enqueue`` / ``_run`` closures; the queue-vs-spawn outcome is conveyed via a captured ``queue_outcome`` dict so the existing response shapes (``status: queued`` vs ``status: ok``) survive. * ``_make_watch_dispatch`` — watch results dispatch. * ``run_retry`` (post-rewind) and ``_run_initial`` (initial-message on workstream creation) — set ``_worker_running`` directly under ws._lock instead of going through session_worker (their structural shape doesn't fit a queue-vs-spawn decision) but stay consistent with the shared gate so they can't race with /send into parallel workers. * ``cancel_generation``'s ``was_running`` snapshot now reads ``_worker_running`` for parity with the dispatcher. Pre-dispatch cancel-await also gates on ``_worker_running`` for consistency. The ``busy_error`` / ``status: busy`` legacy branch (reached only when worker is alive but ws.session is None) is gone — the new path checks ws.session up front and returns the same 500 shape. Test fixtures in test_server_attachments_endpoints.py and test_watch_dispatch.py updated to set ws._worker_running explicitly (MagicMock auto-truthifies the field, which would otherwise mis-route all idle paths into queue mode).	2026-04-25 20:11:47 -07:00
Patrick Buckley	7ffe8d1ca3	feat(core): add session_worker shared dispatch + delegate coord adapter ... Introduces ``turnstone.core.session_worker.send`` — the atomic check-and-(spawn-or-queue) decision both interactive and coordinator HTTP paths use to drive ``ChatSession.send``. Callers pass no-arg ``enqueue`` / ``run`` closures; the shared module owns only the ``ws._worker_running`` lifecycle. CoordinatorAdapter.send now delegates to the shared module — its ``_spawn_worker`` body is gone. Workstream._worker_running's docstring updated to note both kinds use it post-Stage-2-P1.	2026-04-25 20:11:47 -07:00
Patrick Buckley	abf7f62301	fix(server): address PR #409 review feedback ... PR #409 line-level review feedback. Three of four findings valid; the fourth (code-quality bot's "unused TYPE_CHECKING imports") verified as false-positive — removing the imports breaks mypy on the string-form annotations in ``ManagerLookup`` / ``TenantCheck`` / ``CloseAuditEmitter``. CI lint failure (ruff format on ``tests/_coord_test_helpers.py``) addressed alongside. Findings addressed: - **Copilot #1** (``session_routes.py`` SessionEndpointConfig docstring): said the config is "stored on ``app.state.session_endpoint_config``" and "handler bodies pull this config from app.state". Stale after the previous fixup switched the factories to capture ``cfg`` via closure. Rewrote the class docstring + the lifted-handler comment block + the module docstring + the ``create_app`` block comments in both ``server.py`` and ``console/server.py``. - **Copilot #2** (``server.py:_interactive_manager_lookup`` docstring): referenced ``:data:SessionRouteHandlers`` which was renamed to ``SharedSessionVerbHandlers`` AND wasn't the right reference anyway — the callable matches ``SessionEndpointConfig.manager_lookup``. Fixed. - **Bonus**: dropped the now-dead ``app.state.session_endpoint_config = ...`` assignments in both servers (nothing reads them since the closure-capture switch). - **Bonus**: dropped the stale "close (interactive caps + redacts + persists close_reason)" entry from the deferred-verbs comment in ``session_routes.py`` — close was lifted in the previous commit and is no longer in the deferred set. - **CI lint**: ``ruff format`` joined the ``MockStorage.list_services`` signature in ``tests/_coord_test_helpers.py`` to a single line (95 chars, fits the 100-char limit). ruff + ruff format + mypy clean. 88 affected tests pass.	2026-04-24 16:56:06 -07:00
Patrick Buckley	74670cd53e	refactor(server): apply 2nd-pass /review fixups ... Addresses the eight verified findings from the second review pass on the body-convergence work (one bug-flagged behavior change, one defensive-style nit, six quality items). One quality item (q-6, ``request.scope[\"path_params\"]`` mutation in the legacy adapter) is documented but not refactored — restructuring the lifted handler signatures to take ``ws_id`` as an explicit param is bigger than this fixup's scope; the adapter docstring already explains the choice. Findings addressed: - **bug-1 + q-5**: hoist module-level ``log = get_logger(__name__)`` in ``session_routes.py``; bump audit-failure log from ``debug`` to ``warning`` (compliance signal). Document the interactive 500-on-audit-failure → 200+log behavior change in CHANGELOG + in ``make_close_handler``'s docstring. - **bug-2**: switch ``_audit_close_workstream`` to ``getattr(request.app.state, \"auth_storage\", None)`` for consistency with the upstream gate. Same fix on coord side. - **q-1**: pass ``SessionEndpointConfig`` into ``make_approve_handler(cfg)`` and ``make_close_handler(cfg, *, audit_emit, supports_close_reason)`` via closure capture. Removes the implicit ``app.state`` contract and parallels the two factory signatures. Tests + production wiring updated. - **q-2**: promote ``_audit_close_coordinator`` to a module-level function in ``turnstone/console/server.py``. Both test fixtures import it instead of duplicating the body. The previous three near-identical implementations collapse to one. - **q-3**: lift ``_interactive_tenant_check`` and ``_audit_close_workstream`` from nested ``create_app`` closures to module-level functions in ``turnstone/server.py``, beside the other ``_audit_*`` / ``_require_*`` helpers. Add ``_interactive_manager_lookup`` so the config doesn't need a lambda. ``create_app`` shrinks accordingly. - **q-4**: merge the bottom ``if TYPE_CHECKING`` block into the one at the top of ``session_routes.py``. - **q-7**: replace ``assert mgr is not None`` with ``mgr = cast(\"SessionManager\", mgr_opt)`` in both lifted handlers — survives ``python -O`` and makes the type-checker-only intent explicit. - **q-8**: update ``test_coordinator_endpoints.py`` file docstring to mention the lifted-handler wiring. ruff + mypy + 4366 pytest pass. Live console smoke against the unified URLs returns 503 (no coord_mgr in smoke env) — proves the factory-captured config is reachable + manager_lookup fires. CHANGELOG ``[Unreleased]`` entry expanded to flag the audit-failure swallow as an interactive behavior change alongside the existing 500→404 standardization.	2026-04-24 16:56:06 -07:00
Patrick Buckley	06c91294a4	refactor(server): lift close handler into shared session_routes body ... Stage 2 Priority 0 Step 0.2 body-convergence — second verb. ``make_close_handler(audit_emit=..., supports_close_reason=...)`` factory in ``turnstone/core/session_routes.py`` produces the lifted body; both interactive and coord pass their kind-specific audit emitter at app construction. The two body-keyed close URL aliases on the interactive side reach the same lifted body: - ``POST /v1/api/workstreams/{ws_id}/close`` (new, path-keyed) via ``register_session_routes(handlers.close=...)``. - ``POST /v1/api/workstreams/close`` (legacy, body-keyed) via ``make_legacy_body_keyed_adapter(close_handler)``. Coord exposes only the path-keyed shape. Behavior gains: - ``supports_close_reason=True`` (interactive only) keeps the 512- byte UTF-8 cap + credential redaction + ``workstream_config`` persistence path. Coord stays at ``False``; if coord ever wants close-reason metadata, flipping the flag is a one-line change. - ``audit_emit`` is per-kind so each owns its detail dict shape (``{kind, parent_ws_id, reason}`` vs ``{coord_ws_id, src}``) and audit action name (``workstream.closed`` vs ``coordinator.close``). - Standardizes the close-failure status code to 404 across both kinds. The coord code previously returned 500 on a ``mgr.close()`` race-loss, which was overly pessimistic — the semantic is "the ws was popped between .get() and .close()", i.e. not-found. Coord-side test fixtures (``test_coordinator_endpoints``, ``test_coordinator_end_to_end``) swap the imported ``coordinator_close`` for the lifted handler + a local audit_emit adapter so the tests exercise the same code path the live console does. ruff + mypy + 4366 pytest pass. Live console smoke against ``POST /v1/api/workstreams/abc/close`` returns 503 (no coord_mgr loaded in the smoke env) — proves the lifted handler is reachable + the manager_lookup callable fires correctly. Two verbs converged so far (``approve`` + ``close``); the remaining pairs (``send``, ``cancel``, ``open``, ``events``, ``create``, ``list``, ``saved``, ``history``, ``detail``) have substantive behavior divergence that doesn't factor cleanly into the SessionEndpointConfig + factory-handler pattern — see the session_routes module docstring for the per-verb status.	2026-04-24 16:56:06 -07:00
Patrick Buckley	6415eeb91e	refactor(server): lift approve handler into shared session_routes body ... Stage 2 Priority 0 Step 0.2 body-convergence — first verb. Both interactive ``approve`` and coord ``coordinator_approve`` handler bodies collapse into ``make_approve_handler()`` in ``turnstone/core/session_routes.py``. Each kind sets a ``SessionEndpointConfig`` on ``app.state`` carrying the kind- specific policies (auth gate, manager lookup, tenant check, audit prefix, not-found label) the lifted body consults at request time. The two interactive URLs converge: - ``POST /v1/api/workstreams/{ws_id}/approve`` (new, path-keyed) reaches the lifted body directly via ``register_session_routes``. - ``POST /v1/api/approve`` (legacy, body-keyed) keeps shipping; ``make_legacy_body_keyed_adapter`` peeks the body for ``ws_id``, splices it into ``request.path_params``, and forwards to the same lifted body. Frontend can keep using the legacy URL — no caller churn. Coord exposes only the path-keyed shape (its URLs were experimental in 1.5.0aN; the URL-shape commit already removed the ``coordinator/`` prefix). Tenant-check is split out from permission-gate so interactive's ``_require_ws_access`` (404 on cross-owner) and coord's ``_require_admin_coordinator`` (cluster-wide scope) coexist without either kind triggering the wrong gate. Coord-side test fixture (``test_coordinator_endpoints._make_client``) swaps the imported ``coordinator_approve`` for the lifted handler and seeds ``app.state.session_endpoint_config`` so the tests exercise the same code path the live console does. Net delta: ~−25 LOC for this verb on top of the SessionEndpointConfig + legacy-adapter scaffolding (~80 LOC paid once). Subsequent verb lifts amortize against that scaffolding. ruff + mypy + 4366 pytest pass. Live console smoke against the unified URL returns 503 (no coord_mgr loaded in the smoke env) — proves the lifted handler is reachable + the manager_lookup callable fires correctly. Verbs still kind-specific (deferred — bodies have substantive behavior divergence, not just naming): ``send`` (Priority 1 worker dispatch), ``cancel`` (interactive forensics + force flag), ``close`` (interactive close-reason cap+redact+persist), ``open`` (interactive resume vs coord rehydrate), ``events`` (different SSE replay shapes), ``create`` (interactive attachments vs coord initial_message), ``list`` / ``saved`` (different response keys).	2026-04-24 16:56:06 -07:00
Patrick Buckley	ae8ffd4bad	refactor(server): tighten registrar shape per code-review pass ... Addresses the eight quality findings the per-priority /review pass flagged on the registrar refactor. All confirmed by the verifier; none blocking. Net −186 LOC in this fixup. q-1, q-9: trim ``session_routes.py`` module docstring + console ``create_app`` comments to the timeless explanation. The Step 0.1 → 0.4 narrative was already stale within the PR that introduced it (every step had landed by the final commit) and would rot further as the body-convergence follow-on lands. q-2, q-5: group the four attachment handlers into an ``AttachmentHandlers`` dataclass exposed as ``handlers.attachments: AttachmentHandlers | None``. The type system now carries the all-or-none invariant; the parallel four-condition chain + bare ValueError disappear. q-3: drop the ``mgr`` and ``adapter`` placeholder kwargs from both ``register_session_routes`` and ``register_coord_verbs``. Pre- threading them so a future commit avoids "callsite churn" violated the project's "don't pre-build for the next step" norm — the body-convergence follow-on will edit the callsites anyway. Drops ``SessionManager.adapter`` for the same reason. q-4: drop ``SessionRouteConfig`` outright. It existed solely to carry ``supports_legacy_close``; the registrar now mounts the legacy close route whenever ``handlers.close_legacy is not None``, matching the all-Optional convention used for every other handler. q-6: move ``MockStorage`` from ``tests/test_console.py`` into the shared ``tests/_coord_test_helpers.py`` and re-import in test_console + test_session_routes. No more cross-test-module import. q-7: trim the two exhaustive route-table set-equality assertions (``test_coord_shape_mounts_expected_verbs``, ``test_register_coord_verbs_mounts_expected_paths``); replaced with focused ``test_attachment_routes_mount_when_quartet_provided`` and ``test_close_legacy_mounts_when_handler_provided``. The targeted ordering tests still catch the actual registrar bugs. q-8: delete the tombstone comment block where the legacy ``/api/coordinator/`` Routes used to be — per the user's ``feedback_no_tombstone_comments`` norm, deletions don't get narrated inline. q-10: rename ``SessionRouteHandlers`` → ``SharedSessionVerbHandlers`` and ``CoordVerbHandlers`` → ``CoordOnlyVerbHandlers`` so the "shared verbs vs coord-only verbs" symmetry is visible at the type names. Drop the back-compat aliases since nothing uses them.	2026-04-24 16:56:06 -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	377bd58b67	refactor(server): mount coord-only verbs through register_coord_verbs ... Stage 2 Priority 0 Step 0.3 — adds ``CoordVerbHandlers`` + ``register_coord_verbs`` to ``turnstone.core.session_routes`` and wires the seven coord-only verbs (``children`` / ``tasks`` / ``metrics`` / ``trust`` / ``restrict`` / ``stop_cascade`` / ``close_all_children``) through it on the console. These verbs are legitimately kind-specific — they read or mutate state (children registry, parent quota, trust / restrict policy, cascade controls) that doesn't exist on interactive workstreams — so they live on a Protocol distinct from ``SessionRouteHandlers``. The unified URL prefix ``/api/workstreams/{ws_id}/`` is shared with the session verbs; the separate registrar call keeps the kind separation explicit at the wiring site. Legacy ``/api/coordinator/{ws_id}/{verb}`` paths stay live during the transition; both URL shapes resolve to the same handler function. Step 0.4 deletes the legacy shape. The route-table walk in ``test_session_routes`` now covers all eighteen verb pairs (eleven session + seven coord-only) so a future drift between legacy and unified handlers fails CI.	2026-04-24 16:56:06 -07:00
Patrick Buckley	e1ee84af42	refactor(server): mount coord verbs through shared session route registrar ... Stage 2 Priority 0 Step 0.2 — extends ``register_session_routes`` to cover the per-``{ws_id}`` interaction verbs (``send`` / ``approve`` / ``plan`` / ``cancel`` / ``close`` / ``events`` / ``history`` / ``detail``) and wires ``console/server.py`` to mount coord at the unified ``/v1/api/workstreams/`` shape. The legacy ``/v1/api/coordinator/`` paths stay live during the transition; both URL shapes resolve to the same handler functions. Step 0.4 deletes the legacy shape outright once the frontend (Step 0.6) and tests (Step 0.7) move off it. Handler bodies still live in their server modules — body convergence (kind branching behind ``SessionRouteConfig`` flags) is the next Step 0.2 follow-on. Splitting "URL surface unified" from "handler bodies converged" keeps the soak-able diffs small. ``mgr`` and ``adapter`` registrar arguments are now Optional because the console builds its coord ``SessionManager`` inside the lifespan (after app construction). They become required again in the body-convergence follow-on once the lifted handlers read them. New ``tests/test_session_routes.py`` covers the registrar's mounting rules (route ordering, attachment-quartet enforcement, legacy-close gate) and asserts that each unified path on the console points at the SAME handler object as its legacy counterpart — the transition is a pure URL alias, not a fork.	2026-04-24 16:56:06 -07:00
Patrick Buckley	c837e3fa6d	feat(core): Stage 1 SessionManager unification (#408) ... * feat(core): scaffold SessionManager + SessionKindAdapter Protocol Stage 1 step 1 — pure addition, no production wiring. Defines the shape later steps will port the shared mechanics onto: slot accounting, per-ws-id refcounted rehydrate locks, kind-agnostic lifecycle; kind-specific event transport + session construction on the adapter. Pruned from the earlier Protocol draft (see design brief): per-kind permission_scope (static handler map is simpler), allows_child_spawn / quota_policy (deleted in #403), on_child_spawned (coordinator tool owns children registry), allows_active_focus / active_id / switch (frontend owns the active-tab state). * feat(core): port shared session-lifecycle mechanics onto SessionManager Stage 1 step 2. Adds create / open / close / set_state / close_idle / get / list_all / count on top of the Step 1 scaffolding. Pure addition — still no production wiring; the new class doesn't replace any call sites yet. Concurrency shape is ported from CoordinatorManager (the more- complete side): single-phase slot reservation under the manager lock, per-ws refcounted open-lock to serialize concurrent lazy rehydrate, placeholder workstreams count toward max_active but can't evict each other. WSM's two-phase eviction outside the lock is not carried over; it had a window where a burst of creates could silently exceed max_active. Deletions (vs. the union of the two old managers): - "refuse to close last workstream" guard — handled by the dashboard; only existed to protect the now-deleted default startup workstream. - active_id / switch / get_active — frontend owns focus; server-side duplicate state is gone. - _active_coords presence cache — defer measurement to Step 4; if it pays for itself at realistic cluster sizes, the CoordinatorAdapter can maintain it by observing emit_* calls. - Children registry + reverse index — coordinator tool owns this, manager stays kind-agnostic. Skill resolution (name → template_id + applied_version) is now shared via SessionManager._resolve_skill, so WSM's pre-resolve-at- callsite pattern and CM's internal-lookup pattern converge. Callers pass the skill name; the manager does the lookup once. 26 smoke tests cover create eviction + overflow, concurrent-create cap, persist/session rollback, open for missing/deleted/wrong- kind/wrong-user rows, concurrent-open serialization, close unblocks UI + emits closed, set_state + storage + adapter observer, close_idle, list_all ordering, count, eviction fires adapter transport, node_id passthrough. * feat(core): add InteractiveAdapter for SessionManager Stage 1 step 3. Adapter that bridges SessionManager to the node's interactive transport: - emit_created/state/closed → pushes onto the process-wide SSE global_queue (same shape current server.py handlers produce inline) - cleanup_ui → ports WorkstreamManager._cleanup_ui body: unblock _approval_event / _plan_event / _fg_event, broadcast ws_closed to per-UI listener queues (with full-queue fallback), cancel + close the session - build_ui/build_session → delegate to injected factories (ui_factory builds WebUI, session_factory is the existing closure from server.py with judge_model + memory_config captures) Also extends SessionKindAdapter.build_session with **extra passthrough so interactive callers can pass judge_model per-call without polluting the manager API; and adds a reason= kwarg to emit_closed so the frontend's "evicted" special-case keeps working (frontend doesn't differentiate "idle" from "closed", so close_idle collapses into close()). 14 new adapter tests cover wire payload shape, queue.Full tolerance, cleanup_ui event unblocking + listener broadcast + queue-full fallback, session cancel+close, graceful handling of stub UIs / None session, and kwarg passthrough to the session factory. * feat(console): add CoordinatorAdapter for SessionManager Stage 1 step 4. Coordinator-side SessionKindAdapter implementation: - emit_created/state/closed → delegate to the existing ClusterCollector.emit_console_ws_* methods (same wire shape the old CoordinatorManager emitted inline) - cleanup_ui → ports the listener-queue + approval/plan event unblocks from CoordinatorManager._cleanup, with queue-full fallback so an unresponsive browser tab can't wedge close - build_ui/build_session → delegate to injected factories; session factory doesn't accept client_type so we strip it at the adapter boundary Collector emission exceptions are swallowed (same policy as today's inline fan-out — dashboard lag on one tick is preferable to breaking the lifecycle path). Intentionally out of scope: the children registry (_children / _child_to_coord) stays in the coordinator tool when wired in Step 5; the _active_coords lock-free presence cache is deferred pending a measurement at realistic cluster sizes. 10 new tests cover transport payloads, collector-exception tolerance, cleanup_ui event unblock + listener broadcast + queue-full eviction, construction passthrough. * feat(server): wire interactive server.py to SessionManager Stage 1 step 5a. Production-path swap: WorkstreamManager → SessionManager(InteractiveAdapter(...)). - Construction at server startup: build the adapter with the process-wide global_queue, a WebUI ui_factory closure, and the existing session_factory. SessionManager gets storage + max_active. - Default startup workstream wiring removed (the CLI-REPL leftover flagged in the handoff's "Convergence is also a pruning opportunity" section). --resume now lazily creates a workstream scoped to the resumed content; no workstream at all if --resume isn't given. The dashboard handles the 0-ws state. - HTTP handler mgr.create() calls switched to the new kw-only signature (user_id, name, model, skill, ws_id, client_type, judge_model, parent_ws_id). ui_factory/skill_id/skill_version/kind no longer threaded through — adapter handles UI construction and manager resolves skill internally. - Dropped the mgr.last_evicted block in the /new handler (adapter emits ws_closed:evicted automatically on capacity eviction). - mgr.max_workstreams → mgr.max_active. - Added active_id / switch / switch_by_index / get_active / index_of / eviction_count to SessionManager because turnstone/cli.py uses them extensively; the handoff's "delete unless there's a live caller" rule flips here — CLI is a live caller. Test fixtures across 9 files updated to build SessionManager + InteractiveAdapter rather than WorkstreamManager. test_workstream.py stays unchanged (it tests WSM directly; it'll be deleted in step 5d alongside the class itself). Full pytest: 4528 passed. Ruff + mypy clean. Next: 5b (console-side wiring, with the children-registry relocation to the coordinator tool). * feat(console): wire console server to SessionManager Stage 1 step 5b. Production-path swap: CoordinatorManager → SessionManager(CoordinatorAdapter(...)). - CoordinatorAdapter now owns the coord-specific bits that were bolted onto the old CoordinatorManager: the children registry (forward + reverse index), the lock-free active-coords presence cache, the cluster-event fan-out thread, and the worker-dispatch path (send / _spawn_worker). The shared SessionManager stays kind-agnostic. - Added CoordinatorAdapter.attach(mgr) for late-binding the owning manager (the manager's ctor takes the adapter, so the dependency has to break here). Used inside _rebuild_children_registry for the tenant- filtered SQL query, inside send/dispatch for mgr.get(ws_id), and inside the fan-out seed path for mgr.list_all(). - emit_created now seeds the children registry + active-coords slot AND calls _rebuild_children_registry (covers both create — empty query — and open/rehydrate, where the subtree is persisted). emit_closed drops both entries. Collapses the three old call-sites in CoordinatorManager's create/open/close into one per-event hook. - Console server.py builds the manager via: coord_adapter = CoordinatorAdapter(collector=..., ...) coord_mgr = SessionManager(coord_adapter, storage=..., max_active=..., node_id=ClusterCollector.CONSOLE_PSEUDO_NODE_ID) coord_adapter.attach(coord_mgr) ConsoleCoordinatorUI._coord_mgr = coord_mgr app.state.coord_adapter = coord_adapter - HTTP handler call-site updates: - coord_mgr.create drops initial_message; the handler now calls coord_adapter.send(ws.id, initial_message) after create so the worker spawn stays out of the shared manager. - coord_mgr.open_admin(ws_id) → coord_mgr.open(ws_id, user_id="", admin=True). Matches SessionManager.open's unified signature. - coord_mgr.list_for_user(uid) inlined as a list comp on list_all() (SessionManager doesn't expose the filter; two callers). - coord_mgr.children_snapshot / send → coord_adapter.*. - coord_mgr.cancel stays (now lives on SessionManager from 5a). - ConsoleCoordinatorUI.on_state_change now flows state transitions through ConsoleCoordinatorUI._coord_mgr.set_state, mirroring the WebUI pattern. The old _on_state_observer / _on_rename_observer closures the manager used to install are dead code now; leaving the fields in place for 5d cleanup. - Lifespan shutdown calls coord_adapter.shutdown() (was coord_mgr. shutdown()) and resets ConsoleCoordinatorUI._coord_mgr on teardown. Test fixture updates in _coord_test_helpers, test_coordinator_end_to_end, test_coordinator_endpoints, test_phase6_endpoints: build SessionManager + CoordinatorAdapter in _build_mgr, set app.state.coord_adapter, switch mgr.register_children / mgr.children_snapshot tests to mgr._adapter.*, and rewrite test_open_admin_uses_open_admin to assert the unified open(user_id="", admin=True) call shape. Full pytest: 4486 passed. Ruff + mypy clean. Next: 5d (remove CoordinatorManager + WorkstreamManager class bodies and their test files). * feat(core): delete WorkstreamManager + CoordinatorManager classes Stage 1 step 5c + 5d. Final step of the unification — the legacy classes and their test files go away now that every production caller has been ported. - Delete turnstone/console/coordinator.py entirely (CoordinatorManager class + the _enqueue_on_ui helper, which CoordinatorAdapter now hosts its own copy of). - Trim turnstone/core/workstream.py to just the Workstream dataclass + WorkstreamKind + WorkstreamState. ~385 lines of WorkstreamManager logic gone; the remaining shape is pure data types shared by both managers. - Delete tests/test_workstream.py (WSM-specific) and tests/test_coordinator_manager.py (CM-specific). - Wire turnstone/cli.py to SessionManager + InteractiveAdapter, same pattern as turnstone/server.py. The CLI's WorkstreamTerminalUI uses manager.set_state + manager.active_id — both preserved on SessionManager (CLI is a live caller that keeps the focus API honest, per the handoff's "delete unless it pulls its weight" rule). - Add an optional manager-level ``_on_state_change`` observer hook restored for the CLI's background-attention notification (the web path uses the adapter's emit_state; this hook covers callers that don't consume SSE). - Drop dead ``_on_state_observer`` / ``_on_rename_observer`` fields from ConsoleCoordinatorUI — the old CoordinatorManager installed them; SessionManager/CoordinatorAdapter handle fan-out directly. Vulture @ 80% confidence: zero unused symbols across the new SessionManager + adapter files. Ruff + mypy clean (170 files). Full pytest (excluding tests/live): 4414 passed. Net across the whole Stage 1 branch: one unified SessionManager + adapter Protocol replaces two ~500-line parallel managers + a ~600-line CoordinatorManager, and the interactive + coordinator transports stay cleanly separated at the adapter boundary. * refactor(auth): drop workstream row-level ownership gates Turnstone is a trusted-team tool (per #400). user_id stays as metadata for audit + display; it no longer rejects requests. Scope- level auth via admin.workstreams / admin.coordinator tokens is the only gate now. Solves sec-1 (cross-tenant delete via collision on caller-supplied ws_id, because the gate was half-implemented) and sec-2 (blank-sub JWT bypass on empty-owner rows). Net: 359 lines of defensive empty-string comparisons and admin=True bypass plumbing deleted. * fix(core): serialize set_state vs close + worker spawn Three concurrency fixes from the multi-stage review: - bug-3: set_state now looks up ws under self._lock and gates its storage write on ws._closed (a new tombstone flag). close() sets ws._closed=True and does its storage write under ws._lock. A set_state that acquires ws._lock after close sees the tombstone and skips its write instead of resurrecting the closed row. - bug-1: _spawn_worker wraps the check-and-spawn in ws._lock so two concurrent send() HTTP requests can't both observe "no live worker" and start duplicate worker threads on the same ChatSession. - bug-2: replaces Thread.is_alive() as the reuse gate with an explicit ws._worker_running flag. The flag is set before the worker thread starts and cleared in its finally block — both under ws._lock. Using is_alive() left a narrow window where the worker could exit between the check and a queue_message call, stranding the user's message with no consumer. perf-2 (lock-held-across-DB-write) is accepted as-is: per-ws serialization of state transitions behind a DB round-trip is real cost but bounded — a given ws's state flips happen sequentially on its worker thread anyway. Dropping ws._lock around the DB write would reintroduce the bug-3 race. Full pytest: 4401 passed. Ruff + mypy clean. * refactor(core): drop _resolve_skill from SessionManager Skill resolution (name → template_id + applied_version) moves out of the shared manager and back to the HTTP handlers that own the create request. The interactive handler already resolved skill_data + applied_skill_version for other purposes (model override, judge config, post-create session seed) and was passing the name to SessionManager which then redundantly re-resolved via get_skill_by_name + count_skill_versions — two wasted DB round-trips per create on a user-visible latency path. - SessionManager.create: accepts skill_id + skill_version as already-resolved kwargs; _resolve_skill helper deleted. - turnstone/server.py create_workstream: passes the skill_id / applied_skill_version it already computed. - turnstone/console/server.py coordinator_create: pre-resolves inline (parity with interactive) before calling coord_mgr.create. Fixes perf-1 (redundant skill queries per create), q-4 (divergent skill-version computation between manager and handler), q-5 (coordinator-specific lookup on the shared manager surface). Full pytest: 4401 passed. Ruff + mypy clean. * refactor(adapters): extract shared cleanup_ui + drop dead child-registry methods Both InteractiveAdapter.cleanup_ui and CoordinatorAdapter.cleanup_ui (plus their _broadcast_ws_closed_to_listeners helpers) were byte-identical. Pull them into turnstone/core/adapters/_ui_cleanup.py:cleanup_session_ui so the two adapters delegate to one implementation. Also drop CoordinatorAdapter.register_children (only test callers — now use _seed_children in tests/_coord_test_helpers.py) and _add_child (zero callers anywhere). * refactor(adapters): symmetric attach() + fail-loud on unattached manager Add InteractiveAdapter.attach(manager) + .manager property mirroring the coord-side pattern. CLI (cli.py) now uses cli_adapter.attach(manager) instead of the _mgr_ref list-ref late-binding hack; server.py picks up the same call for consistency. CoordinatorAdapter.send / _rebuild_children_registry / _prime_children_from_snapshot no longer silently return when self._manager is None — raise RuntimeError so a forgotten attach() at startup fails loud instead of dropping the whole fan-out. * docs: replace stale WorkstreamManager / CoordinatorManager references Both classes were deleted in 965e0b6; prose docstrings across the codebase still named them. Update to SessionManager (or describe the collapsed-into-one-class architecture where the distinction matters). Leaves the 'Ported from …' historical markers in session_manager.py / coordinator_adapter.py / interactive_adapter.py intact — those are deliberate pointers back to the pre-unification code. * fix(core): atomic close_if_idle + batch pop under one lock bug-5: SessionManager.close_idle re-checked ws.state == IDLE outside the lock, so a pending tool result could flip state IDLE→RUNNING between the snapshot and close() acquiring self._lock. Add _close_if_idle_locked that tests state + pops under self._lock. perf-5: drop the per-victim self._lock acquisition; collect + pop the whole batch in one acquisition, then run cleanup_ui / storage write / emit_closed outside the lock. * perf(coord): split emit_created / emit_rehydrated to skip storage query on fresh creates CoordinatorAdapter.emit_created was unconditionally calling _rebuild_children_registry (storage.list_workstreams with parent_ws_id=... limit=10001) on every create, even for fresh-create paths that provably have zero children. Add emit_rehydrated to the SessionKindAdapter Protocol. SessionManager .create still calls emit_created; .open (lazy rehydrate) now calls emit_rehydrated. CoordinatorAdapter.emit_created seeds the registry + fan-out but skips the rebuild; emit_rehydrated seeds + rebuilds + fans out. InteractiveAdapter.emit_rehydrated delegates to emit_created (no children-registry on the interactive transport). * perf(coord): fold _active_coords into _children_lock + mutate payload in place perf-4: _active_coords used a copy-on-write dict-swap pattern so the fan-out dispatch could read it lock-free, but _dispatch_child_event already re-validates the parent under _children_lock anyway — the lock-free snapshot was premature. Replace with a plain dict read+write both under _children_lock; install and remove collapse to one-liners. Value also drops the user_id half — dead after a46dab1 removed row-level ownership gates — so _active_coords is now just coord_ws_id → ui. perf-6: _enqueue_on_ui was doing {**payload, "ws_id": coord_ws_id} on every dispatch. The dispatch path owns payload and doesn't reuse it — mutate in place. * test(coord): add adapter tests for worker dispatch + children registry + fan-out Fills the coverage gap on CoordinatorAdapter — the review (q-3) flagged the coord-specific concurrency paths ported from the deleted CoordinatorManager as untested. Three new test classes: - TestCoordinatorAdapterWorkerDispatch: _spawn_worker reuse gate, queue.Full backpressure, concurrent-call bug-1 reproducer (two threads → exactly one worker via ws._lock + _worker_running), finally-clears-flag. - TestCoordinatorAdapterChildrenRegistry: registry seed on emit_created vs emit_rehydrated rebuild, _pop_coord_registry_locked reverse-index cleanup, _merge_child_ids_locked idempotency, _prime_children_from_snapshot merge. - TestCoordinatorAdapterDispatchChildEvent: unknown-parent drop, ws_created fan-out, cluster_state / ws_closed reverse-index routing, perf-6 in-place ws_id stamp. * fix: regressions flagged by ultrareview Verify stage of the cloud review surfaced 6 confirmed regressions from Stage 1's adapter layer. Fixing together since they share the same root cause (plumbing moved into adapters without retiring the old emission paths). - Interactive adapter emit_created / emit_state / emit_rehydrated become no-ops. The create_workstream HTTP handler still fires ws_created (after attachment validation, per the pre-Stage-1 "no phantom events on rejected upload" contract); WebUI _broadcast_state still fires ws_state with the full payload (tokens + context_ratio + activity). Firing from the adapter too was duplicating both events. Also closes the phantom-ws-created regression (adapter fired before attachment validation ran). - emit_closed Protocol gains a ``name`` kwarg; the adapter is the sole emitter for ws_closed on interactive now, and the frontend eviction toast needs the name. Manager passes ws.name from close() / create()+open() eviction / close_idle paths. - _idle_cleanup_thread stops firing its own reason="idle" ws_closed — close_idle already fires via the adapter with reason="closed", and the frontend never differentiated the two anyway. - close_workstream_endpoint fix: "Cannot close last workstream" 400 was a stale error (the guard went away with the default-startup workstream). Return 404 on close() == False (which now means the ws was already closed or unknown). Also switches the audit actor from _require_ws_access's stored owner to _auth_user_id — the stored owner is metadata post-#400, so attributing actions to it misrepresents who actually did them. - CLI /ws close mirrors the same stale-error fix. - SessionManager.close now calls storage.delete_workstream_override alongside update_workstream_state, same as the old WorkstreamManager.close did. Without it overrides leak until tombstone cleanup. close_idle does the same. - SessionManager._reserve_and_install_locked records the eviction on turnstone.core.metrics so the global eviction counter keeps working. Old WSM did this inline; the unification dropped it. - ConsoleCoordinatorUI.on_rename now fans out to the cluster collector via a new class attribute ``_collector`` (set at console startup alongside ``_coord_mgr``). The old ``_on_rename_observer`` plumbing went away with CoordinatorManager and the "adapter emit_console_ws_rename runs from whichever code path renames" comment was aspirational — nothing actually did it. Full pytest: 4375 passed (tests/live + test_server_live.py excluded; both pre-existing live-backend failures unrelated to this branch). Ruff + mypy clean. * refactor(ui): extract SessionUIBase for shared UI scaffolding Direct response to review feedback that the unification wasn't merging enough of the two workstream kinds. WebUI (node) and ConsoleCoordinatorUI (console) both: - Keep a per-UI list of SSE listener queues guarded by a lock - Block a worker thread on _approval_event / _plan_event - Fan enqueued events out with the same ws_id-stamping pattern - Resolve approvals / plans with the same broadcast-then-signal pattern All of that now lives once in turnstone/core/session_ui_base.py. Both UIs subclass SessionUIBase; kind-specific bodies (WebUI's per-UI metrics + _broadcast_state + intent-verdict bookkeeping, ConsoleCoordinatorUI's collector fan-out) stay in the subclasses. WebUI.resolve_approval still overrides the base (it adds intent- verdict updates) but now calls super() for the shared broadcast + event-set steps. Same shape as the other approval/plan hooks: subclasses extend, base provides skeleton. Net file-level: +156 LOC for the base, -144 LOC across the two subclasses. The raw number is unexciting — but there's now a single source of truth for the listener + blocking-gate machinery, and bugs (like the duplicate ws_created / ws_state events that prompted this refactor) can't arise from the two implementations drifting. Full pytest: 4375 passed. Ruff + mypy clean. * refactor(ui): move metrics + verdict bookkeeping into SessionUIBase Second pass at unifying the two UIs. Per-workstream metrics accumulators (token counts, tool-call counts, context ratio, activity tracking), intent-judge verdict cache + pending-decision list, and the verdict-persistence path all move to SessionUIBase. Before: WebUI tracked all of it; ConsoleCoordinatorUI tracked none of it (a comment on the old on_intent_verdict literally admitted the deferral — "skip the persistence + late-decision plumbing that WebUI does"). Coord sessions never got verdict rows in storage, never had a user_decision stamped, and the dashboard had no way to show coord token usage because the data wasn't captured. Now the base class captures the data and persists the rows for every kind. Kind-specific broadcast (WebUI's _broadcast_state with rich per-UI payloads) stays on WebUI; prometheus counters on the node (_metrics.record_judge_verdict) stay on WebUI's on_intent_verdict override. Everything else shared. Behaviour change worth flagging: coord sessions now write intent_verdicts and output_assessments rows for every judge call and every output-guard warning. Previously silent; the storage rows now exist and any future coord-dashboard surface can read them. Shape of the unification: - resolve_approval: was overridden on WebUI (intent-verdict decision propagation); now lives on the base. Both kinds inherit unchanged. - on_intent_verdict: WebUI overrides only to add _metrics.record_*; rest of the body is the base. - on_output_warning: was on both separately; fully base-shared now. Full pytest: 4375 passed. Ruff + mypy clean. * fix: regressions flagged by second-pass review Three confirmed findings with direct fixes + a dedicated test file for SessionUIBase (was previously uncovered). bug-1 — Coord approve_tools didn't reset _last_verdict_decision or clear _llm_verdicts between approval rounds. WebUI did (inline). Coord inherited SessionUIBase.on_intent_verdict which stamps via the decision flag, so after the first resolve every subsequent round's verdicts were stamped with the prior round's user_decision before the user had decided the new round. Fix: add SessionUIBase._reset_approval_cycle() clearing both under _ws_lock; call from the top of both subclass approve_tools methods. Single-source invariant — can't drift again. sec-1, sec-2 — delete_workstream_endpoint and open_workstream's rehydrate path recorded the audit row under the stored ws.user_id ("owner_uid") rather than the authenticated caller. With row-level ownership gating gone (a46dab1), any team member acting on a peer's workstream produced an audit row naming the victim as the actor. Fix: pass _auth_user_id(request) as the audit actor, matching the pattern close_workstream already follows. q-2 — SessionUIBase had no direct tests. The new tests/test_session_ui_base.py covers listener fan-out, approval + plan blocking gates, intent-verdict cache + FIFO eviction, verdict persistence paths, output-guard persistence, the reset-between-rounds invariant (bug-1 regression test), a cross-subclass test that verifies BOTH WebUI.approve_tools and ConsoleCoordinatorUI.approve_tools call _reset_approval_cycle (verified it fails without the fix), and a concurrent enqueue/register smoke. Full pytest: 4395 passed (+20 new). Ruff + mypy clean. * fix: PR #408 review findings from copilot + code-quality Three substantive fixes + mechanical side-effect-in-assert cleanup. Copilot findings: - session_ui_base.py: on_intent_verdict had a race with resolve_approval. Previously acquired _ws_lock twice (read decision → release → if unset, acquire again to append). resolve_approval could interleave between the two acquisitions, swap-and-clear the pending list and set the decision — our verdict then got appended to the fresh (empty) list and stamped with the NEXT round's decision on the following resolve. Fix: decision-check + append under ONE acquisition; storage UPDATE (if decision already set) runs outside the lock. New regression test counts lock acquisitions during on_intent_verdict and fails if the two-phase pattern returns. - server.py close_workstream_endpoint: comment said "treat as already-closed success" but handler returned 404. Comment rewritten to match the 404 behaviour ("the ws isn't tracked here" is the only reachable meaning for close() → False now). - test_session_ui_base.py concurrency smoke: the test ended with ``pytest.assume = lambda ...`` — a leftover that mutates pytest globals and can surprise other tests. Replaced with explicit ``not is_alive()`` assertions so the "threads completed cleanly" intent survives -O optimization stripping. Code-quality (assert side-effects): Six ``assert mgr.open(...)`` / ``assert mgr.close(...)`` in test_session_manager.py stripped under ``python -O``. Mechanical fix: extract to local before asserting. Ignored the two "Protocol method body is `...`" flags — that's the standard Protocol idiom; replacing with ``pass`` or ``NotImplementedError`` changes typing semantics. Full pytest: 4396 passed.	2026-04-24 14:28:51 -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	4fe6e8678e	fix(server): trusted-team workstream visibility on listing endpoints (#400) ... * fix(server): trusted-team workstream visibility on listing endpoints The per-user filter on /v1/api/workstreams, /v1/api/dashboard, and /v1/api/workstreams/saved (PR #375's _visible_workstreams helper) was written for a multi-tenant SaaS threat model that doesn't match how turnstone gets deployed. In a self-hosted, trusted-team install the filter created friction without preventing the relevant threats — and hid the auto-created name="default" startup workstream from every web user, leaving fresh installs staring at a blank dashboard. Listing endpoints now return the cluster-wide set to any authenticated caller. Per-workstream MUTATIONS (/send, /close, /open, /title, /delete, /refresh-title) keep their independent ownership checks — the cross-tenant guards from PR #375 stay in force on those handlers (see TestCrossTenant{Delete,Approve,Close,Title,Open}). Listing only exposes metadata (name, state, kind, message_count); message history still requires the per-workstream gate on /history. Resuming a saved workstream still goes through /open's owner check, so the metadata-leak surface ends at "you can see workstream X exists" — not at any actionable cross-user capability. The console collector's service-scope is now load-bearing only for the SSE event stream gate (/v1/api/events/global); kept anyway as belt- and-braces. If turnstone is ever deployed as a true multi-tenant SaaS, the right boundary is a real ``tenant_id`` column with row-level filtering at the storage layer, not the empty-user_id heuristic this used to apply. Tests updated to assert the new contract: listing returns all owners; mutation gates unchanged. * fix(server): repair test mocks + tighten docstrings on listing endpoints - tests/test_auth.py: TestServerAuth + TestServerLogin mocks now set kind / parent_ws_id / user_id explicitly so /v1/api/workstreams JSON- serializes them. Bare MagicMock attributes return another MagicMock that fails json.dumps and surfaces as 500. - turnstone/server.py: list_saved_workstreams docstring corrected to describe what the endpoint actually returns (summary metadata, not history) and to spell out that ownerless persisted rows are claimable by any authenticated caller via /open — consistent with the trusted- team model the listing endpoints assume. Same callout added next to the open_workstream ownership-gate block. Comments throughout rewritten to be timeless (no "previously" / PR-number references). - tests/test_server_authz.py: TestSaved... docstring matches the actual /open behavior for orphan rows (claimable by any authenticated caller, not a separate admin path).	2026-04-23 18:44:46 -07:00
Patrick Buckley	436ce5630b	feat(coord): saved coordinators surface + shared session-card primitives (#399) ... * feat(coord): saved coordinators surface + shared session-card primitives The console home view now lists explicitly-closed coordinators in a "Saved Coordinators" card grid below the active list. Click a card → POST /v1/api/coordinator/{ws_id}/open then navigate; capacity issues surface as a toast instead of a broken detail page. Card click is de-duped by an `is-busy` class so rapid double-clicks don't fire parallel resurrects. GET /v1/api/coordinator/saved is the new backend endpoint (mirrors the interactive list_saved_workstreams shape). Filters at the SQL layer to state='closed' via a new optional `state` parameter on list_workstreams_with_history (added to the protocol + both backends); also drops any rows currently loaded into coord_mgr as defence in depth. The blocking storage call + the lock-acquiring list_all are offloaded via asyncio.to_thread to match coordinator_create's pattern. CoordinatorManager._open_impl now allows resurrect of state='closed' rows (deleted is still a tombstone). The DB state-flip on resurrect that the first cut had is gone — it raced concurrent close()s and the next set_state() call syncs the DB naturally; the saved list filters already keep a still-loaded coordinator from appearing as a saved card even when its on-disk state lags. Frontend dedup that paid for the saved surface ships in the same diff: - shared_static/cards.css: lifted from ui/static/style.css so both surfaces share the basic card primitive (delete-mode rules stay interactive-only until coordinator gets the same UX) - shared_static/cards.js: new renderSessionCard(sess, opts) helper used by both renderSavedWorkstreams (interactive) and renderSavedCoordinators (console) - shared_static/utils.js: formatRelativeTime moved here from ui/static/app.js Coordinator landing visual fixes folded in: - .home-section-title now uses var(--accent) so the COORDINATORS heading reads as a peer of the NODES heading - .home-panel dropped its bg/border/padding so the composer is no longer double-framed (matching the dashboard-composer feel) - "Active coordinators" → "Saved Coordinators" rename + "Coordinators" on the active list ws_closed SSE handler now gates on the closed ws's kind so interactive closes don't spam /v1/api/coordinator/saved on busy clusters. loadSavedCoordinators in-flight de-dup coalesces close-event bursts to one fetch instead of N. Tests cover: caller-scoping, admin sees-all, blank-uid fail-closed, loaded-coordinator filtering, state filter (idle rows excluded), plus the manager-level open-resurrect / open-refuses-deleted contracts. Closes the bug-{1,2,3}, perf-{1,2,3,4}, sec-{1,2}, q-{1,2,3,4,5,6,7} findings from the prior multi-stage review. * fix(design): restore amber accent on the v1 design system The Claude Design handoff swapped the accent hue to teal (h=182). Walking back to amber (h=75) — turnstone's original brand colour. Lightness + chroma bumped slightly (0.62→0.7, 0.10→0.13) so the restored gold matches the visual weight of the legacy #e5a042 token. Hue map header comment updated to record what happened so the next person doesn't repeat the swap. Only surfaces with data-design="v1" on <html> pick this up — currently just turnstone-server's webui. * chore: gitignore design_ideas/ and .claude/ dev directories design_ideas/ holds personal Claude Design handoff scratch + reference HTML; .claude/ holds per-user Claude Code state (worktrees, settings, plugin caches). Neither belongs in version control. * fix(coord): address PR #399 review nits - tests/test_coordinator_endpoints.py: split `assert mgr.close(ws.id)` in `_seed_closed_coord_with_history` so the close call always runs even under `python -O` (asserts stripped). Same fix in test_coordinator_manager.py's `test_open_refuses_deleted_coordinator` for the open() and open_admin() calls. - shared_static/cards.css: `.card-wsid` now reads `var(--font-mono, "IBM Plex Mono", monospace)` so design-v1 surfaces pick up the JetBrains Mono token while console (still pre-v1) keeps the literal fallback.	2026-04-23 17:48:19 -07:00
Patrick Buckley	f510699a4f	feat(auth): inline refresh response + sessionStorage rehydrate hardening (#398) ... * feat(auth): inline refresh response + sessionStorage rehydrate hardening The proactive refresh path now consumes the /refresh response body inline (permissions + exp), eliminating the chained /whoami round-trip and the brief stale-sessionStorage window after refresh succeeds but before whoami completes. Adds AbortController + _loggedOut guards to the whoami fetch so a logout fired mid-flight cannot re-populate sessionStorage after it clears. A non-OK whoami on tab restore now explicitly clears sessionStorage instead of silently leaving stale cosmetic permissions (server-side identity gone → UI gating reflects it on next render). Surfaces window.permissionsReady (one-shot promise) so permission- gated UI can await the initial whoami's completion instead of guessing a setTimeout duration. Tests cover the new refresh response shape, the existing leeway path, the storage-failure fallback, and the no-perms 403 path. Closes the bug-3 / perf-4 / sec-1 / q-6 findings from the multi-stage review of the prior uncommitted change set. * fix(auth): guard whoami superseding race in _scheduleRefreshFromWhoami _scheduleRefreshFromWhoami is invoked from several entry points (initial page load, _onSuccess, BroadcastChannel "login"/"refresh", _tryRefresh fallback). Two firing in quick succession could let an older slow whoami land after a newer one and clobber its effects — clearing permissions right after a successful login, or rescheduling the refresh timer off stale exp. Now aborts any prior _whoamiAbort before starting a new request and guards the .then's _storePermissions / _scheduleRefreshAt with a `_whoamiAbort === ctrl` check so a late arrival from a superseded call is fully neutralised. Addresses Copilot review feedback on PR #398.	2026-04-23 17:43:48 -07:00
Patrick Buckley	fa53b414ed	feat(providers): add gpt-5.5 and gpt-5.5-pro capability entries (#396) ... * feat(providers): add gpt-5.5 and gpt-5.5-pro capability entries OpenAI announced gpt-5.5 on 2026-04-23 (ChatGPT/Codex first, API "very soon"). Mirror the gpt-5.4 / 5.4-pro capability shape: 1M context, native tool search, vision, xhigh effort; pro is always-reasoning with no temperature and medium/high/xhigh only. No provider-logic changes needed — OpenAI announced no API-surface changes vs 5.4. Cache retention already covers 5.5 via the existing startswith("gpt-5") prefix rule. * test(providers): cover gpt-5.4-pro and gpt-5.5-pro in cache retention test Pro variants share the same gpt-5 prefix and should keep 24h retention; explicit coverage guards against regressions if the prefix rule narrows in the future.	2026-04-23 15:20:59 -07:00
Patrick Buckley	42d22bb6b4	feat(auth): cookie refresh endpoint, JWT leeway, coord-token observability (#395) ... * feat(auth): cookie refresh endpoint, JWT leeway, coord-token observability Three robustness wins around the auth/JWT layer. 1. POST /v1/api/auth/refresh — handle_auth_refresh in core/auth.py, wired in both console/server.py and server.py. Sliding-window re-mint of the auth cookie. Re-resolves the user's permissions from storage so a role change propagates within one refresh cycle instead of persisting until the original cookie's natural expiry. Returns the same JSON shape as /api/auth/login plus a fresh Set-Cookie header. Refuses to extend a session for a deleted / role-stripped user (403). Resolves the user-visible "401 after browser tab open >24h" symptom: previously the only refresh path was a full re-login, now a single POST extends the session. 2. validate_jwt now passes leeway=30 to PyJWT. Absorbs minor clock skew between hosts (multi-replica console deployments) and between mint-time and validate-time within the same process. Standard tolerance for short-lived tokens. 3. CoordinatorTokenManager._mint logs at debug. Mirrors the pattern in ServiceTokenManager._mint (auth.py). Premature-401 diagnostics would have been an order of magnitude faster with this in place the first time around. Frontend (shared_static/auth.js): - _scheduleRefreshFromWhoami() reads the JWT exp surfaced via /whoami and sets a setTimeout at 90% of remaining cookie life to call /refresh. Floor 30s, ceiling 24h. Fires on initial page load (silent if not authenticated) and after every successful login. - _tryRefresh() de-dupes concurrent callers via a shared in-flight promise — many parallel authFetch's hitting 401 at once still only fire one /refresh. - authFetch on-401 now attempts a single reactive refresh-then-retry before falling through to the login overlay. Covers cases where the proactive timer didn't fire (tab restored from disk-cache after expiry, system clock jump, page first-load with stale cookie). - BroadcastChannel "refresh" message keeps sibling tabs in sync so they don't redundantly hit /refresh themselves. - logout() cancels the proactive timer. Tests: - validate_jwt accepts 10s-expired tokens (within 30s leeway). - validate_jwt rejects 60s-expired tokens (past leeway). - /whoami includes exp claim with sane bounds. - /refresh returns ok + Set-Cookie + the refreshed cookie keeps working on subsequent authenticated requests. - /refresh without a cookie returns 401. Not addressed: the coordinator.session_jwt_ttl_seconds ceiling (currently 1h) — that's a separate, preventative concern for very- quiet long-running coordinators, orthogonal to the user-visible 401 this PR fixes. Can bump in a follow-up if it actually surfaces. * fix(auth): address Copilot PR #395 feedback Two real bugs caught by Copilot, both fixed. 1. Storage failure was indistinguishable from "user deleted" in handle_auth_refresh. _load_user_permissions() swallows exceptions and returns set(), so a transient DB hiccup looked like "user has no permissions" and returned 403 — logging the user out. Now calls storage.get_user_permissions() directly with try/except. - Exception → log + fall through to in-token claims (refresh succeeds with stale-but-valid permissions; better than fail-closed mid- session for a hiccup). - Empty set returned (no exception) → 403 (legitimate signal: user deleted or role-stripped). Tests: - test_refresh_storage_failure_falls_back: storage raises → 200 + in-token permissions. - test_refresh_user_with_no_perms_403: storage returns empty → 403. 2. Logout race: a /refresh in flight when the user clicks Logout could land AFTER /logout's clear-cookie response and re-set the cookie from /refresh's Set-Cookie header, silently undoing the logout. Fix in shared_static/auth.js: - Add a _loggedOut latch + _refreshAbort AbortController. - logout() sets _loggedOut = true synchronously and aborts any in-flight /refresh BEFORE the /logout fetch fires. - _tryRefresh() bails on its post-fetch effects (don't store perms, don't reschedule, don't broadcast) when _loggedOut is set. The stale Set-Cookie from /refresh is harmless because /logout's response overwrites it on the way back. - _onSuccess() (re-login) clears the latch so subsequent refreshes work again. Race window is small but real on slow networks / contested CPU.	2026-04-22 20:36:00 -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	7c16b0dfa8	refactor(routing): replace hash-ring rebalancer with rendezvous (HRW)… (#384) ... * refactor(routing): replace hash-ring rebalancer with rendezvous (HRW) hashing Routing was a stored bucket table maintained by a central rebalancer daemon, which shared its liveness primitive (services.last_heartbeat) with the collector — when a heartbeat-fresh node went into a zombie HTTP-handler-broken state, neither the collector nor the rebalancer could self-correct, and the router kept directing traffic at it. Rendezvous hashing makes the route a pure function of (ws_id, live_services) so the heartbeat is the single source of truth and any liveness-eviction propagates to the next route call without a separate state-publication step. The rebalancer's central state has no analogue: the new router computes the per-key node winner on every call, the collector pushes membership updates into the router cache from its discovery thread, and per-route overrides survive on workstream_overrides. Eager workstream migration goes away; in-flight workstreams lazily rehydrate from storage on the new owner — already the dead-node behaviour. * fix(tools): describe rendezvous re-routing on spawn/inspect node_id The first pass overclaimed `node_id` "stays canonical for this workstream's lifetime" — under rendezvous routing the active owner re-derives per-call from live membership, so a node join/drop after spawn can shift it. Tool descriptions now say `node_id` is the spawn-time binding; subsequent ops re-route via rendezvous over the current live-node set; the new owner lazily rehydrates from shared storage; coordinators should re-read with inspect_workstream rather than caching the value.	2026-04-18 19:02:52 -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	cab57f244d	refactor(channels): backfill review of Slack/Discord adapters (#382) ... * refactor(channels): backfill review of Slack/Discord adapters Retrospective multi-stage review of the Slack (PR #355) and Discord channel adapters — they shipped before the review pipeline existed, so this pass goes back and fixes everything the pipeline would have caught plus a follow-up round of ultrareview findings. ## Security (8 fixes) - Adapter-side owner checks on all interactive flows: Discord ApprovalView / PlanReviewView encode the owner Discord user ID in the embed footer (`{ws_id}|{corr_id}|{owner_id}`) and reject non-owner clicks; Slack plan-approve / request-changes / feedback-modal gain owner tracking in `_pending_plan_review_ts` and a shared `_ensure_plan_review_owner` gate. These closed the two critical authz gaps where the gateway's service-scoped JWT bypassed server-side ownership checks. - Discord thread-message gate: only the registered invoker can drive the workstream (prevents a linked user posting in another user's public thread from injecting into their assistant). Invoker recorded explicitly so `/ask` follow-ups survive the `channel.create_thread` bot-as-owner quirk. - Slack /link flow + per-user identity gate: unlinked Slack users see an ephemeral `/turnstone link <token>` prompt on every message instead of silently creating workstreams under the shared gateway identity. Rate-limited (5/hour) to block online token enumeration. - Gateway `/v1/api/notify` requires `write` scope on the validated JWT; low-scope tokens get 403 + audit. - Thumbnail URL validator DNS-resolves the hostname before fetch and rejects any resolved IP that's loopback / link-local / multicast / reserved, plus an explicit deny-list for IPv6 cloud metadata (`fd00:ec2::/32` — AWS Nitro IMDS + ECS task metadata) that would otherwise slip past the `is_private` allowance. - Per-user rate limit (10 msgs / 60s) + 8 KiB inbound size cap on Slack DMs / channels / notification-reply threads so one user can't exhaust the shared LLM budget. - Discord /link rate limit (5/hour) for token-enumeration defense. ## Bug fixes (9 correctness issues) - Slack DM routing: each top-level DM no longer spawns a fresh workstream (was using per-message `ts` as the route key). - Multi-chunk Slack responses thread correctly under the first chunk's ts instead of fragmenting as independent top-level messages. - Finalize the outgoing StreamingMessage before swapping channel / thread_ts mid-stream, so buffered tokens still land on the old thread. - Redundant `chat_update` on approve/deny eliminated by popping `_pending_approval[ws_id]` after local resolution. - Notification reply tracking on Discord only registers for DMs (guild-channel targets were storing channel IDs where user IDs were expected, so legitimate replies were always rejected). - `get_channel_default_alias` rolls `_channel_default_ts` back on `list_models()` failure so the next caller retries instead of serving an empty alias for the full TTL. - Slack `subscribe_ws` purges dead SSE tasks before the membership short-circuit (previously an unhandled exception left the ws_id in `_subscribed_ws` forever, silently no-opping subsequent subscribes). - ChannelRouter `_create_locks` is now an LRU-bounded OrderedDict that evicts only unheld locks (original dict grew unbounded; naive LRU could evict a held lock and let a second caller race through the critical section, creating duplicate workstreams). - Slack `_parse_ts` pads the fractional field to 6 digits so `"1.2"` and `"1.000002"` stop colliding as `(1, 2)` in the latest-session tiebreaker. ## Performance (6 fixes) - StreamingMessage keeps a rolling truncated display string capped at `max_length` so per-flush cost is O(max_length) instead of O(total_streamed_chars) — long streaming responses no longer do quadratic work every edit interval. - `StreamingMessage.finalize()` caches the joined content so the Discord stream-end DM-forward path doesn't re-join a multi-MB buffer twice. - `PendingApproval` stores the Block Kit payload posted to Slack; `IntentVerdictEvent` appends the verdict in-place and `chat_update`s, skipping an extra `conversations_history` round-trip. - ChannelRouter `lookup_ws_id()` TTL-caches the channel → ws_id resolution (30s TTL, 4096-entry LRU); hot inbound paths skip storage on every message. - Service-discovery startup retry uses exponential backoff (1s → 8s cap) with a 30s deadline instead of 30 × 1s fixed sleep. - `_archive_session` now calls `router.close_workstream` so the `_node_urls` cache entry is dropped (was leaking one entry per archived session). ## Quality / refactors (19 improvements) - `cli.main()` extracted from a 365-line function into focused helpers; imports carefully kept lazy where test patches target source-module paths. - `_run_gateway` finally block now awaits `adapter.stop()` on every adapter so SSE tasks, httpx clients, and the Slack socket handler close cleanly on shutdown. - Shared SSE reconnect loop extracted to `turnstone/channels/_sse.py` (`run_sse_stream` with `on_event` + `on_stale` callbacks); both adapters' `_sse_listener` methods just wire up callbacks. The "404 stops reconnect" invariant is enforced inside the helper so a broken `on_stale` can't livelock. - `_on_ws_event` god-dispatchers split into per-event `_handle_*` methods with a thin isinstance dispatcher at the top. - Slack `_on_approve` / `_on_deny` collapsed into a single `_resolve_approval(*, approved: bool)`. - `ApproveRequestEvent` policy evaluation hoisted into `ChannelRouter.evaluate_tool_policies` returning a `PolicyVerdict`; adapters switch on the verdict kind. - `ChannelAdapter` protocol trimmed to the four methods adapters actually implement; unused `ChannelEvent` dataclass removed. - Shared constants lifted to `turnstone/channels/_config.py`. - `_cleanup_stale_route` and `unsubscribe_ws` share a `_clear_ws_state` helper. - `StreamingMessage` private attrs promoted to `message` / `message_ts` / `accumulated_text` properties so callers don't reach past the `_`-prefix. - Various cleanups: dead var, noqa'd lambdas, renamed `_policy_handled` → `policy_handled`, inlined single-use helpers, added module docstrings, documented `SlackRoute.parse` edge cases. - `chunk_message` plain-text fast path (no backticks → skip fence bookkeeping). ## Test coverage Added 45 tests (178 → 223): - `tests/test_channel_sse.py` (new) — SSE reconnect / backoff / 404-stale-route / on-stale-exception / invalid-JSON-skip / on-event-exception-doesn't-kill-stream / per-connection token refresh / ConnectError retry. - ApprovalView + PlanReviewView owner-check regression tests (owner allowed, non-owner rejected, legacy 2-pipe footer fails closed, modal path rejected for non-owner, `/ask` bot-as-thread-owner follow-up allowed). - Slack `_recover_routes` latest-ts-wins, `_archive_session` drops route + closes workstream. - SSRF tests: DNS rebinding rejected, IPv4 link-local metadata rejected, IPv6 ULA metadata (fd00:ec2::254 / fd00:ec2::23) rejected. - Slack link prefix match (natural-language prompts don't hijack), link rate-limit ceiling. - SlackRoute round-trip across all three shapes + lax-parse behaviour. Lint (ruff) + mypy clean; 210 channel-focused tests pass. * chore(channels): address PR #382 review-bot feedback Three line-level findings from github-code-quality on the backfill review PR. Copilot had no line-level comments. - _sse.py:132 — the `except httpx.HTTPStatusError: pass` branch was flagged as an empty except. The original status was already logged at WARNING inside the try block (we re-raise ourselves after logging), so the handler has real intent. Added a debug log of the exception text + a comment explaining the control flow, so the empty-except lint stops firing and the next reader sees why we fall through to backoff. - discord/bot.py:430, cli.py:354, slack/bot.py:1127 — `await task` inside `contextlib.suppress` was flagged as "statement has no effect". It's a false positive (await is an effect) and the alternative try/except/pass triggers ruff SIM105. Kept the contextlib.suppress pattern and added an explanatory comment above each call so the intent (await CancelledError propagation before state cleanup) is obvious; will reply on the PR thread noting the false positive. No behavior change. Lint + mypy clean; 210 channel tests pass.	2026-04-18 05:49:54 -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	334edbd580	fix(server,console): kind filter on saved-workstreams + closed coords on landing (#380) ... * fix(server,console): kind filter on saved-workstreams + closed coords on landing Two independent bugs folded into one hotfix: 1. Coordinators leaking into the interactive UI's "saved workstreams" sidebar. ``list_workstreams_with_history`` (SQLite + postgres) was kind-agnostic — every coordinator row with conversation history came back alongside interactive rows, and ``list_saved_workstreams`` serialized them uniformly with no kind field so the interactive UI rendered coordinators as regular interactive entries. Fix: add optional ``kind: WorkstreamKind | str | None = None`` kwarg on ``list_workstreams_with_history`` (storage protocol + both backends + the ``turnstone.core.memory`` helper). Pass ``kind=WorkstreamKind.INTERACTIVE`` from the /v1/api/workstreams/saved handler so the interactive surface only sees interactive rows. Default ``None`` preserves legacy all-kinds behaviour for any other caller that wants both. 2. Closed coordinators vanish from the console landing page. ``_coordinator_rows`` in console/server.py built dashboard rows exclusively from the in-memory ``CoordinatorManager`` registry, which pops rows on ``close()``. The persisted storage row stays (state='closed') but never reached the landing-page poller at /v1/api/cluster/workstreams?node=console. Fix: two-lane merge in ``_coordinator_rows``. The in-memory lane (manager) stays authoritative for live session state (model / model_alias / current state / tokens). A new persisted lane queries ``storage.list_workstreams(kind=COORDINATOR, user_id=uid, limit=200)`` and appends rows NOT already in the in-memory set — surfacing closed / error / deleted coordinators so the operator can still see them on the landing page. Ownership semantics unchanged — non-admin callers only see their own tenant, admin-bypass via admin.users/admin.roles honored on both lanes, empty-string defense-in-depth matches _check_row_owner_or_404. Tests: - tests/test_storage_sqlite.py — two new tests: kind filter excludes coordinators from the history list; string form of kind accepted (matches the memory.py forwarding shape). - tests/test_coordinator_endpoints.py — four new tests: - closed coordinators from storage surface alongside active ones. - in-memory row wins on ws_id dedup (live state authoritative). - persisted rows respect tenant filter (non-admin, admin bypass). - orphan rows (empty user_id) never leak to empty-sub callers. Gate: ruff + mypy + pytest -m "not live" (4315 passed) all clean. * fix(server,console): address Copilot review on PR #380 Three review comments folded in: 1. Tenancy leak in /v1/api/workstreams/saved — the handler called list_workstreams_with_history without a user_id filter, so any authenticated user could see every other user's saved workstream aliases / titles / names. Fix: - Add ``user_id: str | None = None`` kwarg to list_workstreams_with_history on the protocol + both backends (SQLite + postgres). Pushes the filter into SQL. - memory.py helper forwards the kwarg. - /v1/api/workstreams/saved reads ``_auth_scopes(request)``: a service-scoped caller gets cluster-wide visibility (None), a non-service caller with a blank ``sub`` returns an empty list, otherwise the SQL filter is scoped to the caller's uid. Matches the _visible_workstreams pattern used on /workstreams and /dashboard. 2. Loose type annotation on the memory.py helper — ``kind: Any`` tightened to ``WorkstreamKind | str | None`` so mypy catches invalid callers. WorkstreamKind was already imported in the module. 3. Brittle positional indexing in _coordinator_rows persisted-rows lane — ``row[10]`` for user_id encoded a column offset that would silently corrupt the projection on any future SELECT reorder. Drop the test-double fallback entirely; the storage-protocol contract already requires SQLAlchemy Row with _mapping, and every real caller (SQLite + postgres) provides it. Tests: - test_server_authz.py TestSavedWorkstreamsTenantScoping — four new regression tests covering: non-service caller sees only own rows, service scope sees cluster-wide, blank-sub non-service returns empty, and coordinator rows excluded even for service callers. Gate: ruff + mypy + pytest -m "not live" (4319 passed) all clean.	2026-04-18 03:10:19 -07:00
Patrick Buckley	c17eddbbd8	fix(console): service scope on collector token + surface upstream 4xx (#379) ... * fix(console): service scope on collector token + surface upstream 4xx CRITICAL: the console's ClusterCollector ServiceTokenManager was configured with only frozenset({"read"}) scope, but every upstream node's /v1/api/events/global hard-gates on "service" scope (added in PR #375 for cross-tenant authz hardening). Every console→upstream SSE connect 403'd, the collector never populated node state, and the failure was silent — node health, idle workstreams, and interactive- kind workstream rows all disappeared from the console dashboard with no user-visible error. The only surface was a log.debug line in the collector's _node_sse_task that operators had to opt into via DEBUG logging or browser DevTools. Fix: - Add "service" to the collector_token_mgr scopes (turnstone/console/server.py). Matches the proxy_token_mgr (which already has it) and the existing cli / admin / channel-gateway service tokens. Restores /v1/api/events/global SSE subscription and /v1/api/dashboard visibility (which silently tenant-filters non-service callers to zero rows). - Upgrade the 4xx path in _node_sse_task to log.warning with the status code + 200-char body preview, so configuration-level failures (scope misconfig, JWT secret mismatch, expired token) show up in operator logs instead of being masked by the generic except-block debug line. Keep transient network errors (CancelledError, ConnectError) at debug so the log doesn't flood during brief node restarts. - Add reachable_reason field to NodeSnapshot + surface via get_nodes / get_node_detail / get_snapshot (and the browser's buildNodeInfoFromSnapshot). Operators now see the failure cause on the cluster node list without tailing the log. Cleared on successful reconnect in _apply_snapshot. - Test coverage: test_server_authz.py TestGlobalEventsServiceGate gains a positive-path test asserting that a token with exactly the collector's scope set ({"read", "service"}) is accepted by /v1/api/events/global. Locks in the scope contract so any future rename breaks the test before it breaks the dashboard. Gate: ruff + mypy + pytest -m "not live" (4309 passed) all clean. * fix(console): address Copilot review on PR #379 Two review comments folded in: - collector.py — bounded body read for 4xx SSE error previews. The prior ``await source.response.aread()`` buffered the entire upstream error body into memory just to log a 200-char preview; a malicious / oversized upstream response (HTML error page, proxy- generated body) could have forced the collector to download an arbitrary amount of bytes. Iterate ``aiter_bytes()`` and stop once the preview cap (256 bytes, ~200 chars after UTF-8 decode) is satisfied. - test_server_authz.py — tighten the service-scope positive test. The prior ``assert resp.status_code != 403`` could pass on unrelated 500s AND left an SSE stream open indefinitely. Send ``?expected_node_id=definitely-wrong-node-id`` so the handler passes the scope gate, hits the post-auth node-identity check, and returns 409. Now ``assert resp.status_code == 409`` proves the scope contract precisely and terminates the request immediately. Gate: ruff + mypy + pytest -m "not live" (4309 passed) all clean.	2026-04-18 02:48:57 -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	3bdcf9870e	fix(server): close review cleanup items from PRs #374 / #375 review (#376) ... Third and final PR of the retrospective-review series. Addresses the remaining bug / perf / doc findings from the original multi-stage review plus the three inline comments left on #374 and #375. From the original review: - bug-3: delete_workstream now nulls out parent_ws_id on every child row before dropping the target — previously, deleting a coordinator left orphaned parent_ws_id pointers and list_workstreams(parent_ws_id= <deleted>) kept returning ghost-parented rows. Fix lives at the storage edge so both SQLite and PostgreSQL benefit without a schema migration. - perf-1 / perf-2 / perf-3: new migration 041 drops the low-cardinality idx_workstreams_kind outright, rebuilds idx_workstreams_parent as a partial index (WHERE parent_ws_id IS NOT NULL) to halve its btree, and uses CREATE INDEX CONCURRENTLY on postgres so the rebuild doesn't take ACCESS EXCLUSIVE on populated tables. Dialect-guarded; sqlite path is a straight partial CREATE INDEX. - perf-5: _rebuild_children_from_storage bumps its limit sentinel to 10_000 and logs a warning when the cap is hit instead of silently truncating the tail on every console cold-start. - q-2: turnstone.core.memory.list_workstreams wrapper deleted (zero live callers; PR #374 kept it forward-compatible with the new kwargs as a stepping stone). - q-5: migration 039's docstring now warns operators that downgrade drops parent_ws_id irreversibly and notes the 041 dependency. - q-7: GET /v1/api/workstreams row shape now includes kind + parent_ws_id to match /v1/api/dashboard; the Pydantic WorkstreamInfo schema follows so SDK consumers see the same fields. Inline review comments: - #374 (copilot): console/server.py::coordinator_children now pushes user_id into the SQL filter for non-admin callers, so forged / migration-era rows with matching parent_ws_id but a different owner can't leak through. Admins bypass the filter — they're expected to see the full subtree. - #375 (copilot, delete handler): storage.get_workstream(ws_id) for the audit snapshot moved inside the try: block so a transient DB error surfaces through the endpoint's redacted 500 handler instead of an unhandled exception. - #375 (copilot, _require_ws_access): added optional mgr= kwarg — when the workstream is live in the in-memory manager, trust its cached user_id instead of round-tripping storage. In-memory-only handlers (approve / plan / cancel / command / close / events_sse / refresh-title / set-title) pass mgr= so they stay functional during transient DB outages and skip one query on the hot path. Storage-backed handlers (/delete, /open) omit mgr= and keep the storage path for persisted-but-not-loaded rows. Tests: - tests/test_workstream_kind.py adds regression tests for the cascade null-out on delete and the new user_id SQL filter. - tests/test_workstream_endpoints.py updated so the title-handler tests exercise the in-memory fast path (MagicMock manager returning None falls through to storage; explicit ws.user_id set where the mock ws is used). Lint (ruff), typecheck (strict mypy), pytest -m 'not live' all green (4209 passing).	2026-04-17 22:42:46 -07:00
Patrick Buckley	294d6f5766	fix(server): close cross-tenant authz gaps on interactive-ws handlers (#375) ... Second of three PRs addressing the retrospective review of the turnstone-server interactive-kind feature. The first (PR #374) put the structural pieces in place — WorkstreamKind enum + user_id kwarg on the storage protocol. This PR uses them to close the handler-level ownership gaps that shipped under the prior design. - sec-1: approve / plan_feedback / cancel_generation / command now call _require_ws_access before touching the target UI. Previously any authenticated user could resolve pending tool-approvals on another tenant's workstream — RCE-adjacent because the attacker could approve destructive operations the victim would have denied. - sec-2: /v1/api/workstreams/{ws_id}/delete now gates on ownership AND writes a workstream.deleted audit event. Previously any authenticated user could destroy any other tenant's workstream, conversations, and attachments in one call with no tamper-evident trail. - sec-3: /v1/api/events (per-ws SSE) gates before _register_listener so non-owners can't subscribe to another tenant's message / tool / approval stream. - sec-4 / sec-5: /v1/api/workstreams and /v1/api/dashboard filter to the caller's tenant view via a new _visible_workstreams helper; service-scoped tokens (cluster / routing proxy) keep the full view. - sec-6: /v1/api/events/global requires service scope. The global snapshot carries cross-tenant workstream inventory and was never intended for end-user browsers. - sec-7: /v1/api/workstreams/{ws_id}/open verifies the caller is the stored owner (or holds service scope) before rehydrating. Returns 404 on mismatch — existence isn't enumerable by response code. - sec-8 / sec-9: /workstreams/close, /refresh-title, /title all gate on ownership. Cross-tenant close aborts the victim's running generation; cross-tenant rename is a phishing / denial-of-use vector in list / dashboard responses. - sec-11: workstream.created / .deleted / .closed / .opened now land in the audit_events table with kind + parent_ws_id detail, so forensic review can reconstruct lifecycle even after the row is gone. - q-4: new tests/test_server_authz.py covers every gate above via TestClient, plus the PR #1 HTTP-boundary kind-validation branches that had no regression coverage (coordinator / unknown-kind / 400, cross-tenant parent_ws_id / 403, non-interactive open / 400). - q-3: test_workstream_kind.py now uses the conftest storage fixture so it runs against both SQLite and PostgreSQL under --storage-backend=postgresql, closing the sqlite↔postgres drift risk the prior review flagged. Added storage-edge ValueError and user_id SQL filter tests alongside. Tests, lint (ruff), typecheck (strict mypy) all green. Stacked on PR #374 — merges after that lands.	2026-04-17 22:22:03 -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