mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
9adde920d499e0c30823c848b2e716e5c232b51b
39 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 metaclassing
|
9adde920d4 |
feat(models): per-alias backend auth via Entra OBO and app identity (#898)
Adds a per-alias `auth_mode` on model definitions so a model backend can authenticate to an Entra-fronted gateway with a per-request minted token instead of one shared static API key, letting the gateway attribute calls to the actual user or to the app as a machine identity. - `static` (default, unchanged) sends the stored `api_key`. - `entra_obo` mints a per-user On-Behalf-Of token for `obo_audience` from the caller's captured refresh credential. - `entra_app` mints an app-identity token via the client-credentials grant, and covers userless turns that OBO cannot. Reuses the existing OBO grant legs, refresh-token rotation CAS, cluster advisory lock and the `mcp_user_tokens` mint-cache, keyed under synthetic `__model_obo__:<audience>` / `__model_app__:<audience>` rows. The token binds at the call site through `client.with_options(api_key=...)` so each SDK emits it on its own auth path rather than through header injection. Migration 068 adds `auth_mode` and `obo_audience`. Both are additive and existing rows default to `static`, so behaviour is unchanged unless an alias opts in. Operator controls: `model.auth_audience_allowlist` is an exact-match allow-list that gates which audiences may be configured and denies all by default, and changing a mode or audience requires `admin.mcp`. `model.auth_fail_closed` decides whether a failed mint may fall back to an explicitly configured static key. A delegated call with no user, or a dynamic alias with no real static key, always refuses. Two changes here apply regardless of whether any alias opts in: - Storage and app state are now wired into the console MCP client manager. This fixes per-user `oauth_user` / `oauth_obo` dispatch for coordinator-hosted sessions, which previously raised `RuntimeError` on first call because `set_app_state` was only ever called on the node. - Unattended watch restores and `--resume` resolve the persisted workstream owner instead of constructing the session under an empty principal. A workstream with no owner is now a permanent refusal rather than an anonymous, auto-approved run. |
||
|
Patrick Buckley
|
984a10307e |
feat(coordinator): MCP tool surface for coordinator sessions (#725)
Coordinator-kind workstreams get the same MCP surface as interactive sessions — tools, resources, and prompts (read_resource/use_prompt go dual-kind) — gated per-persona exactly like interactive, with no separate feature flag. The console hosts its manager with node parity end to end: boot calls create_mcp_client inline (same catalog resolution: DB rows, then mcp.config_path, then this host's config.toml), the admin reload fan-out lazily constructs and reconciles it under a lock (the node's unlocked equivalent is #873), per-server refresh/reconnect and the admin MCP status view cover it under the collector's console pseudo-node id, and shutdown follows LIFO teardown. Sessions read the live manager through a per-construction getter — the console counterpart of the node factory's mcp_ref[0] read; client presence is the session-level contract, and the kind-aware tool assembly runs the same listener/prime/rebind skeleton as interactive. bind_acting_user re-scopes listeners and per-user pools, which is security-critical for multi-sender coordinators. The wire-safety status projections move verbatim to core/mcp_utils so both hosts present one schema (node endpoint bodies byte-identical); the console's per-server action classification is a pinned COPY of the node endpoints', with a parity test driving both sides across the outcome matrix that fails if either drifts. The shared MCP error card (consent / re-consent / forbidden / operator) moves to mcp_error.js + mcp_error.css, linked by all three card hosts and pinned by className→rule and host→link parity tests; the module joins the whole-file sink-scan and var-ratchet lists. Reload reporting is honest about the console entry: excluded from the unreached-node warning's list and denominator, and the toast claims "+ console" only for a real reconcile, with an explicit note on failure. The pending-consent badge (#874's console half) ships too: the console defines the same onConsentDetected seam the node dashboard exposes — lighting up the shared pane host's existing bridge for hosted interactive panes — and the coordinator pane threads its card's detections through the single MCP-error helper. The badge rides the Admin > MCP Servers rail row, hydrates at boot from the Phase 9 pending-consent endpoint the console already serves, re-syncs to DB truth when the operator views the MCP panel, and the rail-less standalone page carries a status-bar chip instead. A coordinator that hits a consent wall unattended now has a persistent, glanceable signal. Pre-existing bugs fixed along the way: create_mcp_client returned None on pool-only installs, leaving any host managerless after restart until the next admin MCP write; admin_import_mcp_config never scheduled the reload fan-out (stale catalogs after import); the admin settings UI rendered the coordinator settings section unordered and unlabeled. Follow-ups: #873 (node reload double-construct race); #874 narrows to the admin-MCP-view per-server indicator. |
||
|
Patrick Buckley
|
460308241d |
fix(tools): lower working directory and workspace into fs tool descriptions
The process cwd was nowhere in the model's context: shells start in the inherited process cwd (spawn_group_leader passes no cwd), relative file paths resolve against it, but nothing told the model where it was standing — in stock Docker every shell ran in /data while user files sat in the /workspace mount, and the model's only recourse was to probe with pwd (#857, #833). Lower both facts into the tool schemas, where they gate intrinsically on tool availability (a persona without fs tools carries no note, and coordinator envelopes are untouched): - tools/*.json: cwd_note/workspace_note metadata templates on bash, read_file, write_file, edit_file, search, diff_file; bash also states the fresh-shell-per-call semantics (cd does not persist) and drops a stale reference to the removed man tool. - tools.apply_cwd_context(): renders the notes into descriptions; deep-copies noted tools (the fs dicts are shared across TOOLS/INTERACTIVE_TOOLS/TASK_AGENT_TOOLS and aliased through merge_mcp_tools), passes note-less tools through by reference. - ChatSession._apply_cwd_notes(): wraps every fresh interactive build of _tools AND _task_tools (construction, MCP catalog change, MCP disconnect) — assignment-time, so the wire tools block stays byte-stable for provider prompt caches. os.getcwd() is OSError-guarded (MCP rebuilds run on a background thread; eval tears down its workdir); the workspace hint drops when the dir is missing or equals the cwd. Task-agent sub-agents carry their own notes via _task_tools, independent of parent persona visibility. - config.get_workspace_dir(): [tools] workspace_dir with TURNSTONE_WORKSPACE env fallback (searxng pattern), informational only — no chdir, no path confinement (per-workstream working-dir grants are a separate planned feature). - Dockerfile: ENV TURNSTONE_WORKSPACE=/workspace so stock deployments surface the mount with zero operator config. - docs/docker.md: document the /data working directory, the working_dir: /workspace compose override as the operator-level fix, and the SQLite-fallback-DB-in-cwd caveat. Closes #857 |
||
|
Patrick Buckley
|
80e7b9e9ca |
fix(mcp): review round 8 — push-success can't declare health, first-notify never debounced
- A single-kind push SUCCESS no longer clears the server error pill or stamps 'ok': _last_error / _last_refresh are server-scoped but a push refreshes only ONE kind, so a tools-failing server must not go green because its prompts push succeeded (a wrong-healthy window, bounded by the health tick — but a real 200-OK lie). Only a full pass declares 'ok'; the failure's armed health-tick retry runs it. This reverts the over-reach of round 7's push-success outcome write (a self-inflicted regression) — net simpler. - The (server, kind) debounce uses a None sentinel, not a 0.0 default: time.monotonic() counts from boot, so on a node whose process started < _NOTIFICATION_DEBOUNCE (5s) after boot, the 0.0 compare would debounce the VERY FIRST push — dropped with no recovery on the pool path. Absent stamp = never refreshed = always admit. - _record_refresh_skipped completes the outcome-helper set: the three inline 'skipped' stamps now share one config-gated helper (with _record_refresh_success / _record_refresh_failure), and the reconnect-success branch routes through _record_refresh_success — no more hand-copied gates to drift. - The per-message refreshers dict + on_debounce_drop closure are built ONCE per handler (both static and pool), not on every server->client message before the isinstance/debounce/coalesce early-returns. Accepted (documented): an operator /mcp refresh that finds the connect lock busy skips + arms the retry rather than waiting (waiting re-introduces the refresh-budget exhaustion busy-skip exists to prevent). 4 findings refuted. Suite 9408 green. Refs #839 |
||
|
Patrick Buckley
|
52dcb6a47b |
fix(mcp): review round 7 — consolidate the refresh-outcome write path
All three round-7 findings shared one root cause: last_refresh_outcome (the single source of truth for the CLI / endpoint / admin pill) was written inconsistently — ungated writes scattered across _refresh_server and _refresh_all, never written by the push path, never popped on removal. Consolidate every static outcome write through two config-gated helpers so the invariant holds: _last_refresh[name] exists IFF the server is configured and has a real outcome. - _record_refresh_failure now stamps the (config-gated) error:<Class> outcome; _record_refresh_success is its twin (gated ok stamp + pill clear). The ungated writes inside _refresh_server (both the internal error write and the success write) and _refresh_all's except are removed — routed through the helpers. A failure observed for a just-removed server no longer leaves a permanent stale error: row. - The push-driven refresh path (_run_static_notification_refresh._record) now records the outcome on BOTH success and failure, not just the error pill — a green 'ok' outcome no longer persists under a red error row after a push fails, and a successful push clears a prior error. - remove_server_sync pops _last_refresh (via _clear_static_push_state markers=True); a session drop KEEPS it (the outcome persists across a reconnect — only removal clears it). The removed-mid-pass branch drops any stale row too, so last_refresh_outcome doesn't report a departed server's prior 'ok'. _reap_bounded's pending-task concern was reviewed and REFUTED (a pending child on external cancel during shutdown is correctly left to loop teardown). Declined the per-notification refreshers-dict allocation cleanup: trivial (a 3-entry dict on a rare debounced path), and the late binding is deliberate for test overrides + mypy attribute checks. Tests: push-refresh success/failure write the outcome, removal pops it, session drop keeps it, failure for a removed server leaves no stale row; the 3 TestLastRefreshTracking tests updated to the split contract (_refresh_server propagates, the caller records). Suite 9407 green. Refs #839 |
||
|
Patrick Buckley
|
86aeb43120 |
fix(mcp): review round 6 — close the refresh-outcome reporting residuals
Three residual gaps in the round-5 skip-outcome threading, all in _refresh_all's other reconnect branches plus the endpoint ordering: - The disconnected-server reconnect DEFERRAL (_ensure_static_connected returns None: a sibling call in flight on the old stack, lock not held) returned None without stamping 'skipped', so the endpoint and pill read the STALE prior 'ok' and reported a never-run refresh as current. Now stamps 'skipped' like every other skip branch. - A server removed from config between the top-of-loop session check and the cfg lookup fell through to with results[name] UNSET, omitting it from the returned dict — an operator refreshing that one server saw a bare 'refresh complete' with no line. Now reports None so it renders. - internal_mcp_refresh_one checked 'skipped' BEFORE the error pill, so a skip on a server carrying a live error returned a benign 202 instead of 500 — a status-code-keyed caller would treat an erroring server as healthy-but-busy. Error is now checked first. - _reap_bounded swallowed an external CancelledError (shutdown / an operator cancel of the refresh runner) — it now re-raises after a best-effort exception retrieval, honouring the cancel. Dropped the unneeded asyncio.shield in the process. Tests: deferral stamps skipped, removed-mid-pass reported not omitted, endpoint error-beats-skip → 500, reap re-raises external cancel. Suite 9403 green. Refs #839 |
||
|
Patrick Buckley
|
748f670fe8 |
fix(mcp): review round 5 — thread the refresh outcome to every operator surface
The 'skipped'/None refresh sentinel added in round 4 was only half
threaded: consumers still misreported it. Unify all operator surfaces
on ONE source of truth — the per-server last_refresh_outcome ('ok' /
'skipped' / 'error:<Class>') — exposed via a new last_refresh_outcome()
accessor:
- _refresh_all returns None (not ([], [])) for a FAILURE too, so a
failed refresh is never rendered as 'no changes' (the pre-#839 lie
the sentinel exists to close); None is disambiguated skipped-vs-failed
by the outcome. ([], []) now strictly means 'ran, no changes'.
- /mcp refresh renders skip ('skipped — retry scheduled') and failure
('refresh failed (error:X)') distinctly from 'no changes'.
- The node-internal refresh endpoint returns 202 'skipped' instead of a
misleading 200 'ok' for a refresh that never ran (the busy-lock skip);
it reads the outcome from the manager accessor because the public
status projection deliberately whitelists last_refresh_outcome out.
- admin.js paints 'skipped' with a neutral info pill
(.mcp-refresh-pill-skip), not the error-red any-non-'ok' used to get.
- _admit_list_changed rolls back BOTH the coalesce marker and the
debounce stamp when scheduling raises, so a same-kind push in the
window afterward isn't debounced against a refresh that never spawned
(the pool path has no on_debounce_drop recovery).
Tests: endpoint 202-skip, CLI skip/failure render, _refresh_all
failure→None + outcome, spawn-failure stamp+marker rollback. Suite
9399 green.
NOTE filed #843: the admin refresh pill's data (last_refresh_at/outcome)
is stripped by BOTH status projections and never reaches admin.js — a
pre-existing latent bug (the pill has never rendered); the admin.js
color fix here is correct-when-reachable. Out of #839 scope (the read
projection strips it for a privacy reason that needs its own coarsening
decision).
Refs #839
|
||
|
Patrick Buckley
|
1a80466369 |
fix(mcp): review round 4 — removal/reconcile lifecycle, honest skip reporting, same-kind debounce recovery
reconcile_sync no longer abandons a DB-driven removal that timed out: both the removal loop and the config-update loop keep the name in _db_managed (and skip the follow-on add) when remove_server_sync returns its mutated-nothing False, so the next pass retries instead of the deleted/reconfigured server serving stale tools until restart. remove_server_sync is now cancel-safe end to end: it FORCE-drops the session before queueing (parked push runners bail at their session gate instead of serializing ≤30s list calls ahead of the removal — the noisy #839 server was exactly the one whose runners could starve its own removal), and wraps the post-lock cleanup in try/finally so a caller-timeout cancel landing mid-teardown still completes the state pop, catalog rebuild, and lock retirement rather than stranding a config-gone ghost catalog. Config survives a park-cancel, so the health loop recovers it. _refresh_all reports None (not a fake ([], [])) for a busy-skip or supersede, stamps a 'skipped' status row, and /mcp refresh renders it distinctly — the operator is no longer told a never-refreshed server is current. A same-kind push lost to the debounce window (the prior runner already finished; the server won't re-announce) arms the health-tick retry, closing the one staleness hole the per-kind debounce still had; a push covered by a queued runner does not arm (no lost change). Static resource/prompt catalogs are capped at connect discovery and every refresh. _list_resource_pair's reap is bounded so a future SDK cancel-regression can't wedge the lock. Cleanups: _arm_refresh_retry (retry-arm gate, ×3), _spawn_full_refresh (discard+spawn, ×3), _popen_mcp_server (live-server spawn, ×2), the tautological stamp-arithmetic TestNotificationDebounce deleted. Suite 9395 green. Refs #839 |
||
|
Patrick Buckley
|
53f11454ad |
fix(mcp): review round 3 — cap static catalogs, atomic removal, unify the list_changed protocol twins
- Static resource/prompt catalogs are now size-capped at connect
discovery AND on every refresh (mirrors the pool twins and the static
tools path): a misbehaving server's push ran uncapped through the new
spawned refresh path and could balloon the shared node's merged
catalogs on every notification.
- remove_server_sync mutates NOTHING outside the per-name lock: the
up-front config pop meant a removal cancelled while parked (behind
the push-refresh runners that now share this lock) left a
half-removed server — config gone, session and published catalogs
alive, no driver able to reconnect or cleanly re-remove. A timed-out
removal is now honestly retryable.
- _refresh_all's DISCONNECTED branch busy-skips too (parking inside
_ensure_static_connected burned the pass's 30s budget on one
mid-reconnect server), and a busy-skip on either branch ARMS the
health-tick retry — an operator-requested refresh can no longer be
silently dropped with output indistinguishable from 'no changes'.
- reconnect_sync drops the session before queueing on the lock (FORCE
semantics already rebuilt live sessions): parked push runners bail
at their session gate instead of serializing up to one 30s list call
per kind ahead of the operator's recovery action. Residual: one
mid-list holder can still precede the 45s attempt; a timed-out
reconnect is honest and retryable.
- _refresh_server's supersede check gains the session arm: a spawned
retry/post-reconnect pass racing an eviction skipped instead of
manufacturing a false 'not connected' error pill (and a re-arm loop)
for a self-healing condition.
- The list_changed protocol twins are UNIFIED (Closes #842): the
admission half (_admit_list_changed) and the runner half
(_run_list_changed_refresh) each exist once as plain parametrized
methods — values and small closures, no factory layer (mcp v2 drops
the factory pattern; the two thin message_handler closures remain
only as SDK-v1 bindings). The one true asymmetry — coalesce-marker
ownership on the superseded path — is a documented boolean: pool
markers are only ever cleared by their runner; static markers are
cleared by remove_server_sync, so a present marker belongs to the
re-added generation. Both runners keep their names and signatures;
the notification suites pass unchanged.
- Cleanups: per-kind staleness rechecks stripped from the static
refreshers (unreachable under the lock discipline — the MUST-hold-
lock contract is documented instead); _run_hl (5th run-on-loop copy)
replaced at 44 call sites; _poll_until centralizes the live-test
wait loops; docs no longer describe the periodic refresh tier
removed in
|
||
|
Patrick Buckley
|
f8f191686f |
fix(mcp): review round 2 — busy-skip the refresh pass, fail-fast list pairs, health-tick refresh retry
- _refresh_server never parks on a held connect lock: the holder is itself a catalog publisher whose publish supersedes the pass, and parking burned refresh_sync's whole 30s budget on ONE busy server (a reconnect attempt holds the lock up to 45s), failing the operator pass for every healthy server queued behind it. Busy → skip (None), no publish, no status writes; the identity/state recheck stays as belt-and-braces for the one-tick check→acquire race. - _list_resource_pair: the ONE copy of the paired resources/templates list protocol (both twins). Fail-fast — a fast real error (auth / method rejection) surfaces as ITSELF instead of being masked behind a hung sibling's eventual 30s TimeoutError — with the survivor CANCELLED and REAPED inside the timeout scope, never left detached on the shared session. - Health-tick refresh retry: there is NO periodic refresh pass (removed in eb2a119d; the docs still claimed the 4h tier — fixed), so a push refresh that failed while the transport stayed up had no automatic recovery and the shared catalog stayed stale for every user until an operator intervened. Failures and busy-skips arm _static_refresh_retry via the shared recorder; the health tick drains it with one bounded, lock-serialized full pass per tick; success, session drops, removal, and the post-reconnect spawns clear it. This also un-latches the error pill: the retry's completion clears it within a tick. - _record_refresh_failure: the bearer-redaction policy (type + message, never exc_info) lives exactly once; all three refresh-failure sites route through it. - Static runner discards its coalesce marker only AFTER the lock-identity check: on the superseded path a marker present in the set belongs to the re-added generation's parked runner, and discarding it would mint duplicates past the one-parked-runner bound (the pool runner deliberately differs — nothing else clears pool markers, so its marker is its own to release). - _clear_static_push_state: the ONE (server, kind) keyspace walk for stamps + retry flag (+ markers on removal). - Tests: busy-skip, superseded-no-status, fail-fast + reap (<5s bound), retry arm/drain/re-arm/clear quartet, logged-wrapper contract updated to the shared recorder's arg shape; vacuous stamp-math test deleted (behavioral per-kind coverage retained); _free_port/_wait_tcp_ready/_wait_session_live hoisted to conftest for both live tests. Refs #839 |
||
|
Patrick Buckley
|
aefcf53405 |
fix(mcp): review round 1 — supersede retired-lock refreshes, per-kind debounce, complete gather pairs
- _refresh_server: post-acquire lock-identity + state-existence recheck; a pass superseded by remove (or remove + re-add) returns None and writes NO status — it must not run its list calls as a second, unserialized publisher against the re-add's discovery wiring, resurrect status rows for a removed server, or stamp a false "ok" over a generation it never refreshed. _refresh_all treats None as a deliberate skip (no breaker success record). - Debounce stamps are per (server, kind) on BOTH paths: refreshes are kind-scoped, so a server-scoped stamp dropped a different-kind notification inside the window outright — a tools push swallowed the prompts push 100ms behind it, and nothing observed the prompt change until the server pushed that kind again. Teardown pops loop the kinds; remove_server_sync also discards the server's coalesce markers so a parked old-generation runner's marker cannot coalesce away a re-added server's first push. - Resource refreshers (static + pool) gather with return_exceptions=True: fail-fast gather left the surviving list call running detached — outside the timeout scope and the lock serialization — as an unbounded in-flight request on the shared session. - Spawned post-reconnect refreshes route through _refresh_server_logged: the re-raise escaped into _spawn_background's done-callback, whose exc_info log serializes the chained httpx.Request carrying the configured bearer for auth_type=static servers; _refresh_all's except drops exc_info for the same reason. Failure diagnostics widen to "Type: message" in logs and the error pill — the message text is header-free; only the serialized chain leaks. - Accepted + documented: connect-lock contention on dispatch reconnects is bounded to one in-flight list call (parked runners bail instantly post-eviction); the error pill persists until the next COMPLETED refresh (a notification's arrival proves nothing about whether the failure resolved). - Tests: per-kind debounce independence, superseded-pass writes nothing, gather-sibling completion, logged-wrapper swallow with the exc_info channel asserted SILENT, remove clears markers; _run_on_loop/_drain_background hoisted to conftest (4 drifted copies); proc.kill() portability in the live push test. Runner-twin dedup (static/pool protocol duplication) deferred to #842. Refs #839 |
||
|
Patrick Buckley
|
37144991c9 |
fix(mcp): spawn static list_changed refreshes off the receive loop
The static-path notification handler awaited its catalog refresh inline in the SDK's receive loop, but the refresh issues a request on the same session — a request whose response only that (now parked) loop could route. The refresh never completed, and every user's calls on the shared per-node session stalled behind it, unbounded, until the health loop's ping timeout tore the transport down — which was also the only way a pushed catalog change ever landed. Port of the pool-path protocol (#836) onto the static primitives: - Refreshes are debounce-gated, coalesced per (server, kind), and spawned as tracked tasks; the runner serializes on the per-name connect lock so a refresh, a connect's discovery wiring, and the manual/periodic _refresh_server pass can never publish out of order (the remove -> re-add race is closed by lock identity, the static twin of the pool's entry-identity check). - The coalesce marker is cleared at lock-acquire so a change the in-flight list missed spawns exactly one successor; the finally discard is gated on non-acquisition so it never clobbers that successor's marker. - The debounce stamp survives a failed refresh (throttle over lost window) and every teardown/eviction path now pops it via the paired _drop_static_session_and_stamp, so a reconnected transport's first notification refreshes immediately. - All three static list calls are bounded by _CONNECT_TIMEOUT and discard their result if the state entry was replaced mid-flight; the resource pair rides one gather (mirrors the pool sibling). - Failure logging is (Exception, BaseExceptionGroup) type-name-only: an escaping group reaches _spawn_background's exc_info log, which serializes the chained httpx request carrying the configured bearer for auth_type=static servers; the recorded operator error string is type-name-only for the same reason. Non-list-changed notifications no longer clear the server's error pill (that pop was accidental — only a completed refresh proves anything). Includes a live end-to-end repro (FastMCP subprocess pushing tools/list_changed through a real receive loop): pre-fix the triggering call itself deadlocks (verified against main), post-fix it completes with the catalog landing on the original session, no teardown. Closes #839 |
||
|
Patrick Buckley
|
0c2c534c86 |
fix(mcp): route pool transport lifecycles through per-entry owner tasks (#788)
* fix(mcp): route static transport lifecycles through per-server owner tasks A crash-looping MCP server drove the mcp-loop thread to a sustained, climbing 100%+ CPU spin. Root cause: anyio cancel scopes are host-task-bound, and the static path entered the SDK's transport / ClientSession task-group scopes from short-lived connect tasks (every health tick is a new task since #768). Once such a scope was cancelled after its host task had finished - by anyio's task_done when a transport child died with the server, or by ClientSession.__aexit__ during a cross-task teardown - CancelScope._deliver_cancellation could never make progress (task.cancel() on a done task is a no-op) and re-armed itself via call_soon every loop iteration, forever: ~900k callbacks/s per zombie scope, one more per flap cycle (verified against anyio 4.14.1; no upstream fix exists as of that release). Fix: each static server's transport + session cms are now entered, parked, and exited by ONE long-lived owner task (_static_transport_owner), so scopes always have a live host and always exit in the task that entered them. Teardown follows a one-cancel close protocol (signal the close event before the first await, graceful grace, then at most ONE cancel - never a second, which would abandon a scope exit mid-flight). Connect timeouts now cancel only the waiting caller; connect failures are delivered through a readiness future; unrequested owner death (server died under a live session) evicts the session immediately via a done-callback instead of waiting for the next liveness ping. A rate-limited, mcp-loop-scoped gc-walk backstop (_maybe_disarm_orphaned_scopes) disarms any zombie minted by paths not yet migrated (the oauth_user pool keeps the old cross-task-close shape; follow-up). Also fixed: BaseExceptionGroup (BaseException-derived, as raised by anyio task groups wrapping a stray CancelledError, e.g. an accept-then-RST server) escaped `except Exception` in _connect_all and killed it before the health/sweep loops were created - silently disabling all autonomous recovery. Handled there and in the health/sweep/eviction loops and the reconnect/refresh callers. Verified: a live SIGKILL-flap repro went from 130%+ CPU (climbing, one armed scope per cycle) to 0.3% flat with zero armed scopes; the RST repro now leaves both background loops alive (previously both silently dead). New tests: owner-lifecycle + close-protocol units (incl. an exactly-one-cancel pin), a _connect_all BaseExceptionGroup regression, a discriminating disarm-sweep test, and a ~10s live SIGKILL-flap smoke test (real FastMCP subprocess, skips on environment gaps) asserting zero armed scopes, exactly one live owner, and a post-recovery tool call. Full 8470-test suite green; ruff+mypy clean. * fix(mcp): route pool transport lifecycles through per-entry owner tasks Completes the owner-task migration started for the static path: the oauth_user pool path had the same latent anyio cancel-scope exposure (host-task-bound scopes entered by short-lived connect tasks; a scope cancelled after its host finished re-delivers cancellation via call_soon forever - the 100%-CPU zombie), previously covered only by the disarm backstop. Each (user, server) pool entry's transport + ClientSession cms are now entered, parked, and exited by ONE long-lived owner task (_pool_transport_owner). The caller keeps building client_kwargs (the per-user bearer and, when an auth-capture carrier is active, the httpx_client_factory response hook) so 401/WWW-Authenticate capture semantics are unchanged. Teardown is the shared one-cancel close protocol (_teardown_pool_entry: signal before first await, graceful grace, at most ONE cancel), used by the connect stale-guard, idle/LRU eviction, and shutdown (parallel signal-then-reap). Unrequested owner death evicts the session but keeps the entry and its discovered catalog, matching the existing evict-session-keep-entry semantics the auth_401 retry relies on. Discovery still runs in the connecting caller while the transport is hosted by the owner, so a transport collapse mid-discovery (e.g. the SDK tearing its task group down on an upstream 401) cancels the OWNER, not the caller - a bare await on the response stream would hang until the 30s phase timeout. _await_pool_discovery races each discovery await against owner completion and converts owner death into a prompt ConnectionError (the owner is never cancelled there; teardown owns its lifecycle). Carrier-first failure classification preserves auth_401 semantics for captured 401s. With no cross-task stack closes left, _safe_close_stack and _safe_teardown_on_connect_failure are deleted (zero callers). Tests: new tests/test_mcp_pool_owner.py pins the pool close protocol (graceful event-before-await close, exactly-one-cancel escalation, owner-death eviction retaining entry+catalog, caller-cancel-mid-connect cm-exit guarantee, factory-present-iff-capture, and the owner-death-during-discovery fast-fail). 1010 mcp tests and the full 8471-test suite green, including the historical cross-task-anyio sentinel test_integration_pool_reuse_401_refresh_and_retry_succeeds; ruff+mypy clean; zero destroyed-task warnings. * fix(mcp): harden disarm-sweep loop guard and owner BaseException arm Review follow-ups on the owner-task migration: - _maybe_disarm_orphaned_scopes now enforces its mcp-loop requirement instead of trusting callers: it returns without walking (and without advancing the rate-limit clock) unless the currently running loop IS self._loop. A suppressed close can fire before start() or after shutdown(), where the walk would be wasted at best and a cross-thread reach at worst. - The transport owner's BaseException arm now re-raises non-Exception, non-group escapees (KeyboardInterrupt, SystemExit) after delivering them to the readiness future - failure delivery is the arm's job; swallowing an interpreter-level exit was not. * fix(mcp): extend owner-death discovery fast-fail to the static path The static connect path had the same exposure the pool's discovery race closed: discovery runs in the connecting caller while the transport is hosted by the owner task, so a transport collapse mid-discovery cancels the OWNER and the caller's bare await on the response stream hung until the caller-side attempt timeout (~45s) instead of failing promptly. _await_pool_discovery is renamed to _await_owner_discovery (it is now path-neutral) and wired into _connect_one_locked's four discovery awaits. The helper also converts a discovery future that completes CANCELLED without the race's own reap (an SDK-internal cancellation shape) into the same ConnectionError, instead of leaking a bare CancelledError the caller would misread as its own cancellation. The pool transport owner's BaseException arm gains the same refinement the static owner received in review: interpreter-level exits (KeyboardInterrupt, SystemExit) re-raise after delivery to the readiness future instead of being swallowed. Tests: static owner-death-during-discovery fast-fail (<1s vs the ~45s hang), and a direct pin on the cancelled-discovery-future conversion. * fix(mcp): replace owner BaseException arm with targeted catch + finally delivery The owner's failure arm now catches only (BaseExceptionGroup, Exception); waiter delivery for everything else moves to a finally that resolves the readiness future with a clean transport-failure ConnectionError before the task unwinds. Interpreter exits and BaseException-derived library control-flow escapes propagate from the owner exactly once, uncaught - and the waiter can never be left hanging on an unresolved future (the initial _connect_all connect has no outer bound). For SystemExit / KeyboardInterrupt asyncio additionally stops the loop right after, so the delivery is load-bearing for the non-exit BaseException shapes and free for the exits. Pinned by a test driving a BaseException-derived escape through the owner: the waiter resolves promptly with ConnectionError while the escape propagates unswallowed. * fix(mcp): mirror targeted-catch + finally delivery in the pool owner Same shape the static owner received in review: the failure arm catches only (BaseExceptionGroup, Exception), and waiter delivery for anything else moves to a finally that resolves the readiness future with a clean ConnectionError before the task unwinds - interpreter exits and BaseException-derived library escapes propagate exactly once, uncaught, and the waiter can never be left hanging. * test(mcp): narrow the escape test's waiter catch to explicit types * test(mcp): narrow discovery-race waiter catches to explicit types * refactor(mcp): make reap/synchronization awaits explicit to analyzers Full-absorb reaps (cancel-then-drain of a future whose outcome is deliberately consumed) become `await asyncio.gather(x, return_exceptions=True)` - one line, self-describing, and in the owner-died discovery reap it is also a small semantic improvement: a caller cancellation arriving during the reap now propagates instead of being masked by the ConnectionError. Bare synchronization awaits and selective suppress blocks in tests keep their raise-through semantics via throwaway assignment. Applied uniformly across the owner-task test files, including sites introduced by the static-path PR. |
||
|
Patrick Buckley
|
62f62ae624 |
fix(mcp): route static transport lifecycles through per-server owner tasks
A crash-looping MCP server drove the mcp-loop thread to a sustained, climbing 100%+ CPU spin. Root cause: anyio cancel scopes are host-task-bound, and the static path entered the SDK's transport / ClientSession task-group scopes from short-lived connect tasks (every health tick is a new task since #768). Once such a scope was cancelled after its host task had finished - by anyio's task_done when a transport child died with the server, or by ClientSession.__aexit__ during a cross-task teardown - CancelScope._deliver_cancellation could never make progress (task.cancel() on a done task is a no-op) and re-armed itself via call_soon every loop iteration, forever: ~900k callbacks/s per zombie scope, one more per flap cycle (verified against anyio 4.14.1; no upstream fix exists as of that release). Fix: each static server's transport + session cms are now entered, parked, and exited by ONE long-lived owner task (_static_transport_owner), so scopes always have a live host and always exit in the task that entered them. Teardown follows a one-cancel close protocol (signal the close event before the first await, graceful grace, then at most ONE cancel - never a second, which would abandon a scope exit mid-flight). Connect timeouts now cancel only the waiting caller; connect failures are delivered through a readiness future; unrequested owner death (server died under a live session) evicts the session immediately via a done-callback instead of waiting for the next liveness ping. A rate-limited, mcp-loop-scoped gc-walk backstop (_maybe_disarm_orphaned_scopes) disarms any zombie minted by paths not yet migrated (the oauth_user pool keeps the old cross-task-close shape; follow-up). Also fixed: BaseExceptionGroup (BaseException-derived, as raised by anyio task groups wrapping a stray CancelledError, e.g. an accept-then-RST server) escaped `except Exception` in _connect_all and killed it before the health/sweep loops were created - silently disabling all autonomous recovery. Handled there and in the health/sweep/eviction loops and the reconnect/refresh callers. Verified: a live SIGKILL-flap repro went from 130%+ CPU (climbing, one armed scope per cycle) to 0.3% flat with zero armed scopes; the RST repro now leaves both background loops alive (previously both silently dead). New tests: owner-lifecycle + close-protocol units (incl. an exactly-one-cancel pin), a _connect_all BaseExceptionGroup regression, a discriminating disarm-sweep test, and a ~10s live SIGKILL-flap smoke test (real FastMCP subprocess, skips on environment gaps) asserting zero armed scopes, exactly one live owner, and a post-recovery tool call. Full 8470-test suite green; ruff+mypy clean. |
||
|
Patrick Buckley
|
217d3a3a9b |
feat(mcp): autonomous reconnect + liveness for static MCP servers (#768)
* feat(mcp): autonomous reconnect + liveness for static MCP servers Static (non-oauth_user) MCP servers had no autonomous reconnect. Every reconnect path was lazy — a tool dispatch (_cb_auto_reconnect), an operator refresh, or a config edit — and the MCP SDK's own reconnect is a bounded 2-attempt burst on the streamable-http GET stream only (verified: mcp 1.28.1), with no backoff and nothing for the other transports. So a static server that went down and came back while nobody was dispatching to it stayed disconnected until a dispatch or a manual reconnect. Worse, a session whose transport dies while idle survives as a non-None ClientSession with closed streams — nothing evicts it, so even a later dispatch may not notice until it fails. Add a static-server health loop on the mcp-loop (started in _connect_all; config ``static_health_check_seconds`` default 30, <= 0 disables): - Reconnect: a disconnected server (session is None) is reconnected on a capped, jittered, FOREVER backoff (full jitter, base 1s, cap 60s, no attempt limit) — a server that returns after a long outage reconnects within ~a minute, and a permanently-misconfigured one costs at most one attempt per cap. The health loop owns this clock; the circuit breaker stays the DISPATCH fail-fast gate (a tool call to a down server errors immediately rather than blocking on the retry), and the loop keeps breaker state in sync so an open breaker closes on reconnect. - Liveness: a connected server is pinged (send_ping) each cadence; a dead-but- idle one — which nothing else would notice — is evicted so the next tick reconnects it. This is the core of the "never reconnects" failure. Serialize _connect_one per server behind a per-name lock (split into a thin wrapper + _connect_one_locked): the health loop, a dispatch's _cb_auto_reconnect, and an operator refresh could otherwise interleave teardown/rebuild on the shared StaticServerState and corrupt it — a latent pre-existing race this also closes. The body is unchanged (only relocated), so the delicate anyio / wait_for connect logic is untouched. Out of scope (follow-up): silent GET-stream / notification death — the SDK stops the notification stream after 2 attempts while the request path stays alive, so send_ping is blind to it; the fix is bounded session recycling, which needs a static in-flight guard first (only PoolEntryState tracks in_flight today). Tests: backoff bounds (capped / jittered / forever, no overflow), reconnect success resets backoff + closes breaker, reconnect failure retries forever, in-flight skip, per-name serialization (no overlap), ping keeps healthy / evicts dead / evicts on timeout, tick skips oauth_user, connect_all start + disable gating, clean cancel. * fix(mcp): harden static-server health loop (review findings) A max-effort review found 13 concurrency/correctness defects, all from the loop mutating shared StaticServerState without the interlocks the pool path carries. Fix all 13: - In-flight interlock: add StaticServerState.in_flight (parity with PoolEntryState); _static_session_op increments/decrements around the static call_tool/read_resource/get_prompt session ops; the ping skips and never evicts a busy server, so a long tool call can't be torn down mid-flight. - Dead-transport gating: the ping evicts + trips the breaker only on _is_dead_transport(exc); an McpError, httpx.PoolTimeout, or plain ping timeout is "slow, not dead" and only reschedules (matching the dispatch path). - Session-identity: only evict the exact session that was pinged. - asyncio.timeout (invariant-18) not wait_for for the ping; 5s->30s; the timeout-scoped cancel is distinguished from an external shutdown cancel (which still propagates) via .expired(). - Bounded reconnect: wrap _connect_one in asyncio.timeout so a server that handshakes then stalls list_tools can't wedge the loop or hold the per-name lock forever (connect internals untouched). - Concurrent tick under asyncio.gather with a freshly-read clock for the sleep. - Cross-path coordination: reconnect_sync/remove_server_sync take the per-name lock across teardown+rebuild (calling _connect_one_locked directly); _cb_auto_reconnect reuses a health-established session instead of racing a redundant reconnect and no longer trips the breaker on lock contention. - Backoff hygiene on recovery; skip '__' names; on health reconnect clear only the open-circuit deadline (not the failure count) so a connect-ok/calls-fail server still escalates to a trip. Adds 14 tests and adjusts those that assumed the old behavior; suite 195->209. * fix(mcp): unify static-server reconnect coordination (round-3 review) A third review round + live testing found 8 issues on the health loop, five sharing one root: reconnect logic was fragmented across five drivers, each handling the lock / session-reuse / in_flight / config-recheck / breaker / clock differently and incompletely. Introduce one primitive and route every lazy/autonomous driver through it. _ensure_static_connected(name, cfg) — the single lazy (re)connect path, all under the per-name lock: config re-check (+ lock-identity re-check, closing the remove->re-add race) so a removed server is never resurrected; reuse-if-live so a queued/concurrent driver never tears down and rebuilds a live session (the observed reconnect storm); in_flight guard so a reconnect can't tear down a session with a call still in flight on the evicted stack; bounded connect; and the circuit breaker owned in one place (clear the open-circuit deadline on success per finding-13, record one failure on real connect failure). Returns the session on success/reuse, None on a deliberate skip, raises on real failure. Routed through it: the health loop, a dispatch's _cb_auto_reconnect, and _refresh_all; operator reconnect_sync stays a deliberate force-rebuild. Also: fresh-clock deadlines (the stale tick-start clock was landing deadlines in the past and collapsing the backoff into an every-tick retry storm); loop-death fix (the tick no longer re-raises a CancelledError found in the gather results — per-server fallout, not shutdown; the loop returns only when Task.cancelling() marks a genuine shutdown); dispatch breaker records no failure on a sync-boundary reconnect timeout (lock contention is not a server failure; real outcomes recorded once, inside the primitive). Cleanups: extract _teardown_static_session (was copy-pasted 3x); share _capped_exponential between the breaker cooldown and the reconnect backoff. Adds 17 tests; test_mcp_client 204->226. * fix(mcp): coherent timeout hierarchy + round-4 review fixes A fourth review round on the unified reconnect coordination found 5 correctness regressions + 1 cleanup, five sharing one root: the inner reconnect attempt bound (45s) was LONGER than every caller wait (dispatch 30s, remove 15s, reconnect 30s), so a caller cancelling mid-attempt delivered a bare CancelledError that slipped past the primitive's `except Exception`. - Coherent timeout hierarchy: add _STATIC_RECONNECT_CALLER_TIMEOUT_S (> the inner attempt bound) for the dispatch + operator waits, so the inner asyncio.timeout always fires first — a clean TimeoutError the primitive converts, cleans up, and records on the breaker — instead of a caller cancelling a live attempt. Fixes [0] (half-discovered session left installed, served with a stale catalog) and [1] (breaker never trips via dispatch). - Primitive cancel-safe (belt-and-suspenders): its handler is now `except BaseException`, so even a bare CancelledError drops the partial session and records the failed attempt before re-raising. - Operator waits: reconnect_sync / remove_server_sync default timeouts raised above the reconnect bound; remove_server_sync now CANCELS the pending _remove on timeout (so it can't later pop a re-added entry and corrupt state) and reports failure instead of a false 'removed' ([2], [4]). - in_flight defer is gated on defer_if_busy: autonomous callers (health loop, _refresh_all) defer, but a DISPATCH reconnects rather than hard-fail a reachable server with an in-flight sibling ([3]). - Cleanup: extract _schedule_next_ping (was a copy-pasted triplet in 3 branches) [5]. Adds 6 tests; the lock-contention test now shadows the caller-timeout constant so it runs in ~1s instead of the full wait. * fix(mcp): address PR review feedback on static reconnect - _ensure_static_connected: skip breaker record on CancelledError (cancel proves nothing about the server; aligns docstring with impl) - reconnect_sync: add asyncio.timeout wrapper so discovery-phase stalls get a clean TimeoutError inside the lock - reconnect_sync: change except Exception to except BaseException so CancelledError from future.cancel() triggers catalog cleanup - reconnect_sync: null state.session on failure so a tool-less session isn't mistaken for a live one - _static_reconnect_one: use fresh monotonic clock for backoff gate instead of stale tick-start snapshot; remove dead now param - _static_health_tick: correct docstring (0.5s clamp prevents busy-spin, not 'no sleep through short backoff') - Test: new test for reconnect_sync timeout + catalog cleanup - Test: update cancelled-attempt test for new CancelledError semantics - Test: narrow except BaseException to except Exception + type hints - Test: fix typo 'Understone' -> 'Turnstone' - Test: remove unused stale_now variable; update mock signatures * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
Patrick Buckley
|
acc262c405 |
feat(mcp): proactively keep consented OAuth (OBO) tokens fresh for unattended work
Per-user OAuth (auth_type=oauth_user) token refresh is entirely lazy: a token is refreshed only when a tool is dispatched or a session binds the acting user, and a dead refresh token is discovered only when a dispatch fails. That assumes a human is driving the session, which breaks for autonomous / scheduled work acting on behalf of an absent user — the token may be expired (latency), in a transient-failure cooldown (unavailable), or the grant may be dead with nobody present to re-consent. The only periodic MCP-loop task, idle eviction, actively tears OBO connections down; nothing keeps tokens warm. Add a background token-freshness sweep that keeps every consented oauth_user grant hot WITHOUT keeping connections warm and WITHOUT mutating consent state on a timer. Sweep (_user_token_sweep_loop, default 240s): - Enumerates consented (user, server) grants from the token store and runs the canonical refresh path for each. Strictly oauth_user-scoped: gates on _oauth_user_server_names and drives off mcp_user_tokens rows, so a static / no-auth server — which has neither — is structurally invisible (no DB scan, no authorization-server round-trip, no MCP-server call). Never connects to the MCP server; connections stay lazy. - Observe-only: passes revoke_on_failure=False (new parameter on get_user_access_token_classified) so a timer NEVER deletes a token, emits token_revoked, or mutates the shared ambiguous-streak / cooldown. A dead grant is only surfaced (proactive dashboard pending-consent badge); the authoritative revoke stays on the lazy-dispatch path where a real user action justifies it. Because the row survives, a spurious server-wide invalid_grant (an AS maintenance window) self-heals — the badge is dropped on the tick the grant works again. - Keepalive: force-refreshes a grant whose refresh token has sat un-exercised past user_token_refresh_keepalive_seconds (default 1800s) even while the access token is still fresh, so a provider that ages out idle refresh tokens can't expire one between a user's real sessions. - Surfaces dead grants once per transition, pinning the pair only after a durable badge write so a failed persist retries rather than being lost. First sweep runs after a short startup grace so a restart surfaces a downed grant within seconds, not a full cadence later. Cadence <= 0 disables the sweep; a positive value is floored (30s) so a misconfigured tiny cadence can't turn the loop into a busy-loop. Reuses the per-key refresh lock, so a keepalive force cannot double-refresh against a concurrent dispatch. Storage: add list_mcp_user_token_reconcile_targets() returning (user_id, server_name, COALESCE(last_refreshed, created)) — expiry-unfiltered, no ciphertext projected — on the protocol, sqlite, and postgres backends. Tests: the sweep's no-auth invisibility (zero DB / AS calls with no oauth_user server), observe-only non-destruction (token kept and shared streak untouched on a background permanent / ambiguous failure), keepalive gating, badge persist-then-pin retry, self-heal on recovery, cadence clamp / disable, and the storage enumerator. |
||
|
Patrick Buckley
|
a7cab83dd1 |
fix(mcp): address pre-push review findings
A max-effort review of the branch before pushing surfaced six defects, several introduced by this branch's own commits. All fixed: [0]+[3] oauth priming (refined). Fully non-destructive priming never cleared a genuinely-revoked grant — the dead token stayed "consented", its tools never entered the catalog, and (bug) the PERMANENT branch returned before arming the cooldown, so every session re-hit the AS with a dead refresh token. Root cause: invalid_grant (PERMANENT) is a RELIABLE dead-grant signal (RFC 6749 §5.2), so deferring its revoke was net-harmful. Renamed the flag revoke_on_dead_grant -> revoke_ambiguous_escalation: priming now revokes genuinely-dead grants (permanent / expired-no-refresh) so the catalog isn't stranded cold behind a phantom token, and defers ONLY the sustained-UNCLASSIFIABLE (ambiguous) escalation to lazy dispatch — the case the "don't revoke an unused server's grant on a misclassification" concern actually applies to. The cooldown is armed before the ambiguous path, so the deferred case can't hammer the AS either. [1] server.py. _public_server_status (operator refresh/reconnect endpoints) didn't forward the new scope, so after per-user scoping every warm oauth_user server rendered disconnected/empty there. Now passes aggregate=True (operator / approve-scoped cluster view, matching the admin console). [5] _is_dead_transport. The widened httpx.TimeoutException swept in httpx.PoolTimeout — pool saturation, NOT a dead connection — so transient load would evict a healthy session and trip the shared breaker for all users. Narrowed to Connect/Read/WriteTimeout (kept NetworkError, RemoteProtocolError). [8] _is_dead_transport. The exact-message "session terminated" fallback still fired on a healthy session-owning server's protocol error with that message. The SDK-synthesized code 32600 is the only deterministic signal (the message is application-controlled), so match the code ALONE and drop the message fallback. [11] cleanup. The dead-transport except block was triplicated across call_tool_sync / read_resource_sync / get_prompt_sync — the exact drift this branch had to repair. Extracted _record_and_evict_on_dead_transport. Tests updated/added: prime revokes-permanent / defers-ambiguous (drives the real resolver both ways); PoolTimeout-is-not-dead; exact-"Session terminated"-message stays alive; _public_server_status aggregate. 836 test_mcp_* green, ruff + mypy clean. |
||
|
Patrick Buckley
|
b8addd55c0 |
fix(mcp): complete dead-transport handling + harden oauth_user status
Follow-up to
|
||
|
pow3rtool
|
f585c47b7d | mcp dead transport fix and token refresh | ||
|
Patrick Buckley
|
3e5f2c3870 |
test: zero out the suite's warning noise
121 warnings -> 0. Two upstream deprecations get narrowly-scoped filterwarnings entries (the mcp streamablehttp_client rename — adoption deliberately rides the v2 migration since the new entry point's call shape changes again there; the starlette httpx TestClient notice). The one real RuntimeWarning is fixed at the source: tests that mock asyncio.run_coroutine_threadsafe handed real coroutines to a stub that never awaited them, GC-firing 'coroutine was never awaited' inside whatever unrelated test ran later (the same cross-test bleed mechanism as the CI closed-stream spew — per-test filterwarnings markers cannot catch it, which is why two such markers existed and still leaked). A shared _dispatch_stub now closes real coroutines before returning the canned future; the obsolete markers are removed. |
||
|
Patrick Buckley
|
3f5ee333fb |
fix(mcp): close the shutdown drain race + close the owned loop
Review feedback: (1) gating the drain on a main-thread truthiness check of _background_tasks could skip cancellation when a spawn queued via call_soon_threadsafe had not reached the set yet — submit whenever the loop is RUNNING and snapshot on the loop, where FIFO callback order guarantees earlier-queued spawns have landed; (2) shutdown stopped the loop thread but never closed the loop or cleared _loop/_thread, leaking selector resources for embedders that cycle managers — close + clear when we own the thread and it actually stopped (loud warning when it does not); unowned loops (tests wiring _loop directly) stay untouched; (3) the bare await-in-suppress drain loops become asyncio.gather(return_exceptions=True) in both the shutdown drain and the test fixture. |
||
|
Patrick Buckley
|
6c48af1900 |
fix(mcp): track fire-and-forget background tasks; harden loop teardown
The post-reconnect catalog refresh was scheduled as a bare
asyncio.create_task: no strong reference (the task could be GC'd
mid-flight, so the refresh might silently never run) and no exception
retrieval (failures surfaced as "Task exception was never retrieved"
at GC time — in CI, onto an already-closed pytest capture stream, the
"I/O operation on closed file" spew; a suspected contributor to the
flaky 60-minute CI hangs via cross-test loop/task state bleed).
- _spawn_background(coro, label): tracked-task set + done-callback
that retrieves and logs failures at warning; discard runs LAST so
set-emptiness means "done AND reported"
- shutdown() drains tracked tasks FIRST, so stack teardown can't race
an in-flight refresh; same run_coroutine_threadsafe idiom and
timeouts as the existing close steps
- running_loop_mgr fixture: cancel-pending -> drain -> stop ->
join(5) with a loud assert -> loop.close() (was stop + silent
join(2), never closed)
- the false-property test ("swallows refresh failure" — nothing
swallowed it) now waits for completion and asserts the logged
warning via the patched module logger (structlog; caplog cannot
observe it), polling inside the patch context
|
||
|
Patrick Buckley
|
110d44b07e |
refactor(tools): remove man, math, and plan_agent built-in tools
`man` and `math` duplicated capabilities already reachable through `bash`; `plan_agent` is better expressed as a `task_agent` running a planning skill, and carried a large amount of special-case machinery (plan-review gate, refinement loop, per-kind model routing). Removing all three shrinks the tool surface and cuts per-call token cost. Also removed, as dead-once-the-tools-are-gone: - the `math` sandbox executor (`turnstone.core.sandbox`) and its `[sandbox]` extra; the eval analyst now runs bash-only - the read-only `AGENT_TOOLS` sub-agent tool set and the `agent` tool-metadata key (`task_agent`/`TASK_AGENT_TOOLS` retained) - the plan-review protocol end to end: the `on_plan_review` UI hook, `resolve_plan`, `POST /v1/api/plan` + `POST /v1/api/route/plan`, the `plan_review`/`plan_resolved` SSE events, and their Python SDK / TypeScript SDK / OpenAPI / frontend / Discord+Slack bindings - the `model.plan_alias` / `model.plan_effort` settings and the registry `plan_model` / `plan_effort` routing fields TOOLS 31->28, TASK_AGENT_TOOLS 13->11; COORDINATOR_TOOLS unchanged. BREAKING CHANGE: removes the `man`, `math`, `plan_agent` tools, the plan-review SSE/HTTP/SDK surface, and the plan_* model-routing settings from the experimental 1.6 line. |
||
|
Patrick Buckley
|
adeb10bc2c |
feat(mcp): admin status, deferred-consent persistence, operator docs (Phase 9) (#516)
* feat(mcp): admin status, deferred-consent persistence, operator docs (Phase 9)
Completes the OAuth-MCP build-out (Phases 0-8 shipped) by closing the
operator + deferred-consent gaps:
1. **Per-(user, server) deferred-consent persistence** — when a
non-interactive run (scheduled / channel) hits ``mcp_consent_required``
or ``mcp_insufficient_scope``, the sync pool dispatchers now upsert a
row into a new ``mcp_pending_consent`` table. The dashboard hydrates
the gear-icon badge from this table on load, so users who weren't
online to see the in-flight SSE prompt still surface the deferred
work on next login. Cleared automatically by the OAuth callback
handler on consent completion; manual user dismiss via new DELETE
endpoints. Composite PK ``(user_id, server_name)`` collapses repeat
occurrences for the same server — no NULLs-not-distinct trap.
2. **Admin status pill + bulk-revoke** — the MCP Servers admin row now
shows ``consented_users_count`` for ``auth_type=oauth_user`` rows
when ≥1, with a two-step-confirm ``bulk-revoke`` button that drops
every user's token for the server via the existing
``delete_mcp_oauth_rows_by_server_name`` primitive. Upstream RFC
7009 revoke is intentionally NOT attempted in bulk (avoids N
upstream HTTP calls per admin click); audit detail records
``upstream_revoke_outcome=bulk_admin_no_upstream``. A "last
refresh" pill (age + outcome) renders on each row, sourced from a
new ``_last_refresh`` dict populated by ``_refresh_server`` on every
call (both manual ``refresh_sync`` and the ``_cb_auto_reconnect``
follow-up).
3. **ClientType.SCHEDULED** added to the prompts module + scheduler
passes it through to ``create_workstream``. ``ChatSession`` now
computes ``_is_interactive_for_consent`` at construction (WEB / CLI
are interactive; CHAT / SCHEDULED are not) and plumbs the flag
through ``call_tool_sync`` / ``read_resource_sync`` /
``get_prompt_sync`` to the three sync dispatchers. The wrap at the
``_is_structured_error`` gate routes consent codes to the new
``_record_pending_consent_best_effort`` helper for non-interactive
callers only; interactive sessions stay on the in-flight SSE path
Phase 8 ships unchanged.
4. **Operator docs** — ``docs/mcp-oauth.md`` (operator guide, parallel
to ``docs/oidc.md``: ``auth_type`` choice, OAuth client setup,
encryption-key rotation, troubleshooting matrix) and
``docs/operations/mcp-oauth-headless.md`` (one-paragraph runbook
per ``feedback_runbook_trust_llm.md``: pre-consent recipe for
scheduled / channel-driven runs).
Schema
- Migration 054_mcp_pending_consent.py — composite PK
``(user_id, server_name)``, ``occurrence_count`` + ``first_seen_at`` /
``last_seen_at`` for recency metadata, ``idx_mcp_pending_consent_user``
for the badge-load query. No FKs (matches the rest of the
oauth_user schema).
- Migration 055_mcp_user_tokens_server_index.py — adds
``idx_mcp_user_tokens_server`` on ``(server_name, expires_at)`` so
the admin pill's ``count_mcp_consented_users_*`` queries don't
full-scan against the leading-``user_id`` composite PK.
- Cross-backend: works on SQLite + PostgreSQL via dialect-specific
``on_conflict_do_update`` (PG ``postgresql.insert`` / SQLite
``sqlalchemy.dialects.sqlite.insert``). No ``NULLS NOT DISTINCT``
needed — the simplified PK eliminates the cross-version trap.
Endpoints
- ``GET /v1/api/mcp/oauth/pending`` — list deferred-consent records for
the authenticated user. Install-level gate via cached
``any_oauth_user_mcp_servers`` short-circuits to ``{pending: 0}`` on
installs with no oauth_user MCP servers — local-auth deployments
exercise zero new storage queries on this path. The gate result is
cached on ``app.state`` with a 60s TTL to spare repeat dashboard
loads.
- ``DELETE /v1/api/mcp/oauth/pending/{server_name}`` — single dismiss.
Returns 204 in both existed-and-deleted and never-existed cases
(no cross-tenant existence leak); audits
``mcp_server.oauth.pending_consent_dismissed`` with
``mode=single`` + ``cleared=0|1`` so a session-hijack attacker
scrubbing breadcrumbs leaves an audit trail.
- ``DELETE /v1/api/mcp/oauth/pending`` — bulk dismiss; audits
``mode=bulk`` + ``cleared=N``.
- ``POST /v1/api/admin/mcp-servers/{name}/bulk-revoke`` — admin
bulk-revoke for the named server's per-user tokens. Requires
``admin.mcp`` permission + 400s when the row isn't ``oauth_user``.
All four registered on both ``turnstone-server`` and
``turnstone-console`` (mirrors the Phase 8 ``/connections`` endpoint
shape).
Performance
- Admin list handler now uses a single ``GROUP BY`` bulk-count query
(``count_mcp_consented_users_grouped_by_server``) wrapped in
``asyncio.to_thread`` rather than N per-row sync DB round-trips
inside the async handler. Skipped entirely when no row is
oauth_user.
Frontend
- ``ui/static/app.js``: ``loadPendingConsents()`` hydrates the
existing ``_pendingConsentServers`` set on dashboard init + after
the user opens the settings modal. Endpoint failures stay silent
— the badge will be re-driven by the next in-flight tool error.
- ``console/static/admin.js``: ``consented_users_count`` pill +
``bulk-revoke`` button on each MCP row (only when ≥1 consented),
two-step confirm matching the existing delete pattern. ``last-
refresh`` age + outcome pill in the per-row status cell, sourced
from the freshest per-node entry in ``status[*].last_refresh_at`` /
``last_refresh_outcome``. CSS for the pills in ``style.css``.
Tests
- ``test_mcp_pending_consent_storage`` — 13 tests covering upsert
idempotency, list ordering, per-user isolation, single/bulk delete,
count-by-server + grouped variant, install-level gate.
- ``test_mcp_pending_consent_dispatch`` — 9 tests, including the
boundary-cross gate per ``feedback_tests_through_boundaries.md``:
drives the real ``call_tool_sync`` → ``_dispatch_pool_sync`` →
``_is_structured_error`` → ``_record_pending_consent_best_effort``
with a mocked classified-lookup so the structural plumb-through is
verified end-to-end. Includes a storage-failure test that pins
the docstring's "envelope unchanged on storage failure" promise.
- ``test_mcp_pending_consent_endpoints`` — 11 tests: install gate,
list-for-self, no-cross-user-leak, single/bulk delete, idempotent
not-found, audit emission on single + bulk + cross-tenant dismiss.
- ``test_chat_session_interactivity_flag`` — 7 tests pinning the
``ClientType`` → ``_is_interactive_for_consent`` mapping against
the module-level ``INTERACTIVE_CONSENT_CLIENT_TYPES`` frozenset.
- ``test_mcp_admin_bulk_revoke`` — 7 tests covering admin.mcp
permission gate, 404 on missing, 400 on non-oauth_user, 200 with
``rows_deleted`` + ``consented_users_before``, audit row with
``upstream_revoke_outcome=bulk_admin_no_upstream``, cross-server
isolation.
- ``test_mcp_oauth_handlers`` — 2 new callback tests pin the post-
callback ``delete_mcp_pending_consent`` invocation: success-clears
+ storage-failure-still-redirects.
- 636 tests pass on the impacted surface (47 new + Phase 0-8 OAuth-MCP
+ session + prompts + storage admin). ruff + mypy clean.
Hard invariants honored
- Static path byte-identical for ``auth_type ∈ {none, static}`` — the
flag flows only through the pool dispatchers, which only fire when
the row resolves to ``oauth_user``.
- ``asyncio.timeout`` (not ``asyncio.wait_for``) preserved on every
AS / SDK / pool-loop await — no new awaits added to the hot path.
- Install-level gate on the badge endpoint: cached
``any_oauth_user_mcp_servers`` returns False on a row-less
deployment → endpoint short-circuits without touching the pending-
consent table; 60s TTL bounds the staleness window after admin
flips ``auth_type``.
- Operator-actionable codes (key-unknown, url-insecure, *_forbidden)
explicitly filtered out of persistence — they're outside the
user-facing consent badge scope.
- Best-effort write: the structured-error envelope returned to the
agent is identical whether the persistence write succeeds or fails
(storage exception is logged with type name only — no chained
context that could carry an ``httpx.Request`` bearer header).
- No ``exc_info=True`` on any new path that can chain a bearer-bearing
``httpx.Request``.
- Defensive parsing: ``_parse_pending_consent_envelope`` mirrors
``_is_structured_error``'s ``isinstance(decoded, dict)`` guard plus
filters scope tokens through ``is_valid_scope_token`` capped at
``MAX_INSUFFICIENT_SCOPE_REPORTED`` — defense-in-depth even though
production callers already validate upstream.
- Audit events on every dismiss endpoint so a session-control attacker
scrubbing dashboard breadcrumbs still leaves a trail.
Cross-backend
- Tested on SQLite via the conftest backend fixture.
- PostgreSQL path uses ``postgresql.insert(...).on_conflict_do_update``
parallel to the existing ``mcp_user_tokens`` upsert in Phase 3.
Deferred (not Phase 9 blockers)
- Multi-node pool eviction on bulk-revoke: only local-node sessions
would be evicted if we built it, and there's no bulk-by-server
primitive on MCPClientManager today; remote nodes will surface as
a 401 on next dispatch which refreshes through the (now empty)
token row.
- RFC 8693 / Azure OBO ``auth_type=oauth_token_exchange`` — captured
in the design doc as a future architectural direction (~600 LOC +
IdP-side admin work); requires OIDC token capture and per-MCP-server
resource-trust configuration that v1 does not ship.
* docs(mcp): address Copilot review feedback on Phase 9
- Fix misleading admin.js comment that claimed the refresh pill rendered
"<short-relative> <outcome>" — the pill actually renders only the short
age, with outcome reflected via CSS class and tooltip.
- Replace broken feedback_secrets_not_in_env.md repo-root link in
mcp-oauth.md with the inlined rationale (env-borne secrets reachable
via shell tools / os.environ; TOML secrets are not).
|
||
|
Patrick Buckley
|
a8b34bfe54 |
feat(mcp): per-user catalog scoping (Phase 7 — tools)
Light up production reachability of pool dispatch (RFC §3, invariant 8)
by widening the public catalog API to optionally take a ``user_id``:
- ``MCPClientManager.get_tools(user_id=None)`` returns the merged
static + per-user pool view when ``user_id`` is supplied; the default
preserves the legacy global-only contract.
- ``is_mcp_tool(name, *, user_id=None)`` extends the lookup to the
per-user ``_user_tool_map``. Pool tools become reachable from
``ChatSession._prepare_tool`` only when the session-bound user_id
flows through — flipping invariant 8 from "must hold" to "satisfied".
- Listener identity becomes ``(user_id, callback)``. Static-path
changes fire ALL listeners (admin + every user); pool-entry
changes fire only matching-user + admin (``None``) listeners.
RFC §3.3.
- Pool sessions discover their tool list on first connect
(``_connect_one_pool`` → ``await session.list_tools()``); the
notification closure binds to ``(user_id, server_name)`` so
push-driven ``list_changed`` updates target the correct user's
catalog. R6 verified empirically: ``list_tools()`` 401 propagates
through anyio TaskGroup unwinding, no hang — plain ``await`` is
fine, no carrier-race shape needed for discovery.
- ``_evict_session`` drops ``entry.tools`` and rebuilds the user's
index so an evicted-then-reconnected session doesn't carry
stale catalog state.
- ``web_search.resolve_web_search_client`` refuses
``auth_type=oauth_user`` backends (per-node web search can't
carry per-user tokens).
Resources / prompts pool dispatch deferred to Phase 7b — invariant 8
is satisfied by the tool path alone, and the resource/prompt path
needs sibling ``_dispatch_pool_resource_sync`` /
``_dispatch_pool_prompt_sync`` helpers each with their own
carrier-race plumbing (~400 LOC). Phase 7b will follow the patterns
established here.
CLI sessions default ``user_id=""`` and so cannot use oauth_user
MCP servers — documented limitation; users must use the web UI.
Round-1 review fixes (4-finder review applied, no push yet):
- bug-1: get_tools(user_id) was iterating _user_pool_entries from sync
threads while the mcp-loop concurrently mutated it (RuntimeError:
dictionary changed size during iteration). Now reads from a sibling
_user_tools dict updated atomically by _rebuild_user_tool_map.
- bug-2: _close_pool_entry_if_idle (LRU/TTL eviction) skipped the
catalog cleanup that _evict_session does — stale tools persisted
in _user_tool_map and ChatSession's tool list never rebuilt. Now
mirrors _evict_session.
- perf-1: _last_pool_notification_refresh debounce dict was never
pruned in either eviction path. Now popped alongside the entry.
- perf-3: web_search resolver was issuing a sync SQL query per LLM
turn to gate oauth_user backends. Now reads from the cached
in-memory config.
- sec-1: bearer token could leak into exc_info-rendered tracebacks
via Sentry/faulthandler. log.debug now uses structured fields,
not exc_info.
- sec-2: tools-per-server response now capped at 1000 (defensive,
mirrors _MAX_ERROR_LEN / _MAX_INSUFFICIENT_SCOPE_REPORTED).
- Test cleanup: dropped two listener fan-out tests duplicating
test_mcp_client.py coverage; renamed test_pool_session_notification_handler
to match its actual scope (_refresh_pool_server_tools); removed
stale comments referencing /tmp/r6-spike*.py scratchpads and a
misleading "copy-on-write" comment.
Round-2 pre-push review fixes (focused single-pass review applied):
- round2-1: bug-2's catalog-cleanup block in _close_pool_entry_if_idle
had no integration test (exactly the failure mode flagged in
feedback_tests_through_boundaries.md). Added
test_close_pool_entry_if_idle_clears_catalog_and_fires_listener
driving the LRU/TTL eviction path through real streamablehttp_client +
MockTransport. Negative-test verified: reverting the
_rebuild_user_tool_map / _notify_user_tool_listeners calls makes
the new test fail.
- round2-3: documented the _oauth_user_server_names cache invariant
in add_server_sync / remove_server_sync docstrings. Cache is
reconcile_sync's sole owner — direct callers leave it stale, but
_db_servers_to_config strips oauth_user rows so production paths
are unaffected. Static→oauth_user transitions correctly leave the
name in the cache because remove_server_sync drops the static
connection, not the cache identity.
- round2-6: strengthened test_rebuild_user_tool_map_populates and
test_rebuild_user_tool_map_drops_empty_user to assert on the
_user_tools sibling cache (bug-1 fix). Without this, a future
revert dropping the sibling write would still pass the unit
tests because get_tools coverage lives in separate tests.
Round-3 full-stack review fixes (multi-stage review on the final
state caught what the layered apply passes missed):
- q-1 REGRESSION: pool tool-discovery used asyncio.wait_for around
session.list_tools(), the exact pattern the
|
||
|
Patrick Buckley
|
4db7d9c6cf |
feat(mcp): per-(user, server) ClientSession pool with OAuth dispatch
Phase 5 of OAuth-MCP — adds a per-(user, MCP-server) ClientSession
pool to MCPClientManager alongside the existing static-server path,
gated entirely on the per-server `auth_type='oauth_user'` config.
Pool architecture:
- `_user_pool_entries: dict[(user_id, server_name), PoolEntryState]`
with lazy connect on first dispatch, per-key asyncio.Lock allocated
on the mcp-loop, idle eviction coroutine (default 600s TTL, LRU cap
200), and an `in_flight` counter as the eviction interlock so live
calls can never be torn down mid-flight.
- `_dispatch_pool` runs the token-state machine: missing token →
`mcp_consent_required`; key-rotation decrypt failure →
`mcp_token_undecryptable_key_unknown` with NO consent prompt and NO
auto-delete; expired token → silent refresh under per-(user, server)
advisory lock; refresh failure → revoke + consent.
- `_classify_failure` separates transport (trips breaker) from auth
401/403 (does NOT trip breaker — server-only invariant) from
protocol (no breaker change).
- `entry.open_lock` held only across connect-or-reuse and released
before the `await session.call_tool` so concurrent calls from one
user against one server overlap (validated by Spike 1 scenario 2).
Auth-class failures are fail-soft in Phase 5: any 401/403 surfaced by
the SDK propagates to the agent as a tool error and the next dispatch
reconnects on a fresh refresh. Real introspection of upstream 401/403
is a Phase 6 concern — the MCP SDK's `streamable_http` post_writer
swallows `httpx.HTTPStatusError` upstream, so detecting status from
the response chain requires `McpError(CONNECTION_CLOSED)` payload
parsing or a custom httpx middleware around `streamablehttp_client`.
The mid-flight 401 refresh-retry path and the `mcp_insufficient_scope`
structured error for 403 step-up land together in Phase 6, gated by
an integration test that drives a real upstream 401/403 (the unit-
test injection of `HTTPStatusError` is what masked the production gap
on the first apply-findings pass — the integration test is the
structural gate so the gap can't reopen). RFC §1.5 steps 4-5 and the
phase table in §Implementation phases reflect this scope split.
Multi-node refresh contention:
- New `StorageBackend.acquire_advisory_lock_sync` Protocol method.
SQLite returns nullcontext (single-node, in-process asyncio.Lock
is sufficient). Postgres uses `pg_try_advisory_xact_lock` with
retry on a fresh per-attempt connection, so waiters don't pin pool
connections during the AS roundtrip. Inner try/except + nested
finally ensures conn is always returned to the pool, even when
begin / execute / yield / commit raises mid-body.
- Lock ordering: pg_advisory outer, asyncio.Lock inner. Re-read after
lock collapses cluster-wide contention to one HTTP roundtrip per
(user, server) per refresh window.
- `_PgRefreshLock` enter/exit pinned to a single-worker
ThreadPoolExecutor so SQLAlchemy connection state stays
thread-affine across cancellations.
Token storage refactor:
- `get_user_access_token_classified` returns a tagged TokenLookupResult
(Token / MissingToken / DecryptFailure / RefreshFailed) so the
dispatcher maps each state to the right user-facing error.
- `get_user_access_token` is now a thin wrapper around the classified
variant; the previous duplicated state machine is gone.
Security:
- Pool dispatch + admin endpoints reject `http://` URLs for
`auth_type='oauth_user'` servers (only exact loopback hostnames are
exempt — `*.localhost` is intentionally NOT honored because RFC 6761
localhost-zone resolution is configuration-dependent and could route
bearers to non-loopback IPs via custom resolvers / hosts file /
Docker overlays). Validated at three layers:
`_dispatch_pool` (structured `mcp_oauth_url_insecure` error),
`_connect_one_pool` (defensive ValueError), and
`admin_create_mcp_server` / `admin_update_mcp_server` (400 before
storage write).
- Admin URL change on an oauth_user row purges per-user OAuth tokens
bound to the old URL: bearers are bound (via OAuth resource /
audience) to the URL active at consent time, so silently rebinding
them to a new URL is a token-binding violation. Re-consent forces
fresh issuance for the new resource.
- Encryption-key fingerprints stay in audit logs only; no longer
surfaced in agent-facing error payloads.
User_id thread-through:
- `MCPClientManager.call_tool_sync(..., user_id=None)` (additive;
default None preserves the static path byte-identically).
- `ChatSession._exec_mcp_tool` passes `self._user_id or None`.
- `set_app_state(app_state)` setter wires OAuth state at lifespan
startup, called from both turnstone-server and turnstone-console.
Performance:
- LRU cap eviction iterates `_user_pool_entries` (not
`_user_pool_last_used`) so pre-dispatch entries are eligible.
- Eviction batch closes via `asyncio.gather` instead of serial await.
- `_resolve_pool_target` returns the resolved server row to
`_dispatch_pool` to eliminate the second DB lookup.
- Production reachability of pool dispatch is gated on Phase 7
(catalog scoping) wiring pool tools into `_tool_map`; until then
pool dispatch is reachable only via direct `call_tool_sync` with a
prefixed name (the path the new pool tests exercise).
Hardening parity preserved:
- Static path (auth_type ∈ {none, static}) byte-identical; PR #296
hardening (SDK #2147 mitigations, anyio cancel-scope, stale-session-
and-stack guard, server-only circuit breaker) intact.
- `test_reconnect_preserves_static_state_identity` unchanged + green.
- `MCPTokenStore.get_user_token` does not auto-delete on
MCPTokenDecryptError (key-rotation safety).
- Notification debounce stays manager-level.
- Connect-failure cleanup factored into
`_safe_teardown_on_connect_failure` shared by both connect paths.
Tests: 5475 → 5493 (+18). New file `tests/test_mcp_user_pool.py`
plus additions to test_mcp_oauth_refresh.py, test_mcp_admin_api.py,
and test_mcp_client.py covering: pool data structures, lazy connect,
eviction TTL + LRU + lock interlock, dispatch state machine (token
states), failure classification, http-rejection at dispatch and
admin layers, URL-change-purges-tokens (sec), concurrent dispatch on
one (user, server), pg_advisory lock parity, and user_id threading.
Phase exit criterion (synthetic load test 50 users × 3 servers × LRU
30 × 1000 calls × 200 evictions) deferred to a post-Phase-5 fitness
spike that runs against a staging deployment with real FDs and real
network behaviour, not a CI mock — same shape as Spike 1's
pre-Phase-0 SDK validation.
Out-of-scope for Phase 5 (Phase 6+): SDK-level 401 refresh-retry +
403 `mcp_insufficient_scope` (Phase 6), per-user catalog scoping
(Phase 7), consent UX SSE event + dashboard renderer (Phase 8),
admin UI status indicators (Phase 9).
|
||
|
Patrick Buckley
|
29c42c1427 |
feat(mcp): per-(user, server) OAuth 2.1 + PKCE flow
Lands the OAuth flow that uses the token-at-rest store from the prior
commit: discovery (RFC 9728 PRM + RFC 8414 AS metadata with operator-
override precedence), PKCE S256 (mandatory — refuse AS without it),
RFC 8707 resource indicator on every authorize and token request,
RFC 7591 minimal one-shot dynamic client registration, authorization-
code exchange, refresh-token grant with re-read-after-acquire single-
flight lock, and the /v1/api/mcp/oauth/{start,callback} endpoints
mounted on both server and console.
Refactored:
- validate_url_no_ssrf, validate_discovered_endpoint, is_localhost,
effective_port, sanitize_log_text moved out of oidc.py into a shared
oauth_ssrf module; oidc.py re-exports for compatibility. The shared
helpers also expose async wrappers (validate_url_no_ssrf_async,
validate_discovered_endpoint_async) so OAuth-MCP discovery — invoked
from async handlers — does not block the event loop on the
synchronous socket.getaddrinfo call.
- MCPTokenStore.get_oauth_client_secret reader path added (the prior
commit was write-only)
- Storage protocol gains create/pop/cleanup_*_mcp_oauth_pending_state
and get_mcp_oauth_client_secret_ct (mirror OIDC pending-state
pattern: SQLite BEGIN IMMEDIATE select-then-delete, Postgres atomic
DELETE...RETURNING)
Refresh-grant correctness:
- When the AS omits refresh_token (RFC 6749 §6 — MAY rotate), the
existing refresh value is preserved at the OAuth-flow layer rather
than cleared, so production ASes (Google, Auth0 default, Okta) don't
force re-consent every hour
- expires_in accepts int, float, str-with-decimal — earlier int-coerce
through str() failed on float and silently dropped expiry tracking
- The refresh-grant `resource=` parameter (RFC 8707) is the canonical
MCP server URL, not the audience. Audience and resource are distinct
concepts; using audience as resource would mismatch the AS RS
allowlist.
Audience handling:
- _validate_token_audience accepts str or tuple; the callback resolves
accepted_audiences = {server_url, oauth_audience} and validates
against the set, so Auth0-style ASes that honor `audience=` (not
RFC 8707 `resource=`) issue tokens that pass audience-bound
validation
- build_authorize_url emits both `resource=` (RFC 8707) and
`audience=` (Auth0-style) per server config; comment documents which
AS implementations need which form
Security hardening:
- redirect_uri pinned to oidc_config.redirect_base instead of the
request Host header — closes the same Host-header injection PR #476
fixed for OIDC. Both /start and /callback return 503 with operator-
actionable hint when redirect_base is unset
- DCR registration runs under per-server asyncio.Lock with re-fetch
inside the lock, so concurrent /start callers don't both register
and overwrite each other's client_id (the second user's code is no
longer rejected on callback)
- /callback error branch pops the pending state row before redirecting
so a leaked state can't be replayed against a separately-obtained
code in the 60s cleanup window
- WWW-Authenticate Bearer parser handles RFC 7235 quoted-string
escapes (\" and \\) instead of the naive [^"]+ regex
- AS-controlled response bodies and error_description query params go
through sanitize_log_text before reaching exception messages or
audit details. AS error responses are parsed for the standard
RFC 6749 fields (error, error_description, error_uri), each
capped at 80 chars and run through redact_credentials to defend
against ASes that echo the request body back into their error
payload.
- oauth_as_issuer_cached is re-validated against the SSRF guard on
read; on rejection the column is cleared and PRM rediscovery runs
- DCR / token-endpoint / refresh-endpoint response bodies cap at 64
KiB (PRM/AS metadata cap stays at 256 KiB) so a hostile or
malfunctioning AS can't exhaust client memory.
- oauth_client_secret operator input capped at 1024 chars at the
admin-form boundary; longer plaintext rejected with 400.
- /start and /callback responses stamp `X-Frame-Options: DENY` so the
redirected pages can't be framed by attacker sites.
- delete_user cascades to mcp_user_tokens and mcp_oauth_pending so
user deletion no longer leaves dangling per-user OAuth state.
- Renaming or deleting an oauth_user MCP server purges per-user
tokens and pending OAuth state for the previous server name
(delete_mcp_oauth_rows_by_server_name). The OAuth tables key on the
mutable server_name; without this purge, a future server with the
same name (and an attacker-controlled URL) would silently rebind
prior user tokens. A future schema migration will replace the
server_name key with a server_id FK + ON DELETE CASCADE.
- get_user_access_token catches MCPTokenDecryptError (raised when no
installed key can decrypt the row, e.g. after key rotation) and
falls through to None so dispatch surfaces a re-consent rather than
crashing.
- oauth_user MCP server rows are skipped in the static auto-connect
path. Auto-connecting them at startup with empty headers fails the
AS check and trips the circuit breaker; per-user tokens come online
lazily once the user has consented.
Audit (mcp_server.oauth.* prefix):
- consent_started, consent_completed, consent_failed, token_refreshed,
token_revoked, dcr_registered. _audit_event is async and wraps
record_audit in asyncio.to_thread so the audit write doesn't block
the event loop. resource_id on the audit row is the immutable
server_id (PK UUID) so admin-driven server renames don't break
event correlation; server_name is exposed in detail for cross-
reference. dcr_registered detail.has_secret reflects whether the
DCR-issued secret was actually persisted (the prior code reported
has_secret=true even on persistence failure).
- _admin_mcp_action audits the immutable server_id, not the mutable
server_name (which is what the column is — the table's PK was
always server_id).
- All OAuth-flow log keys use the mcp_server.oauth.* prefix to match
the audit-action taxonomy.
Lifespan close-order in turnstone.server and turnstone.console.server
is reversed (LIFO) — mcp_oauth → mcp_crypto → oidc — to match init
order.
Deferred until the upcoming per-user pool integration:
- Multi-node refresh-lock contention via pg_advisory_lock
- DCR re-register on token-endpoint 401 (the dispatch path surfaces
those 401s)
- TTL-LRU caching of decrypted plaintext access tokens
- DNS-rebinding hardening (httpx Transport pin) — documented as
limitation in oauth_ssrf module docstring
Tests: 7 new test files / ~85 new tests covering discovery precedence
+ PRM quoted-string parsing, PKCE round-trip, SSRF helper extraction,
authorize/callback handlers including 503-on-no-redirect-base + DCR
concurrency + JWT audience polymorphism + callback-error-pops-pending,
refresh single-flight lock, refresh resource-vs-audience regression,
decrypt-error fallthrough, _db_servers_to_config skipping oauth_user,
pending-state CRUD round-trip.
|
||
|
Patrick Buckley
|
be0950bb98 |
refactor(mcp): consolidate per-server state into StaticServerState dataclass
Phase 0 of the OAuth-MCP RFC: prepare MCPClientManager for the per-(user, server) session pool that lands in Phase 5, without changing static-path behavior. Two changes: 1. Hardening helpers _pre_close_streams and _tcp_probe rename their first parameter from `name` to `key`. Type stays `str` for now; widening to `str | tuple[str, str]` happens in Phase 5 when callers actually pass tuples. _safe_close_stack takes the stack directly and is unchanged. 2. The eleven parallel name-keyed dicts (_sessions, _per_server_stacks, _per_server_tools, _per_server_resources, _per_server_prompts, _supports_list_changed, _supports_resources, _supports_resource_list_changed, _supports_prompts, _supports_prompt_list_changed, _server_streams) are consolidated into _static_servers: dict[str, StaticServerState]. Server- level state (circuit breaker, notification debounce, last-error, db-managed, merged catalog maps, listener lists) stays on the manager, unchanged. PoolEntryState is defined for Phase 5 use but no code instantiates it. The typed map declarations (dict[str, StaticServerState] vs dict[tuple[str, str], PoolEntryState]) make accidental cross-keying lookups easier to catch. PR #296 hardening preserved exactly: - pre-close-streams atomic take-and-clear before stack teardown - stale-session-and-stack guard at _connect_one top: both state.session and state.stack checked, cleared independently, entry preserved (not popped) - transport-error session-eviction in dispatch sets state.session=None only, leaving stack/streams for the next connect-time guard sweep - _safe_close_stack CancelledError suppression unchanged - TCP probe before streamablehttp_client unchanged - future.cancel() after TimeoutError in all sync bridges unchanged - notification debounce stays manager-level (not migrated into the dataclass) Refresh helpers (_refresh_server_tools/_resources/_prompts) snapshot state.session into a local immediately after the None guard so concurrent transport-error eviction during await cannot null the session reference mid-call. Tests: shared _seed_static_state helper in tests/conftest.py replaces eleven direct dict mutations; new test_reconnect_preserves_static_state_identity guards the entry-preservation invariant. Pass count rises 5266 → 5267. |
||
|
Patrick Buckley
|
eb2a119da9 |
refactor(mcp): remove periodic refresh, add manual refresh/reconnect controls
Deletes the _periodic_refresh task and its supporting state
(_refresh_task, _refresh_failures, _refresh_backoff_until,
_REFRESH_BACKOFF_BASE/MAX, _DEFAULT_REFRESH_INTERVAL, refresh_interval
kwarg) from MCPClientManager. Push notifications and operator-driven
manual refresh now cover all catalog-update needs; the long-running
4-hour timer was dead complexity that obscured the per-user pool
work to come.
Catalog freshness on auto-reconnect is preserved by scheduling an
unblocking _refresh_server task on the mcp-loop after _connect_one
succeeds; the calling thread returns immediately so half-open
recovery latency does not double. Adds MCPClientManager.reconnect_sync
(clears the circuit, closes any existing session, calls _connect_one,
clears stale catalog on failure).
Wires a new pair of operator endpoints —
POST /v1/api/admin/mcp-servers/{name}/refresh and
/v1/api/admin/mcp-servers/{name}/reconnect — that fan out to all
nodes through the existing _internal route family, with per-row
"Refresh" and "Reconnect" buttons in the MCP Servers admin tab.
The new node-internal paths /api/_internal/mcp-{refresh,reconnect}/
are gated to the approve scope to prevent direct unprivileged
reconnects bypassing the console's admin.mcp gate. Internal
endpoints return generic error messages and a filtered status
payload (no command/url) to keep transport details admin-gated.
Drops the [mcp] refresh_interval setting, the
--mcp-refresh-interval CLI flag, and the matching config-mapping
entry; updates docs/architecture.md, docs/tools.md,
docs/settings.md, and the three PlantUML diagrams that referenced
the periodic loop.
Tradeoffs (intentional):
- Idle nodes will not auto-rejoin a recovered MCP server until
traffic arrives or an operator clicks Reconnect. The previous
background reconnection loop is gone by design — push
notifications + operator controls replace it.
- Console fan-out blocks on the slowest node (existing pattern);
not changed here.
This is Phase 1 of the OAuth-MCP series — feature subtraction
ahead of per-user state.
|
||
|
Patrick Buckley
|
e42add1b77 |
feat(coordinator): coordinator workstream kind — phase 1 (#368)
* feat(coordinator): coordinator workstream kind — phase 1
Adds a new ``kind="coordinator"`` workstream that runs inside the
``turnstone-console`` process (first ChatSession hosted on the console)
with a dedicated tool set for spawning and driving child workstreams.
Supersedes the external ``turnstone-coordinator`` MCP side-car for new
installs; the extension is marked deprecated in
``examples/mcp-cluster-ops/README.md`` but still works on 1.4-and-earlier
clusters.
Phase 1 ships: the workstream class, 6 lifecycle tools, console hosting,
9 HTTP endpoints, per-user audit attribution, and a one-pane web UI at
``/coordinator/{ws_id}``. Node/skill discovery tools, task-list tool,
tree-view UI, and routing-proxy audit middleware follow in a later PR.
## Schema
Migration 039 adds ``kind`` / ``parent_ws_id`` columns + indexes to
``workstreams``. Both SQLite and PostgreSQL backends take the new
kwargs on ``register_workstream``; empty-string ``parent_ws_id``
normalises to ``NULL`` at the storage edge. PostgreSQL uses
``INSERT ... ON CONFLICT DO NOTHING`` to match SQLite's ``OR IGNORE``
and close a pre-existing SELECT-then-INSERT TOCTOU window.
``list_workstreams`` gains optional ``parent_ws_id`` / ``kind`` filters;
new ``get_workstream(ws_id)`` returns the full row (the existing
``get_workstream_metadata`` stays untouched for back-compat).
## Core session + kind routing
- ``ChatSession.__init__`` accepts ``kind`` / ``parent_ws_id`` /
``coord_client``. On ``kind="coordinator"`` it swaps
``_tools = COORDINATOR_TOOLS`` and zeros sub-agent tool lists.
- ``Workstream`` dataclass extended with ``user_id`` / ``kind`` /
``parent_ws_id``. Both ``WorkstreamManager`` and the new
``CoordinatorManager`` use the same type — no parallel hierarchy.
- ``_SessionFactory`` Protocol + server / cli factory closures thread
the new kwargs. ``POST /v1/api/workstreams/new`` rejects
``kind != "interactive"`` with 400; ``POST
/v1/api/workstreams/{ws_id}/open`` refuses coordinator rows so a
server node can't accidentally rehydrate one.
## Coordinator tool set
Six tools (``spawn``, ``inspect``, ``send``, ``close``, ``delete``,
``list_workstreams``) with a ``coordinator: true`` metadata flag,
scoped to coordinator-kind sessions only. ``inspect`` and ``list`` are
auto-approved reads; the four mutators need approval. ``list`` returns
``{"children": [...], "truncated": bool}`` so the model can detect
post-filter under-fill and paginate.
## CoordinatorClient (in-process, sync)
Mutating ops HTTP-POST to the console's own ``/v1/api/route/*`` on the
local bind URL so every existing middleware (auth, rate-limit) runs.
Read ops hit ``storage.list_workstreams`` / ``get_workstream`` /
``load_messages`` directly — the routing proxy doesn't expose
list/inspect paths. URL paths are a validated constant table (avoids
an httpx ``base_url``-merge trap). A new
``/v1/api/route/workstreams/delete`` proxy handler joins the existing
route-proxy endpoints.
## Per-session coordinator JWT
``CoordinatorTokenManager`` mints short-lived JWTs with ``sub=<real
user>`` (attribution preserved), ``src="coordinator"``,
``aud="turnstone-console"``, ``coord_ws_id=<ws>`` custom claim.
``_proxy_auth_headers`` preserves ``src`` + ``coord_ws_id`` across the
upstream re-mint so server-side middleware sees coordinator-origin,
not ``console-proxy``. ``AuthResult.extra_claims`` carries
non-reserved claims through validate→remint; ``create_jwt``'s
reserved-claim set (now including ``nbf`` / ``jti``) is symmetric with
``validate_jwt``.
## Console hosts the ChatSession
- New ConfigStore settings: ``coordinator.model_alias`` (required),
``reasoning_effort``, ``max_active`` (default 5),
``session_jwt_ttl_seconds``.
- Console lifespan builds a ``ModelRegistry`` +
``CoordinatorManager``. Missing / unresolvable alias returns **503**
with remediation text — never 500.
- ``CoordinatorManager``: placeholder-slot reservation under lock,
rollback on factory failure, per-ws_id rehydration lock to serialise
concurrent lazy-opens, ``max_active`` enforced via ``close_idle``
eviction semantics.
- ``ConsoleCoordinatorUI`` is a thin ``SessionUI`` implementation — no
global broadcast, no per-node metrics, shared
``_APPROVAL_WAIT_TIMEOUT`` constant across approval + plan paths.
- No eager startup rehydration: persisted coordinator rows load lazily
on first ``GET /v1/api/coordinator/{ws_id}``.
## Console coordinator API
Nine endpoints under ``/v1/api/coordinator/*`` gated by ``approve``
scope + new **``admin.coordinator``** permission (added to
``_VALID_PERMISSIONS``; not in any builtin role — operators opt in
explicitly). Ownership failures return **404, not 403** and use
strict equality so empty-owner rows don't leak across tenants.
Correlation-id masking on every factory-raising path
(``coordinator_create`` + ``coordinator_detail`` lazy rehydrate) — no
stack traces to the client.
## Audit attribution
Three console-side events (``coordinator.create`` / ``.close`` /
``.cancel``) with the real creator's ``user_id`` plus
``detail={coord_ws_id, src="coordinator"}``. No schema migration
required. Per-tool-call audit across the routing proxy is deferred
(needs either a ``source`` column on ``audit_events`` or
``record_audit`` calls wired into the route-proxy handlers).
## Web UI (``/coordinator/{ws_id}``)
One-pane chat served by the console. Reuses ``shared_static``
(``base.css``, ``auth.js``, ``theme.js``, ``toast.js``, ``utils.js``,
``kb.js``) and the server UI's ``renderer.js`` pipeline (KaTeX, Mermaid,
highlight.js already bundled).
- SSE to ``/v1/api/coordinator/{ws_id}/events`` with exponential-
backoff reconnect; status line carries a leading glyph
(● / ○ / ⚠) so state isn't conveyed by colour alone.
- Renders content, reasoning (dimmed italic
``.role-reasoning``), tool_result, approve_request, intent_verdict,
output_warning.
- Child ws_id references auto-wrap to
``/node/{node_id}/?ws_id={child}`` links — both ids regex-validated
before interpolation, everything else HTML-escaped.
- Non-modal approval bar (``role="region"``) with a batch header
("Approve N tool calls"), initial focus on the approve button,
buttons disabled during the in-flight POST, red-bordered deny.
``aria-live`` flips to ``off`` during streaming.
- "New coordinator" button on the dashboard header — permission-gated
on the UI side, matching the backend 403.
- Mobile composer capped under ``@media (max-width: 700px)``.
## Tests
~120 new tests across 8 files: workstream-kind storage + dataclass
semantics, CoordinatorClient URL map + token minting + storage reads +
truncation signalling, tool prepare/exec dispatch and approval gating,
CoordinatorManager create / rollback / eviction / lazy rehydration +
concurrency, HTTP endpoint auth + 404-on-ownership + 503-on-misconfig,
proxy-auth ``src`` preservation, full lifecycle end-to-end, coordinator
page HTML-injection guard. ``test_tools_schema.py`` widened to 25
tools (19 existing + 6 coordinator).
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 files), ``pytest`` 4054 passed (5 pre-existing failures unrelated
to this change — confirmed against ``main``).
* polish(coordinator): address PR review + CI + tool-namespace isolation
CI:
- `ruff format`: two files reformatted, matches the in-repo pre-commit config.
- `wheel-completeness`: add `turnstone/console/static/coordinator/*.html` +
`*.js` to the hatch wheel-include list. Without this the coordinator UI
was missing from published wheels.
- `test (3.11/3.12/3.13)` + `test-postgres`: three `TestExecReadImage`
tests were masking a real bug — my 6 new tool JSONs pushed tool count
19→25, crossing the default `tool_search.auto` threshold (20), which
made `ChatSession.__init__` construct a `ToolSearchManager` and cache
`_cached_capabilities` during init. Tests that later patched
`session._provider.get_capabilities` saw the cached value instead.
Root-cause fix: the tool-search threshold code path now reads
capabilities through `_resolve_capabilities(...)` directly — no cache
populate — so the patch takes.
Tool-namespace isolation (bigger fix than CI symptoms suggested):
- `TOOLS` was the union of all loaded tool JSONs including the 6 new
coordinator tools. Interactive sessions were getting coordinator
tools in their function-calling surface (which is nonsense — they
require a console-hosted `coord_client`), and coordinator sessions
counted against the interactive tool-search threshold. Fix:
- New `INTERACTIVE_TOOLS` / `INTERACTIVE_TOOL_NAMES` in
`turnstone/core/tools.py` exclude anything with `coordinator: true`
metadata. `TOOLS` stays as the union for schema introspection +
eval catalog.
- `ChatSession.__init__` selects tool set by kind: coordinator gets
fixed `COORDINATOR_TOOLS` (no MCP merge, no listeners registered);
interactive gets `INTERACTIVE_TOOLS` (+ MCP if configured).
Coordinators are meta-orchestrators that spawn child workstreams;
MCP tools / resources / prompts live on the children, not on the
coordinator's own surface.
- `_on_mcp_tools_changed` no-ops for coordinator sessions
(defence-in-depth in case listeners were registered).
- `always_on_names` on `ToolSearchManager` is now the set of builtin
tools actually present in the session (kind-aware) rather than the
full `BUILTIN_TOOL_NAMES` frozenset.
- `turnstone/eval.py` uses `INTERACTIVE_TOOLS` (coordinator tools
aren't in scope for the eval harness which tests interactive agent
behaviour).
- Regression tests in `tests/test_workstream_kind.py`:
- `INTERACTIVE_TOOLS ∩ COORDINATOR_TOOLS == ∅` and their union is
`TOOLS`.
- Interactive `ChatSession._tools` does not include any
coordinator tool name.
- Coordinator `ChatSession._tools` contains `spawn_workstream` but
not `bash` / `edit_file` / `memory`; sub-agent lists are empty.
- Coordinator `ChatSession` with an MCP client attached does NOT
merge MCP tools and does NOT register any MCP listeners.
PR review findings:
- **#10 / #11** (Copilot): coordinator UI claimed to reuse the server
renderer pipeline but loaded none of its JS. Mirrored
`turnstone/ui/static/renderer.js` into
`turnstone/console/static/coordinator/renderer.js` (flagged in-file
as a cleanup candidate to promote into `shared_static/`), added
`katex.min.js` / `highlight.min.js` / `renderer.js` script tags to
`coordinator/index.html`. `coordinator.js` now buffers raw markdown
via `textContent` during streaming, then swaps to `renderMarkdown` +
`postRenderMarkdown` on `stream_end`.
- **#7** (Copilot): N+1 query pattern in
`CoordinatorClient.list_children()` — per-row `storage.get_workstream`
just to read `skill_id`. Pushed `skill_id` + `skill_version` into
the `list_workstreams` SELECT projection on both backends; the
client reads them from `row._mapping` directly. New
`test_list_children_skill_filter_avoids_n_plus_one` pins the
behaviour (asserts `storage.get_workstream` call count is 0).
- **#8 / #9** (Copilot): `spawn_workstream` tool JSON said "if empty,
the workstream is created idle" but the prepare method rejected
empty and the field was marked required. Resolved by allowing
empty end-to-end: removed from `required`, prepare builds a
"spawn idle workstream" header + empty preview when empty,
updated `test_spawn_prepare_allows_empty_initial_message`.
- **#1–#5** (github-code-quality): five asserts with side-effecting
method calls in `test_coordinator_manager.py` (`mgr.close`,
`mgr.open`, `mgr.create` in a dead `_c = ...`). Extracted each
call to a local variable so `python -O` can't strip the side
effect.
Verification:
- `ruff check turnstone tests` clean.
- `mypy turnstone` clean (156 source files).
- `pytest -m "not live"` — 4063 passed, 3 deselected (live-backend
tests), 0 failed. The 3 image tests that were failing on this
branch now pass; wheel + lint both green locally.
* polish(coordinator): address Copilot re-review findings
Two findings from the re-review of #368 after the first polish commit.
**user_id wired into `mgr.create()` at the server handlers.** Phase 1
added ``user_id`` to the ``Workstream`` dataclass and
``WorkstreamManager.create()`` signature, but the two call sites in
``turnstone/server.py`` forgot to pass the authenticated caller
through. Result: interactive workstreams created via
``POST /v1/api/workstreams/new`` (including coordinator-spawned
children, which route through this handler) were landing with blank
``user_id``, defeating ownership-based access control on subsequent
sends / approvals / closes (``_require_ws_access`` treats blank
owners as legacy/allowed). Two changes:
- ``server.py:create_workstream`` forwards ``user_id=uid`` — the same
``uid`` already resolved from the auth result (with trusted-service
forwarding preserved).
- ``server.py:open_workstream`` prefers the persisted owner on the
workstream row over the rehydrating caller so reloading someone
else's workstream doesn't silently re-parent it. Falls back to
the authenticated caller when the stored row has no owner
recorded (pre-phase-1 rows).
Regression test in ``tests/test_workstream.py`` pins
``WorkstreamManager.create(user_id=X)`` → ``ws.user_id == X`` so the
manager seam can't regress silently on a future refactor.
**Malformed-JSON recovery allowlist expanded for coordinator args.**
``_prepare_tool()`` has a two-stage salvage path for models that
emit malformed JSON: a regex-extract (fallback 1) and a bare-string
→ primary_key wrap (fallback 2). The fallback-1 key list didn't
include coordinator argument names, so a slightly malformed
``spawn_workstream`` / ``send_to_workstream`` / etc. call would
hard-fail instead of salvaging into a minimal-args dict for retry.
Added ``ws_id`` / ``message`` / ``initial_message`` / ``parent_ws_id``
to the allowlist (kept alphabetised) so the coordinator tools get
the same model-self-correction behaviour as the interactive tools.
Fallback 2 already covers the ``ws_id``-primary-key tools via
``PRIMARY_KEY_MAP``; the regex path matters when the model emits
``{"ws_id": "abc", "message": "..."}`` with a trailing syntax error.
Verification: ``ruff check`` clean, ``mypy turnstone`` clean
(156 source files), ``pytest -m "not live"`` → 4065 passed, 3
deselected (live-backend), 0 failed.
* fix(coordinator): address ultrareview findings on coordinator workstream kind
Security
- Cross-tenant leak: CoordinatorClient.inspect/list_children now constrain
to the coordinator's own ws_id + direct children; an LLM coerced via
prompt injection can no longer exfiltrate other tenants' workstreams.
- Empty-owner short-circuit bypass: strict equality at coordinator.py
ownership gate and at the storage-fallback branch in coordinator_history;
orphan/system-owned coordinator rows can no longer be rehydrated by
arbitrary holders of admin.coordinator (DoS + history disclosure vector).
- Closed coordinators no longer silently resurrect on subsequent GET —
the Close button is now actually durable across URL revisits and tab
refreshes; rows with state in {closed, deleted} refuse rehydration.
Correctness
- ChatSession.close() now releases the CoordinatorClient httpx.Client
pool; previously every closed/evicted coordinator dropped a connection
pool on the floor until non-deterministic GC.
- open_workstream rehydration now forwards parent_ws_id + kind, so
coordinator-spawned children survive node restart / idle eviction
with their parent link intact instead of becoming silent orphans.
- list_children truncated flag now signals whenever the SQL fetch hit
the page cap (previously permanently False in the no-filter case,
causing confident-but-incomplete summaries from the coordinator).
- ConsoleCoordinatorUI.approve_tools: per-tool auto-approve now checks
auto_approve_tools independently of the blanket auto_approve flag,
so 'Always approve this tool' actually works on the next invocation.
Concurrency
- _spawn_worker no longer falls through to start a second concurrent
worker thread on the same ChatSession when queue.Full fires; instead
send() returns False and the endpoint surfaces HTTP 429.
- _open_locks entries are now refcounted under self._lock and only
popped when the last waiter releases — eliminates the race where a
rehydration-failure path lets two threads serialize on different lock
instances for the same ws_id and trip the "already tracked" guard.
Tests: +6 regression cases covering closed-coordinator refusal,
empty-owner non-admin refusal, queue.Full no-duplicate-worker,
inspect/list_children cross-tenant rejection, and truncated semantics.
|
||
|
Patrick Buckley
|
2bfc0f2c5d |
fix: harden MCP client against misbehaving servers (#296)
* fix: harden MCP client against misbehaving servers Misbehaving/failed/misconfigured MCP servers could peg CPU at 100% due to anyio cancel-scope busy-loops (SDK #2147), uncancelled orphaned futures, and missing application-layer resilience. Five fixes: 1. Cancel orphaned futures on timeout — future.cancel() in all sync bridge methods prevents coroutine accumulation on the event loop 2. Per-server circuit breaker — 3-failure threshold with exponential cooldown (30s–5min), per-server jitter, auto-reconnect on half-open probe, McpError excluded (protocol errors from healthy servers) 3. Safe transport stream pre-close — store stream refs and close them before stack teardown in all error/shutdown paths, preventing the anyio zero-buffer CPU busy-loop 4. Notification debounce — 5s per-server rate limit on list_changed refresh storms from buggy servers 5. Periodic refresh backoff with auto-reconnect — disconnected servers get reconnection attempts with exponential backoff (60s–1hr) instead of being silently skipped forever * docs: add MCP resilience section to architecture docs and diagram Document the circuit breaker, future cancellation, stream pre-close, notification debounce, and periodic refresh backoff in the architecture guide and the MCP architecture PlantUML diagram. * fix: address review — stack leak on transport error, half-open comment - Widen _connect_one guard to check _per_server_stacks too, not just _sessions. Transport errors in sync dispatch methods evict the session but left the stack behind, leaking anyio tasks on reconnect. - Clarify half-open design: multiple callers are intentionally allowed through (reconnects serialize on the event loop, first failure re-trips). |
||
|
Patrick Buckley
|
db0baefeb2 |
feat: render rich media embeds for MCP tool results (#292)
* feat: render rich media embeds for MCP tool results Detect structured media JSON (stream_url, results, sessions) in MCP tool output and render interactive cards instead of plain text. Web UI: media cards with thumbnail, title, metadata, and click-to-play video/audio. HLS via lazy-loaded hls.js with direct-stream preference. Collapsed raw JSON (API keys redacted) for inspection. Discord: rich embeds with proxied thumbnail images (fetched by the bot since Discord CDN cannot reach private media servers). Search results as numbered lists, session state as "Now Playing" cards. Stream URLs never exposed in embeds — web_url used for safe clickable links. CI: vendor hls.js 1.6.15 with renovate tracking and update script. * fix: address PR #292 review — SSRF guards, streaming fetch, tests - URL validation: reject non-http(s) schemes and userinfo in thumbnail URLs. Private IPs intentionally allowed (media servers are on LAN). - Streaming fetch: use http.stream() with aiter_bytes() and a running byte count to enforce the 2MB cap without buffering the full response. Validate content-type is image/* before downloading. - Resilience: wrap try_build_media_embed in try/except in bot.py so a media embed failure falls through to the code-block path. - LICENSE: download hls.js LICENSE from npm on update instead of only copying from old dir. - Tests: add 19 new tests — try_parse_media (8 cases), _is_safe_image_url (7 cases), embed builders (4 cases including stream_url exclusion and string season/episode safety). * chore: add LICENSE file for vendored hls.js * fix: remove ANSI escape codes from tool preview fields Preview text (tool args, URLs, queries) was wrapped in DIM/RESET ANSI codes at the source in session.py, which leaked into SSE events and rendered as raw escape sequences in Discord and the web UI. Move ANSI styling to the CLI consumer (cli.py) where it belongs. Also escape markdown in Discord tool name titles to prevent __ from being interpreted as underline formatting. * fix: drop [MCP: server] prefix from tool descriptions The prefix made MCP tools look second-class compared to builtins, causing models to hesitate using them. The server name is already encoded in the tool name (mcp__server__tool). * feat: pretty-print JSON tool output, player error state, broader key redaction - JSON tool results are detected and pretty-printed with 2-space indent instead of rendering as a wall of text - API key redaction extended to cover api_key, apiKey, api-key, and token query params across all tool output (not just media embeds) - Video/audio player shows styled error message when stream fails to load instead of leaving a broken player element - Both appendToolOutput and replayHistory use shared renderToolOutput() * fix: designer review — player error retry, contrast, tool-cmd cap - Player error: role="alert" for screen readers, retry button that reuses existing play handler, includes media title in error message - Light theme: darken --red from #dc2626 to #b91c1c (5.7:1 contrast on --code-bg, was 4.3:1 failing WCAG AA at 12px) - Pretty-print collapsed raw JSON in media embeds (was missed earlier) - Cap .tool-cmd at 120px to prevent tools with many args from making approval blocks disproportionately tall in history replay - Dedicated .media-player-error class instead of reusing .tool-output * fix: Discord tool info name matching regression, suppress deprecation warning The escape_markdown call on tool names was stored for matching against ToolResultEvent.name, but event.name is raw/unescaped. The escaped name never matched, so the "Running → Done" transition silently failed and previews disappeared from the status embed. Fix: store raw name for matching, use escaped name only for display. Also suppress discord.py's re.sub count deprecation warning (Python 3.13+ issue, fixed upstream). * fix: update MCP tool description tests to match prefix removal * fix: address PR #292 review round 2 - Retry button: handle missing span children in click handler so retry buttons from player error state don't throw - Footer count: use len(lines) instead of min(len(results), 10) to reflect actual rendered count after char budget truncation - Null display: use "null" instead of "None" in JS tool arg preview - Broader redaction: also redact JSON "api_key": "..." patterns - SSRF hardening: block loopback and link-local IPs plus cloud metadata hostnames in thumbnail fetch (private LAN IPs still allowed) |
||
|
Patrick Buckley
|
6c9a7d7351 |
fix: harden tool call handling for local model servers (#200)
* fix: harden tool call handling for local model servers Local models (Qwen 3.5 9B, etc.) via llama.cpp produce tool calls with empty IDs, whitespace-padded names, and malformed JSON arguments. These defensive gaps caused cascading conversation corruption and silent failures. - Strip whitespace from tool names in both main and agent paths - Generate synthetic UUIDs when tool call IDs are empty/null - Surface malformed tool call errors to the user via on_error - Give the model actionable hints (expected JSON format, available tools) so it can self-correct on retry - Surface metacognition nudge types to UI via on_info Ref: #186, #117 * refactor: extract _ensure_tool_call_ids helper, include MCP tools in error Address Copilot review feedback on PR #200: - Extract duplicated ID fixup into _ensure_tool_call_ids() static method - Tests now exercise the actual helper instead of reimplementing the logic - Unknown tool error now includes MCP tool names alongside builtins |
||
|
Patrick Buckley
|
3518f7953c |
fix: prevent 100% CPU spin from unreachable HTTP MCP servers (#199)
When an HTTP MCP server is unreachable and TCP connect fails immediately (ECONNREFUSED, DNS failure), the anyio task group inside streamablehttp_client produces a CancelledError that escapes asyncio.wait_for and leaves orphaned cancel-scope tasks in an infinite _deliver_cancellation loop (~800K callbacks/sec). Three-part fix: - TCP pre-flight probe (5s timeout) before entering the anyio transport context — fails fast on unreachable servers, avoiding the bug entirely - Catch CancelledError in _connect_one with current_task().cancelling() check to distinguish stray anyio cancels from real shutdown - _safe_close_stack helper with bounded timeout that never raises, preventing cleanup errors from masking the original exception |
||
|
Patrick Buckley
|
8b2e2130fc |
fix: MCP resource template URI expansion via prefix matching (#46)
* fix: MCP resource template URI expansion via prefix matching
Resource templates (RFC 6570 URI patterns like `db://tables/{table}/rows/{id}`)
were discovered from MCP servers but non-functional — `read_resource_sync()`
only accepted exact URIs from `_resource_map`, which excludes templates.
Add prefix-based fallback: extract the static prefix from each template
(everything before the first `{`), store a prefix→server mapping, and
fall back to longest-prefix matching when exact URI lookup fails. MCP
servers handle URI routing internally so we just need to route the
expanded URI to the correct server.
Also surface templates in the system message catalog and `/mcp` command
so the model knows they exist and can construct expanded URIs.
* fix: address PR #46 review feedback
- Template prefix collision now keeps more specific (longer) template
URI instead of blindly overriding
- Fix _match_template docstring to accurately describe startswith
matching on static prefixes (not full template matching)
- Add missing loop.close() in integration test finally block
- Rewrite test_template_longest_prefix_wins with genuinely different
prefix lengths to avoid brittle collision-order dependency
|
||
|
Patrick Buckley
|
be165c1971 |
feat: MCP resource and prompt discovery with read_resource tool (#44)
* feat: MCP resource and prompt discovery with read_resource tool Extends MCPClientManager with resource and prompt discovery alongside existing tool support. Resources and prompts are discovered on connect, cached per-server with copy-on-write rebuilds, and refreshed via push notifications, periodic polling, or manual /mcp refresh. New read_resource built-in tool reads MCP resources by URI. Requires user approval (same as MCP tool calls) since resources are served by external MCP servers. Resource catalog injected into system message with XML delimiters. Error messages sanitized to prevent leaking server internals to the model. Prompt discovery stores prefixed names (mcp__server__prompt) and exposes get_prompt_sync() for future use_prompt tool (Chunk D). /mcp command now shows tools, resources, and prompts. Docs and diagrams updated. * feat: MCP prompt governance sync with origin tracking and readonly guards Migration 009 adds origin, mcp_server, and readonly columns to prompt_templates. MCP prompts discovered by MCPClientManager are automatically synced into the governance table as read-only templates with origin="mcp". Sync engine handles: create on connect, update on prompt refresh, delete when prompts are removed from server. Manual templates take precedence on name collision (MCP prompt skipped with warning). Admin API returns 403 on update/delete of readonly templates. Console UI shows MCP origin badge and disables edit/delete buttons. Storage backends gain get_prompt_template_by_name, list_prompt_templates_by_origin, and delete_prompt_templates_by_server methods. Also addresses PR #44 review feedback: concurrent.futures.TimeoutError handling in sync dispatch, XML-escape resource catalog descriptions, resource template entries excluded from _resource_map, URI collision warnings, needs_periodic capability-aware computation, malformed JSON primary key fallback for read_resource. * feat: use_prompt tool, prompt catalog, and PR review hardening New use_prompt built-in tool invokes MCP prompt templates by name, expanding them into messages. Requires user approval (external MCP servers). Prompt catalog injected into system message with XML delimiters (up to 30 prompts, HTML-escaped). Prompt listener registered in session for catalog rebuild on changes. Addresses PR #44 review feedback: - _init_system_messages() now uses copy-on-write (build locally, assign atomically) so background thread callbacks never see partial system messages - sync_prompts_to_storage() serialized behind _sync_lock to prevent races between set_storage() (main thread) and MCP background thread - shutdown() clears listener lists to release callback references Docs and diagrams updated for 18 built-in tools. * feat: granular tool policies for MCP resources, prompts, and tools Policy evaluation now uses approval_label (falling back to func_name) for fnmatch pattern matching, enabling fine-grained per-URI and per-server policies: - read_resource: mcp_resource__{normalized_uri} - use_prompt: mcp__{server}__{prompt} (prefixed name) - MCP tools: mcp__{server}__{tool} (was static "mcp_tool") URI normalization resolves .. path segments to prevent traversal bypasses in policy matching. Resource templates filtered from system message catalog (not directly readable). use_prompt arguments validated as dict with string coercion. TypeScript SDK PromptTemplateInfo gains origin, mcp_server, readonly fields. Governance docs updated with MCP policy patterns. * feat: MCP visibility in server and console UIs Server health endpoint includes mcp.servers, mcp.resources, mcp.prompts counts. Server UI status bar shows magenta MCP indicator with tooltip. Console cluster status bar shows MCP metrics with magenta LED dot. Console node detail view shows per-node MCP summary. Console collector aggregates MCP counts across nodes in overview. Uses var(--magenta) design token with new --magenta-glow for theme adaptation. ARIA roles on MCP status elements. Tooltips on console MCP metric labels. Node MCP summary hidden on mobile (< 700px). New diagram: 20-mcp-architecture.puml covering full MCP lifecycle (connection, discovery, refresh, governance sync, policy, UI). * fix: McpStatus in health schema, count properties, catalog name fidelity Adds McpStatus model to HealthResponse (Python + TypeScript SDKs) so typed clients see the mcp field from /health. Addresses Copilot review feedback: - resource_count/prompt_count properties avoid list allocation on /health and /metrics polls - get_tools/resources/prompts return shallow-copied dicts to prevent callers from mutating internal cache - Prompt names and arg names in system message catalog are NOT HTML-escaped (model must use exact strings in use_prompt calls); only descriptions are escaped * fix: OpenAPI spec McpStatus + diagram approval column accuracy Adds McpStatus schema and optional mcp field to HealthResponse in openapi-server.json, matching the Python schema and TypeScript types. Fixes tool pipeline diagram: math, web_fetch, web_search correctly shown as auto-approve (not "Yes" for approval). |
||
|
Patrick Buckley
|
c79c47b940 |
Add MCP dynamic tool refresh with push notifications and periodic pol… (#31)
* Add MCP dynamic tool refresh with push notifications and periodic polling MCP tool lists now stay up-to-date without restart via three mechanisms: push notifications (ToolListChangedNotification) for servers that support it, staggered periodic polling for servers that don't, and manual /mcp refresh [server] command. MCPClientManager tracks tools per-server with copy-on-write rebuild, notifies ChatSession listeners which rebuild tool lists and ToolSearchManager (preserving expanded tools). * Address Copilot review feedback on MCP refresh PR - Fix /mcp refresh typo matching (startswith → exact token check) - Validate --mcp-refresh-interval >= 0 at parse time via shared nonneg_float in config.py (deduplicated from cli.py + server.py) - Clamp negative refresh_interval to 0 in MCPClientManager constructor - Fix periodic refresh first poll timing (was initial_delay + interval, now initial_delay then immediate first poll) - Clarify _on_mcp_tools_changed docstring re: O(n) BM25 build cost |
||
|
Patrick Buckley
|
2b58c127b1 |
Add pluggable storage backend (SQLite + PostgreSQL) and deployment packaging (#20)
* Add pluggable storage backend (SQLite + PostgreSQL) and deployment packaging Database abstraction: StorageBackend protocol with 21 methods, SQLAlchemy Core schema, SQLite backend (FTS5), PostgreSQL backend (tsvector/ILIKE), Alembic migrations, singleton registry. memory.py reduced to thin facade. Session.py open_db() calls replaced with generic KV methods. [database] config section with env var support. Deployment: Docker Compose production profile with PostgreSQL, Dockerfile with postgres extras and migration entrypoint, Helm chart with bitnami subcharts, Terraform AWS ECS/Fargate module with RDS + ElastiCache + ALB. 39 new storage tests (934 total). mypy strict clean. Docs and diagrams updated. * Address PR #20 review feedback (16 items) - Backends only call create_all() when Alembic migrations are disabled - Helm configmap uses correct TURNSTONE_DB_BACKEND env var; DB URL constructed via env expansion with secret reference instead of ConfigMap - Migration errors fail fast for PostgreSQL (only non-fatal for SQLite) - save_memory/delete_memory wrapped in exception handling like other facade fns - pool_size passed through from config/env to init_storage() in cli + server - Terraform: DB URL moved to Secrets Manager, auth enabled flag set, optional TLS listeners with certificate_arn, Redis transit encryption on - Docker entrypoint no longer suppresses migration output - Diagram fixes: removed StaticPool claim, removed non-existent migration ref - compose.yaml/README: clarified production profile requires DB env vars |
||
|
Patrick Buckley
|
5118808f24 |
Add MCP client support for external tool servers (#8)
* Add MCP client support for external tool servers
MCPClientManager connects to stdio and HTTP MCP servers via a background
asyncio event loop, discovers tools at startup, and converts schemas to
OpenAI function-calling format with mcp__{server}__{tool} prefixing.
- New turnstone/core/mcp_client.py: async-sync bridge, config loader
(TOML [mcp.servers.*] + standard mcpServers JSON), tool discovery
- session.py: mcp_client param, self._tools/_task_tools/_agent_tools,
_prepare_mcp_tool/_exec_mcp_tool, /mcp introspection command
- tools.py: merge_mcp_tools() helper
- cli.py + server.py: --mcp-config arg, client lifecycle, banner info
- pyproject.toml: mcp>=1.6 required dependency, mypy override
- 30 new tests (config, schema conversion, session integration, errors)
- Docs: README MCP section, tools.md MCP reference, architecture.md
MCP subsection, 3 updated PlantUML diagrams + PNGs
* Fix Copilot PR #8 review: hermetic MCP config tests, approval docs wording
- Patch load_config in test_json_file_not_found and test_invalid_json so
a developer's local config.toml doesn't leak into test results
- Clarify MCP approval docs: tools require approval by default, but
--skip-permissions and UI auto-approve override this
|