unsloth/studio/backend/routes/mcp_servers.py
Nilay 9a907a8acb
Studio: add remote MCP server support (#5750)
* added remote MCP server support

* trim

* added tests

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* increased timeout

* disabling MCP chat toggle

* Fix MCP OpenAI function-name validation + cancel propagation for PR #5750

OpenAI requires function.name to match ^[a-zA-Z0-9_-]{1,64}$ before
streaming starts. The existing 64-char length check is necessary but
not sufficient: MCP servers can return tool names containing '.', '/',
spaces, etc. that would 400 the whole chat request. Validate the
composed mcp__<server_id>__<tool> name against the regex, skip + warn
on miss, and drop duplicate tool names from the same server (which
would also 400 the request as "duplicates").

Also propagate the agentic-loop cancel_event into MCP tool execution
so a /cancel POST during a long-running MCP call (e.g. GitHub MCP
search across a large repo) actually interrupts the in-flight HTTP
call instead of waiting out the 300 s timeout. The watcher polls the
threading.Event at 50 ms cadence inside the asyncio loop (matches
routes/inference.py's existing cancel-watcher cadence) and races
against the call task with asyncio.wait FIRST_COMPLETED.

Tests added:
  - test_mcp_specs_skip_invalid_openai_function_names: drops bad chars
  - test_mcp_specs_skip_empty_tool_name
  - test_mcp_specs_drops_duplicate_names
  - test_call_tool_sync_respects_pre_set_cancel_event

Also fix test_desktop_auth.py's router stub that listed every existing
router but missed mcp_servers_router, so importing main.py fails after
this PR adds it to routes/__init__.py.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* PR #5750 round 2: OAuth cleanup on delete/url-change + mcp_enabled standalone

Round 2 of cross-platform validation surfaced two more P1 findings:

1. OAuth tokens never get cleared. fastmcp keys tokens by MCP URL, not by
   server row, and delete / URL change / use_oauth toggle only updated
   the SQLite row. Re-registering the same URL would silently reuse the
   old account's credentials. Adds clear_oauth_tokens_async() in
   mcp_client.py and calls it from the delete + put route handlers when
   the row had use_oauth=True and either the URL changes or OAuth is
   turned off.

2. mcp_enabled=true was ignored unless the caller also sent
   enable_tools=true. The frontend always sends both together so the UI
   path was fine, but a direct API caller sending only mcp_enabled would
   silently get no MCP tools, which contradicts the field's documented
   "append tools from every enabled MCP server" behavior. Loosens the
   use_tools gate in both the GGUF and safetensors paths so mcp_enabled
   opens the tool loop on its own; when the caller did not also opt
   into built-ins, the built-in list starts empty.

Tests added:
  - test_clear_oauth_tokens_async_no_op_safe
  - test_delete_server_calls_oauth_cleanup_when_oauth_was_on
  - test_delete_server_skips_oauth_cleanup_when_oauth_off
  - test_update_server_clears_oauth_on_url_change
  - test_update_server_clears_oauth_when_oauth_disabled

26 backend MCP tests pass; full studio/backend suite 1710 passed locally.
Cross-platform CI (Linux, macOS, Windows) green on staging fork.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* PR #5750 round 3: reject null bool updates + /test surfaces 400

Round 3 of cross-platform validation:

1. PUT /api/mcp/servers/<id> would 500 with TypeError when the body
   explicitly set is_enabled or use_oauth to null. Pydantic accepts
   None for an Optional[bool] and _changes_from_payload then passed
   None into mcp_servers_db.update_server, which int(None)d. Reject
   explicit null at the validation layer with 400 instead.

2. POST /api/mcp/servers/test caught HTTPException under
   "except Exception", so an invalid URL came back as HTTP 200 with
   {"ok": false, "error": "400: ..."} instead of a real 400. The
   create + update paths return 400 for the same input. Move
   validation outside the transport try/except so it surfaces 400.

Tests added:
  - test_changes_from_payload_rejects_null_is_enabled
  - test_changes_from_payload_rejects_null_use_oauth
  - test_test_endpoint_surfaces_url_validation_as_400

* PR #5750 round 4: hyphenated MCP tool names + empty-tool-list gate

Round 4 surfaces two more interaction bugs between the new MCP path
and existing safetensors tool plumbing:

1. OpenAI accepts ^[a-zA-Z0-9_-]{1,64}$ for function.name, and round 1
   widened the MCP regex to that set, so MCP tools can now be advertised
   as `mcp__srv__list-issues`. But the XML tool-call parser in
   tool_call_parser.py used `\w+` (no hyphen), so the model could call
   the tool but Studio could not parse the call. Same in
   routes/inference.py's `_TOOL_XML_RE` stripper, which would leave
   hyphenated tool-call XML in the visible content. Both regexes now
   use `[\w-]+`.

2. safetensors_agentic treats `tools=[]` as "allow all" (documented
   contract, exercised by test_empty_tools_list_does_not_enforce_allowlist).
   When a caller sends `enable_tools=true` + `enabled_tools=[]` +
   `mcp_enabled=true` and MCP discovery returns 0, the resolved tool
   list is genuinely empty and built-in tools (web_search / python /
   terminal) could execute via the model's emitted call. Fix at the
   route gate instead of breaking the documented contract: set
   `use_tools=False` when the resolved list is empty, in both GGUF and
   safetensors paths. Existing callers who omit `enabled_tools` still
   get ALL_TOOLS and are unaffected.

Tests added (32 total):
  - test_tool_xml_parser_handles_hyphenated_function_names
  - test_tool_xml_strip_handles_hyphenated_function_names
  - test_safetensors_agentic_empty_allowlist_still_means_allow_all
    (documents the contract round 4 preserved)

1716 passed locally; cross-platform CI on staging fork still green.

* PR #5750 round 5: GGUF allow-list + CLI policy + hyphenated params + cancel race

Round 5 of parallel-reviewer aggregation surfaced six additional
findings; five are real and fixed here:

1. Hyphenated MCP parameter names (`<parameter=issue-number>`) were
   dropped by the XML parser's `\w+` regex. Extended to `[\w-]+` in
   both core/inference/tool_call_parser.py and core/tool_healing.py.
   The latter is GGUF's own copy of the parser/strip patterns and was
   missed by round 4.

2. core/tool_healing.py's `strip_tool_call_markup` still used
   `<function=\w+>` so hyphenated MCP tool-call XML leaked into the
   GGUF visible content even after round 4 fixed the shared parser.

3+4. `mcp_enabled` re-opened the tool loop even when the operator
   passed `unsloth run --disable-tools` (CLI policy False). Round 2's
   `(_tools_on or payload.mcp_enabled)` gate ignored the raw process
   policy. Now reads `state.tool_policy.get_tool_policy()` and gates
   mcp_enabled on `_cli_policy is not False`. Applied to both GGUF
   and safetensors paths.

5. GGUF's agentic loop called `execute_tool(tool_name, ...)` without
   checking the model-emitted name against the per-request tool list,
   while the safetensors loop already enforces this. Added the same
   allow-list check so a model that hallucinates a filtered MCP name
   or a built-in the caller opted out of returns "not enabled" instead
   of executing.

Bonus P2 fixes:
  - `call_tool_sync` now checks `cancel_event.is_set()` BEFORE
    creating the call task, so a pre-set cancellation does not open
    the HTTP transport.
  - `clear_oauth_tokens_async` moved the OAuth import + construction
    inside the protected try block; a fastmcp.client.auth load error
    used to escape and 500 the delete / update route.

NOT fixed (verified false or out of scope):
  - finding #10 "structured_content vs structuredContent": fastmcp's
    CallToolResult dataclass uses snake_case (verified live against
    structured-only tool result; fields are
    `dict_keys(['content', 'structured_content', 'meta', 'data', 'is_error'])`).
  - finding #11 "asyncio.run from running loop": call_tool_sync is
    invoked from `asyncio.to_thread` worker threads which have no
    event loop; asyncio.run() is safe there.

Tests added (37 total): hyphenated param names, tool_healing strip,
GGUF allow-list gate, cancel pre-set short-circuit, OAuth cleanup
constructor-error swallowing. 1721 passed locally, no regressions.

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: danielhanchen <danielhanchen@gmail.com>
2026-05-27 07:01:11 -07:00

223 lines
7.6 KiB
Python

# SPDX-License-Identifier: AGPL-3.0-only
# Copyright 2026-present the Unsloth AI Inc. team. All rights reserved. See /studio/LICENSE.AGPL-3.0
import json
import uuid
from urllib.parse import urlparse
import structlog
from fastapi import APIRouter, Depends, HTTPException
from auth.authentication import get_current_subject
from core.inference.mcp_client import (
clear_oauth_tokens_async,
list_tools_async,
parse_server_headers,
)
from models.mcp_servers import (
McpServerCreate,
McpServerProbeResult,
McpServerResponse,
McpServerTestRequest,
McpServerUpdate,
)
from storage import mcp_servers_db
logger = structlog.get_logger(__name__)
router = APIRouter()
_PROBE_TIMEOUT_SECONDS = 8.0
# When OAuth probes need to open a browser, wait long enough for the user to
# sign in. Matches fastmcp's default OAuth callback_timeout (300 s) + slack.
_OAUTH_PROBE_TIMEOUT_SECONDS = 305.0
def _validate_url(url: str) -> str:
trimmed = (url or "").strip()
if not trimmed:
raise HTTPException(status_code = 400, detail = "url must not be empty")
parsed = urlparse(trimmed)
if parsed.scheme not in ("http", "https"):
raise HTTPException(
status_code = 400,
detail = "url must start with http:// or https://",
)
if not parsed.netloc:
raise HTTPException(status_code = 400, detail = "url is missing a host")
return trimmed
def _normalize_headers(headers: dict[str, str] | None) -> dict[str, str] | None:
"""Trim header names, drop empties, coerce values to str. None if nothing left."""
if not headers:
return None
out: dict[str, str] = {}
for raw_key, value in headers.items():
key = str(raw_key).strip()
if key:
out[key] = str(value)
return out or None
def _row_to_response(row: dict) -> McpServerResponse:
return McpServerResponse(
id = row["id"],
display_name = row["display_name"],
url = row["url"],
headers = parse_server_headers(row) or {},
is_enabled = bool(row["is_enabled"]),
use_oauth = bool(row.get("use_oauth")),
created_at = row["created_at"],
updated_at = row["updated_at"],
)
@router.get("/", response_model = list[McpServerResponse])
async def list_mcp_servers(
current_subject: str = Depends(get_current_subject),
):
return [_row_to_response(row) for row in mcp_servers_db.list_servers()]
@router.post("/", response_model = McpServerResponse, status_code = 201)
async def create_mcp_server(
payload: McpServerCreate,
current_subject: str = Depends(get_current_subject),
):
display_name = (payload.display_name or "").strip()
if not display_name:
raise HTTPException(status_code = 400, detail = "display_name must not be empty")
url = _validate_url(payload.url)
headers = _normalize_headers(payload.headers)
server_id = uuid.uuid4().hex[:16]
mcp_servers_db.create_server(
id = server_id,
display_name = display_name,
url = url,
headers_json = json.dumps(headers) if headers else None,
is_enabled = payload.is_enabled,
use_oauth = payload.use_oauth,
)
return _row_to_response(mcp_servers_db.get_server(server_id))
def _changes_from_payload(payload: McpServerUpdate) -> dict:
sent = payload.model_fields_set
changes: dict = {}
if "display_name" in sent:
name = (payload.display_name or "").strip()
if not name:
raise HTTPException(
status_code = 400, detail = "display_name must not be empty"
)
changes["display_name"] = name
if "url" in sent:
changes["url"] = _validate_url(payload.url or "")
if "headers" in sent:
headers = _normalize_headers(payload.headers)
changes["headers_json"] = json.dumps(headers) if headers else None
if "is_enabled" in sent:
if payload.is_enabled is None:
raise HTTPException(
status_code = 400, detail = "is_enabled must be true or false"
)
changes["is_enabled"] = payload.is_enabled
if "use_oauth" in sent:
if payload.use_oauth is None:
raise HTTPException(
status_code = 400, detail = "use_oauth must be true or false"
)
changes["use_oauth"] = payload.use_oauth
return changes
@router.put("/{server_id}", response_model = McpServerResponse)
async def update_mcp_server(
server_id: str,
payload: McpServerUpdate,
current_subject: str = Depends(get_current_subject),
):
old = mcp_servers_db.get_server(server_id)
if not old:
raise HTTPException(status_code = 404, detail = "MCP server not found")
changes = _changes_from_payload(payload)
if not changes:
raise HTTPException(status_code = 400, detail = "No fields to update")
# Clear persisted OAuth tokens when the URL changes or OAuth is
# disabled; fastmcp keys tokens by URL and would otherwise let a
# re-pointed server silently inherit the old account's credentials.
if bool(old.get("use_oauth")) and (
("url" in changes and changes["url"] != old["url"])
or changes.get("use_oauth") is False
):
await clear_oauth_tokens_async(old["url"])
mcp_servers_db.update_server(server_id, changes)
return _row_to_response(mcp_servers_db.get_server(server_id))
@router.delete("/{server_id}", status_code = 204)
async def delete_mcp_server(
server_id: str,
current_subject: str = Depends(get_current_subject),
):
old = mcp_servers_db.get_server(server_id)
if not old:
raise HTTPException(status_code = 404, detail = "MCP server not found")
if old.get("use_oauth"):
await clear_oauth_tokens_async(old["url"])
mcp_servers_db.delete_server(server_id)
@router.post("/{server_id}/refresh", response_model = McpServerProbeResult)
async def refresh_mcp_server_tools(
server_id: str,
current_subject: str = Depends(get_current_subject),
):
server = mcp_servers_db.get_server(server_id)
if not server:
raise HTTPException(status_code = 404, detail = "MCP server not found")
use_oauth = bool(server.get("use_oauth"))
try:
tools = await list_tools_async(
url = server["url"],
headers = parse_server_headers(server),
timeout = _OAUTH_PROBE_TIMEOUT_SECONDS
if use_oauth
else _PROBE_TIMEOUT_SECONDS,
use_oauth = use_oauth,
)
except Exception as exc: # noqa: BLE001 — surface transport+timeout errors to UI
logger.warning("MCP refresh failed", server_id = server_id, error = str(exc))
return McpServerProbeResult(ok = False, error = str(exc))
return McpServerProbeResult(ok = True, tool_count = len(tools))
@router.post("/test", response_model = McpServerProbeResult)
async def test_mcp_server(
payload: McpServerTestRequest,
current_subject: str = Depends(get_current_subject),
):
# URL/header validation must surface as 400 like create/update so the
# frontend's create-form pre-flight gets the same error semantics as
# the actual save call. Only catch transport/timeout errors below.
url = _validate_url(payload.url)
headers = _normalize_headers(payload.headers)
try:
tools = await list_tools_async(
url = url,
headers = headers,
timeout = _OAUTH_PROBE_TIMEOUT_SECONDS
if payload.use_oauth
else _PROBE_TIMEOUT_SECONDS,
use_oauth = payload.use_oauth,
)
except Exception as exc: # noqa: BLE001
return McpServerProbeResult(ok = False, error = str(exc))
return McpServerProbeResult(ok = True, tool_count = len(tools))