mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
main
7 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Patrick Buckley
|
36419a9809 |
fix: scope private-project workstream visibility to members, not admins
Workstreams attached to a private project were visible -- including their conversation content -- to holders of admin.cluster.inspect / admin.coordinator (both default builtin-admin permissions), defeating the project's confidentiality boundary. Enforce that a private project's resources are visible only to people IN the project (owner, workstream creator, or an explicit member), even for admins. Surfaces closed: - WorkstreamProjectVisibility bypass narrowed to service scope only (node->console machine plumbing, re-filtered per-user at the console edge). No human principal bypasses; admin.cluster.inspect gates the inspect surfaces, not tenancy. This flows to /dashboard, session listings, the attachment row-gate, cluster_workstreams, cluster_node_detail, and cluster_snapshot/SSE. - cluster_ws_detail 404-masks a workstream in a private project the caller can't see; cluster_ws_live_bulk routes such ids to the denied list (no private-project oracle). - Coordinator operator verbs (history/export/detail/send/approve/set_title/open/ children/tasks/attachments) now enforce project tenancy: _coordinator_tenant_check on coord_endpoint_config, the gate in _resolve_coordinator_or_404 (children/tasks), the tenant_check now run in make_open_handler before rehydrate, and a project-visibility check in _coord_attachment_owner. admin.coordinator gates the surface cluster-wide, but a non-member is 404-masked. The tenant-check mirrors the manager-first + coordinator-kind ladder so kind-isolation is preserved. - service scope is no longer user-assignable: admin_create_token and both turnstone-admin CLI mint paths reject it via reject_unassignable_scopes, so an admin.users holder cannot self-mint a service token and restore the bypass. Service scope is minted only by ServiceTokenManager / the JWT secret. - The events/global node proxy (service-elevated cross-tenant firehose) is gated on admin.cluster.inspect so a plain authenticated user cannot reach it through the console proxy. Updates the OpenAPI description, the row-gate/tenancy-filter docstrings, and adds tests for every surface (visibility predicate + cluster detail/bulk + coordinator history/export/children/open/attachments + events/global proxy + scope-mint rejection); inverts the tests that pinned the old admin-bypass contract. |
||
|
Patrick Buckley
|
7f0e0406b3 |
test(approvals): concurrency matrix + suite migration to the cycle model
New regression matrix for the release blockers: cross-approval independence, lost-wakeup at gate entry, FIFO selector-less resolution, resolve-all sweep, double-resolution no-op, cards/legacy view tracking, and the generation-exactness set — stale delivery rejection, Smart-Approvals origin check, purge keep_origin, the purge-to-register window eviction, late cross-generation "superseded" stamping, concurrent smart+human gates, and the pre-delivered-verdict fast path. Plus sub-agent judge wiring (agent_gate off the main slot, close() firing all generations) and endpoint tests for cycle pinning and the Approve+Always race guard. Gate threads run under one shared mock-patch harness — mock.patch start/stop of the same target from concurrent threads corrupts the patcher's restore stack — with a sweep-until-dead teardown so the conftest leak guard can't trip. Existing suites migrate off the singleton fields to cycle assertions and the pending_approval_details wire shape. |
||
|
Patrick Buckley
|
7d2d7db9d2 |
feat(approve): pass pending_approval_detail through cluster live-bulk
Threads the field added by Chunk 1 through the console's live-bulk endpoint so coord tree UI can read it without a separate per-child fetch. Three touchpoints: - _CLUSTER_WS_LIVE_KEYS gains the new key so _fetch_live_block's projection forwards it from the upstream /dashboard response on node-backed child rows. - _coordinator_live_snapshot synthesizes the same shape from ConsoleCoordinatorUI._pending_approval for in-process coord rows (no upstream /dashboard exists on the console pseudo-node). - One source of truth: SessionUIBase.serialize_pending_approval_detail. Both branches now emit the same 12-key live block; coord judge isn't wired today so coord-self judge_verdict is always None — flagged in the plan as a stretch follow-up. Plan: docs/design/inline-child-approvals.md (chunk 2 of 4). |
||
|
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).
|
||
|
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.
|
||
|
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 |
||
|
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.
|