feat: use saved HTTP MCP servers in web chat

[charlesrhoward]

Enabled HTTP servers saved under Connections → MCP Servers now provide tools to workspace chat and Control on the next turn, even when the user has no Integrations. Previously those definitions only synced to the CLI. The add form defaults to Streamable HTTP and labels local stdio servers as CLI-only.

The shared loader reads only the caller's enabled HTTP entries, resolves saved headers server-side, blocks private destinations and redirects, preserves tool restrictions and explicit approval settings, namespaces tool names, and closes clients after use. Catalog or server failures leave other tools available. Existing team capability checks run before catalog access. Saved-server startup returns partial results after six seconds, before Control's existing eight-second deadline. Healthy sessions detach from the startup signal, so a stalled server cannot abort their open streams or later tool calls. Existing Integration transport behavior is preserved. No schema or CLI changes.

The requested automatic availability applies to existing enabled public HTTP rows as well as new ones. With no explicit policy, tools run automatically, matching web Integrations; saved Extra JSON can restrict tools or request Control approval. CLI approve means pre-approved, equivalent to auto. Invalid known policy fields fail closed; unrelated CLI fields are ignored.

Validation: 19 focused runtime tests (including real MCP SDK discovery/calls over fixture HTTP), 26 unit tests, a PGlite query regression, 12 browser tests, and desktop/mobile visual review. Regression checks failed with the old behavior and with owner filtering removed, then passed after restoration. Lint/typecheck and production build passed. No live provider credentials or connections changed.

Docs: Mogplex/docs#137 (merge after the app release).

[mogplex]

Mogplex PR Review

Status: Attention needed

Solid, well-tested change: the saved-MCP loader filters by owner/enabled/transport before touching Vault, re-validates every outbound URL (including redirect targets), namespaces tool names with a per-server hash, and cleans up clients idempotently. Capability gating runs before catalog access in both the workspace (lib/agents/tools/index.ts) and Control (app/api/control/chat/_lib/connection-tools.ts) paths, and the added tests cover cross-user, disabled, stdio, private-URL, missing-secret, and catalog-failure cases. No critical security or correctness defects found. Four things worth resolving before merge: (1) the workspace-chat path now dials remote MCP servers on every turn for every user with no timeout and no cap, while Control deliberately budgets 8s; (2) the "approve" approval mode is treated as auto-execute, which is a fail-open reading of a CLI-defined enum I could not verify (web research tooling was unavailable: EXA_API_KEY not configured); (3) redirect: "error" and per-request DNS revalidation in lib/connections/mcp-tools.ts now apply to all existing Integration MCP connections, a behavior change beyond the PR's stated scope; (4) load failures are logged without any server identity or error class, so a broken server silently drops its tools with nothing to debug from.

Warnings

• Workspace chat has no timeout or cap when loading saved MCP servers (lib/mcp-servers/chat.ts)
  loadSavedMcpServerTools() fans out over every enabled HTTP row (listSavedHttpMcpServers has no .limit()) and awaits the full MCP handshake plus tools() for each, with no time budget. buildDynamicConnectionTools awaits it unconditionally when ctx.userId is set, and lib/agents/tools/index.ts awaits buildDynamicConnectionTools before returning tools, so a slow or hanging third-party server now delays first token on every workspace turn.

This is the exact failure Control explicitly guards against — CONTROL_CONNECTION_TOOLS_TIMEOUT_MS in app/api/control/chat/_lib/connection-tools.ts documents the posture: "a slow one must cost the turn its connection tools, never the turn itself." The workspace path has no equivalent, and removing the connections.length === 0 fast path means users who previously had zero connections (and therefore zero remote I/O) are now exposed to it too.

Suggestion: wrap loadSavedMcpServerTools in the same race-with-timeout used in loadControlConnectionTools (cleaning up late clients via cleanupMcpClients), and add a bounded .limit(N) to listSavedHttpMcpServers so N saved servers cannot mean N unbounded concurrent outbound handshakes (and N servers' worth of tool schemas in context) per turn.

• "approve" approval mode is treated as auto-execute — confirm this matches CLI semantics (lib/mcp-servers/chat.ts)
  toolApproval() can return "auto", "approve", "prompt", or "deny", but the gate only handles two of them: if (approval === "deny" || (approval === "prompt" && !canAskApproval)) continue;. "approve" therefore falls through and the tool is registered as freely callable, and it is not added to askToolNames.

The enum having both "auto" and "approve" is the smell: if they both mean "run without asking" one of them is redundant, which suggests "approve" may mean "requires an explicit approval" in the CLI config that owns these keys (extra is spread verbatim into the CLI record). If that is the case, this silently executes tools the user gated. Note the failure direction is asymmetric: an unrecognized mode string fails closed (zod throws and the whole server is dropped), but a misread "approve" fails open.

I could not verify the CLI's definition — web research was unavailable in this run (EXA_API_KEY not configured) and the semantics live in the companion docs PR. Was auto-execute intentional? If so, please add a short comment citing the CLI definition next to toolApproval; if not, treat "approve" like "prompt" (gate on canAskApproval and add to askToolNames).

• redirect: "error" and per-request DNS checks now apply to all existing Integration MCP connections (lib/connections/mcp-tools.ts)
  The new custom fetch in getRemoteMcpTools is on the shared path: getMcpTools delegates to it, so every existing Integration MCP connection now (a) refuses any 3xx response outright and (b) performs a fresh assertSafeOutboundHttpUrlWithDns — i.e. a node dns.lookup — on every request, including each streamable-HTTP/SSE message and every tool call.

The security intent is right (stored headers must not follow a redirect to an unvalidated host), but two side effects reach beyond the PR's stated scope of "saved HTTP servers": providers that 3xx to a canonical MCP endpoint (trailing-slash or /sse → /sse/ style redirects are common) will now fail to load where they previously worked, and per-request DNS resolution adds latency to every tool call since dns.lookup is not cached in-process.

Suggestion: allow a single hop when the redirect target passes assertSafeOutboundHttpUrlWithDns and is same-origin (dropping stored headers otherwise), or confirm none of the shipped presets redirect and call the break out in the description. Consider memoizing the DNS assertion per origin for the lifetime of the client to avoid a lookup per message.

• Load failures are unattributable: no server id, no error class (lib/mcp-servers/chat.ts)
  Both failure paths log a bare constant string — "[mcp-servers] A saved HTTP server could not load for chat" for a rejected per-server promise, and "[mcp-servers] Saved HTTP servers could not load for chat" in the outer catch. Avoiding error.message is the right call (URLs and header values can carry secrets), but dropping the identity of the failing row means an operator cannot tell which server broke, how many broke, or whether the cause was SSRF rejection, a missing Vault secret, an invalid extra policy, or an upstream 500. There is also no user-facing signal: tools just silently stop appearing (unlike withheldConnections, which surfaces withheld ask connections).

Suggestion: log server.id plus a non-message discriminator that is safe to emit — e.g. error.name, and error.code for UnsafeOutboundUrlError (INVALID_URL vs UNSAFE_OUTBOUND_TARGET) and McpServerValidationError. Consider returning the failed server ids so the settings page or chat surface can tell the user their server is not loading.

Suggestions

• Minor: tool mutation and a narrow cleanup leak in the registration loop (lib/mcp-servers/chat.ts)
  Two small things in the registration loop:

1. dynamicTools[toolName] = Object.assign(tool, { description: ... }) mutates the Tool object handed back by the MCP client rather than copying it. Harmless today since each client's tools are freshly created per load, but { ...tool, description: ... } expresses the intent without reaching into SDK-owned state.

2. mcpCleanups.push(loaded.cleanup) happens per result inside the try, so if anything in the naming/registration loop throws mid-iteration, clients from later fulfilled results are never pushed and never closed — the outer catch returns only the cleanups collected so far. Pushing every fulfilled result's cleanup in one pass before the naming loop (or scoping the try to the load only) closes that window.

View check run

[mogplex]

Mogplex PR Review

Status: Attention needed

Solid, well-tested change overall: the SSRF posture (pre-flight DNS validation, per-request re-validation, redirect: "error"), owner-scoped catalog reads before Vault access, fail-open error handling, and client cleanup are all handled carefully, and the test coverage (real MCP SDK over a fixture transport, PGlite owner filtering, e2e) is better than typical. No critical security or correctness defects found.

What I'd like discussed before merge is the permission and rollout posture rather than a code defect: with no extra policy (the common case, since the web UI exposes no approval setting), every tool from an enabled HTTP server is registered as an auto-run tool, and servers users previously saved for CLI-only use are opted into hosted web chat — with their Vault headers now read by the hosted runtime — on deploy, with no opt-in or feature flag. Related: approval_mode: "approve" is accepted by the schema but treated exactly like auto, which is only correct if the CLI also means "pre-approved" by that word.

Secondary concerns: one AbortController and one timer are shared across all saved servers, and the inner startup budget is now literally the same constant as the Control turn budget, so the "one slow server doesn't cost the other servers' tools" property is tighter than it looks; and every workspace-chat turn now re-dials all saved HTTP servers with no cache or cap. Details in the findings.

5 findings were added inline.

Warnings

• Every turn re-dials all saved servers; no cache, no cap, and no outer timeout on the workspace path (lib/agents/tools/index.ts)
  buildTools in lib/agents/tools/index.ts no longer short-circuits when a user has no connections, so each workspace-chat turn now performs: one user_mcp_servers query for every user (including the majority with zero saved servers), then, per saved server, a Vault decrypted_secrets query plus a full MCP initialize + tools/list round trip. There is no caching across turns and no cap on server or tool count.

Concretely:

• The workspace path has no outer timeout equivalent to Control's, so the internal 8s startup budget becomes added latency on every turn when a saved server is slow or down. A permanently-unreachable saved server costs the user ~8s per message with only a console.warn to show for it.
• resolveVaultSecrets is called once per server rather than batched across servers (uniqueSecretIds already exists for the batched shape used elsewhere).
• Tool count is unbounded: N servers x M tools all land in the prompt every turn, affecting both token cost and provider tool-count limits.

Suggestions: batch the secret resolution across servers; consider caching discovery per conversation/turn or a short-lived per-user cache; and add a cap (with a warning) on servers/tools admitted per turn. At minimum, confirm user_mcp_servers has an index covering (user_id, enabled, transport) since this query now runs on every chat turn for every user.

View check run

[mogplex]

Mogplex PR Review

Status: Attention needed

Solid, carefully built change. The security posture on the new path is good: the catalog query is owner/enabled/transport filtered before any Vault read, assertSafeOutboundHttpUrlWithDns runs per request, redirect: "error" blocks redirect-based SSRF, missing secrets fail closed instead of dialing anonymously, unknown policy shapes fail closed, and clients are closed on both success and failure. The startup-signal detach (per-server inner AbortController whose listener is removed in finally) correctly prevents the 6s deadline from killing healthy long-lived sessions, and the test suite covers the sad paths unusually well (owner/enabled/stdio/local-URL exclusion, catalog isolation, partial startup, per-server failure isolation, name collisions).

No critical issues found. Six non-blocking items, two of which I'd like an answer on before merge: (1) the saved-server fan-out is unbounded per turn, and (2) one Integration-path test assertion doesn't appear reconcilable with the implementation — either it fails, or it passes for a reason that makes the guard assertions weaker than they look. I also flagged the approve → auto-run mapping as the one policy translation where being wrong silently weakens a user's explicit gate.

Reviewed: lib/mcp-servers/chat.ts, lib/connections/mcp-tools.ts, lib/agents/tools/connections.ts, lib/agents/tools/index.ts, app/api/control/chat/_lib/connection-tools.ts, lib/mcp-servers/chat.test.ts, plus supporting secrets/validation/types/transport modules. UI copy changes were not inspected in depth (low risk).

Verdict: 💬 needs discussion — approve-ready once the fan-out bound and the redirect assertion are settled.

Warnings

• Saved-server fan-out per turn is unbounded (lib/mcp-servers/chat.ts)
  listSavedHttpMcpServers selects every enabled HTTP row for the user with no .limit(), and loadSavedMcpServerTools then dials all of them in parallel via Promise.allSettled — one Vault decrypt read plus a full MCP handshake (initialize + notifications/initialized + tools/list + DELETE) per row, on every Control and workspace turn. POST /api/mcp-servers does not appear to cap rows per user, so a single account can turn each of its own chat turns into hundreds of concurrent outbound requests from the server runtime: socket/memory pressure on the hosted runtime and an outbound amplifier aimed at third parties. The 6s deadline bounds latency, not concurrency or volume.

Suggestion: add .limit(MAX_SAVED_CHAT_SERVERS) (ordered by created_at, as today) and log once when rows are truncated so the user-visible symptom is explainable; optionally bound concurrency (batch the allSettled in chunks) rather than dialing all at once.

• Integration-path test asserts redirect: "error", which the implementation only sets for saved servers (lib/mcp-servers/chat.test.ts)
  In "keeps existing Integration tools available when the saved catalog fails", the test asserts requests.find(r => r.url.startsWith("https://8.8.8.8"))?.redirect is "error". That request comes from getMcpTools → getRemoteMcpTools(transport) with no options, so validateRequests is undefined and fetch is passed as undefined — the SDK's default fetch is used and no redirect: "error" is applied (the stub builds new Request(input, init), which defaults to follow). failCatalog = true in this test, so no saved-server request can satisfy the find.

I can't verify @ai-sdk/mcp's internals from here, so I'm flagging this as a question rather than a definite failure. Two possibilities, both worth resolving: either this assertion fails in CI, or @ai-sdk/mcp already sets redirect: "error" itself — in which case the identical assertions in the saved-server tests do not actually prove the new guarded fetch is installed, and the redirect protection should be asserted some other way (e.g. assert the custom fetch ran by having the stub observe a marker, or assert on a 3xx being rejected).

Please confirm this file is green on head.

• approve mapped to auto-run; prompt tools vanish silently in workspace chat (lib/mcp-servers/chat.ts)
  toolApproval treats approval_mode: "approve" as "run automatically" (only deny and prompt are special-cased). This is the one policy translation where being wrong is a real security regression: if the CLI's "approve" means "ask the operator before running", every such tool now executes server-side without a gate, using the user's Vault-backed credentials. The CLI lives outside this repo so I can't check its semantics — please add a short comment citing the CLI definition (file/symbol) next to toolApproval so the mapping is auditable, and confirm the docs PR states it.

Related, lower stakes: buildTools (workspace chat) never passes canAskApproval, so prompt-policy tools are dropped with no user-visible signal. Integrations have withheldConnections for exactly this case; saved servers are not represented there, so the user just sees missing tools. Consider extending the withheld reporting (or logging per server) so "my saved server's tools aren't showing up" is diagnosable.

Suggestions

• Startup budget: magic - 2000 and an inverted home for the Control constant (lib/connections/mcp-tools.ts)
  CONNECTION_TOOL_STARTUP_TIMEOUT_MS = 8000 now lives in the low-level transport module, is re-exported as Control's turn budget in app/api/control/chat/_lib/connection-tools.ts, and is reused in lib/mcp-servers/chat.ts as CONNECTION_TOOL_STARTUP_TIMEOUT_MS - 2000. Two consequences: the Control turn policy is now defined in a transport file (layering inversion), and any future reduction of the constant to ≤ 2000 makes the saved-server timer fire immediately or with a negative delay, so saved servers silently never load.

Suggestion: export the saved-server budget explicitly (e.g. SAVED_SERVER_STARTUP_TIMEOUT_MS = Math.max(1000, CONNECTION_TOOL_STARTUP_TIMEOUT_MS - SAVED_SERVER_HEADROOM_MS)) next to the deadline it protects, with the headroom named rather than inlined.

• Outer catch discards the failure reason entirely (lib/mcp-servers/chat.ts)
  The outer catch {} in loadSavedMcpServerTools logs only { userId, timedOut }. Unlike the per-server path (where redacting is justified — URLs and header values can carry secrets), this branch can only be reached by the catalog read (listSavedHttpMcpServers), whose errors are Postgres/PostgREST errors with no secret material. As written, a 503 or a permissions/RLS change on user_mcp_servers produces a log line that can't distinguish it from a timeout.

Suggestion: include error instanceof Error ? error.message : String(error) in that one warn call.

• Saved servers ignore repo scoping that Integrations honor (lib/agents/tools/connections.ts)
  buildDynamicConnectionTools passes only ctx.userId to loadSavedMcpServerTools, while Integrations go through loadScopedConnections(userId, repoId) → getResolvedConnections(userId, repoId). So a saved HTTP server is injected into chat for every repo/workspace with no per-repo opt-out, which diverges from the scoping model users already learned from Integrations (and there are no schema changes here to add it). Reasonable for v1, but worth stating explicitly in the docs PR, and worth a brief comment at the call site noting that saved servers are account-scoped by design.

View check run