From 32bd82b3e78a1cbb66fe71dc6f62d3edf5823ff2 Mon Sep 17 00:00:00 2001 From: Chris Co Date: Sat, 15 Aug 2026 04:40:40 +0000 Subject: [PATCH] fix(mcps): migrate servers to MCP SDK 2 MCP 2 removes FastMCP and runs synchronous tool handlers concurrently in worker threads. The servers rely on serialized state and cache access. Update to MCPServer and use one shared lock context around each server's tool bodies to preserve MCP 1 ordering. Test concurrent requests for one uncached package and verify that it is cloned only once. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: dd338a3f-cff6-4b03-8c00-fce99b2ac4a4 --- scripts/mcps/_mcp_utils.py | 9 +- scripts/mcps/fedora-distgit-mcp.py | 431 +++++++++--------- scripts/mcps/koji-mcp.py | 276 +++++------ scripts/mcps/requirements.txt | 2 +- scripts/mcps/tests/__init__.py | 3 + scripts/mcps/tests/conftest.py | 10 + scripts/mcps/tests/requirements.txt | 2 + .../mcps/tests/test_distgit_concurrency.py | 95 ++++ 8 files changed, 481 insertions(+), 347 deletions(-) create mode 100644 scripts/mcps/tests/__init__.py create mode 100644 scripts/mcps/tests/conftest.py create mode 100644 scripts/mcps/tests/requirements.txt create mode 100644 scripts/mcps/tests/test_distgit_concurrency.py diff --git a/scripts/mcps/_mcp_utils.py b/scripts/mcps/_mcp_utils.py index cd300a444ca..0efb64a9790 100644 --- a/scripts/mcps/_mcp_utils.py +++ b/scripts/mcps/_mcp_utils.py @@ -11,9 +11,12 @@ import sys import tempfile from pathlib import Path -from typing import Any +from typing import TYPE_CHECKING, Any from urllib.parse import urlparse +if TYPE_CHECKING: + from urllib.parse import ParseResult + # Return type for all MCP tools — values may be str, int, list, or None. StatusDict = dict[str, Any] @@ -29,7 +32,7 @@ sys.exit(1) try: - from mcp.server.fastmcp import FastMCP # noqa: F401 — re-exported + from mcp.server import MCPServer # noqa: F401 - re-exported for MCP entrypoints except ImportError: sys.stderr.write("\n" + "=" * 60 + "\n") sys.stderr.write(" MISSING DEPENDENCY: 'mcp' package not found\n") @@ -99,7 +102,7 @@ def validate_base_url(base_url: str) -> tuple[str, str | None]: return normalized, None -def effective_port(parsed) -> int | None: +def effective_port(parsed: ParseResult) -> int | None: """Return the effective port for a parsed URL (explicit or scheme default).""" if parsed.port is not None: return parsed.port diff --git a/scripts/mcps/fedora-distgit-mcp.py b/scripts/mcps/fedora-distgit-mcp.py index 8d233ad6412..40b65628340 100644 --- a/scripts/mcps/fedora-distgit-mcp.py +++ b/scripts/mcps/fedora-distgit-mcp.py @@ -30,10 +30,11 @@ import urllib.error import urllib.request from pathlib import Path +from threading import Lock from urllib.parse import urlparse from _mcp_utils import ( - FastMCP, + MCPServer, StatusDict, check_ssrf, load_env, @@ -42,7 +43,11 @@ write_output, ) -mcp = FastMCP("fedora-distgit") +mcp = MCPServer("fedora-distgit") + +# MCP 2 runs synchronous tools in worker threads. Preserve the serialized +# state and filesystem access that these tools relied on under MCP 1. +_tool_lock = Lock() # Load .env config — may set AZLDEV_WORK_DIR etc. load_env() @@ -204,7 +209,8 @@ def distgit_status() -> StatusDict: Returns the configured base URL, scratch directory, and cached repos. """ - return _add_status({}, full=True) + with _tool_lock: + return _add_status({}, full=True) @mcp.tool() @@ -214,12 +220,13 @@ def set_distgit_url(base_url: str) -> StatusDict: Defaults to https://src.fedoraproject.org. Only needs to be called if using a mirror or alternate instance.""" global _base_url - old_url = _base_url - normalized, err = validate_base_url(base_url) - if err: - return _add_status({"error": err}, full=False) - _base_url = normalized - return _add_status({"old_url": old_url}, full=False) + with _tool_lock: + old_url = _base_url + normalized, err = validate_base_url(base_url) + if err: + return _add_status({"error": err}, full=False) + _base_url = normalized + return _add_status({"old_url": old_url}, full=False) @mcp.tool() @@ -235,49 +242,50 @@ def distgit_fetch(path: str, override_base_url: str | None = None) -> StatusDict - /rpms/atlas/raw/rawhide/f/atlas.spec (raw spec file) Response is written to a temp file. Use read_file or grep_search to inspect.""" - if override_base_url: - base, err = validate_base_url(override_base_url) - if err: - return _add_status({"error": err}, full=False) - else: - base = _base_url + with _tool_lock: + if override_base_url: + base, err = validate_base_url(override_base_url) + if err: + return _add_status({"error": err}, full=False) + else: + base = _base_url - if not path.startswith("/"): - return _add_status({"error": "path must start with '/'"}, full=False) + if not path.startswith("/"): + return _add_status({"error": "path must start with '/'"}, full=False) - url = base + path + url = base + path - # Guard against SSRF via URL authority tricks - ssrf_err = check_ssrf(base, url) - if ssrf_err: - return _add_status({"error": ssrf_err}, full=False) + # Guard against SSRF via URL authority tricks + ssrf_err = check_ssrf(base, url) + if ssrf_err: + return _add_status({"error": ssrf_err}, full=False) - req = urllib.request.Request(url, headers={"User-Agent": "fedora-distgit-mcp/1.0"}) + req = urllib.request.Request(url, headers={"User-Agent": "fedora-distgit-mcp/1.0"}) - try: - with urllib.request.urlopen(req, timeout=15) as resp: - data = resp.read() - except urllib.error.HTTPError as e: - return _add_status({"error": f"HTTP {e.code} fetching {url}: {e.reason}"}, full=False) - except urllib.error.URLError as e: - return _add_status({"error": f"can't fetch {url}: {e.reason}"}, full=False) - except Exception as e: - return _add_status({"error": f"can't fetch {url}: {e}"}, full=False) + try: + with urllib.request.urlopen(req, timeout=15) as resp: + data = resp.read() + except urllib.error.HTTPError as e: + return _add_status({"error": f"HTTP {e.code} fetching {url}: {e.reason}"}, full=False) + except urllib.error.URLError as e: + return _add_status({"error": f"can't fetch {url}: {e.reason}"}, full=False) + except Exception as e: + return _add_status({"error": f"can't fetch {url}: {e}"}, full=False) - try: - text = data.decode("utf-8") - except UnicodeDecodeError: - text = data.decode("latin-1") + try: + text = data.decode("utf-8") + except UnicodeDecodeError: + text = data.decode("latin-1") - # Pretty-print JSON responses for readability - try: - parsed = json.loads(text) - text = json.dumps(parsed, indent=2) - except (json.JSONDecodeError, ValueError): - pass + # Pretty-print JSON responses for readability + try: + parsed = json.loads(text) + text = json.dumps(parsed, indent=2) + except (json.JSONDecodeError, ValueError): + pass - output = write_output(text, output_dir=_fetch_dir, prefix="distgit_") - return _add_status({"output": output}, full=False) + output = write_output(text, output_dir=_fetch_dir, prefix="distgit_") + return _add_status({"output": output}, full=False) @mcp.tool() @@ -311,115 +319,116 @@ def distgit_search( Results are written to a temp file. Use read_file or grep_search to inspect. """ - if override_base_url: - base, err = validate_base_url(override_base_url) - if err: - return _add_status({"error": err}, full=False) - else: - base = _base_url - - valid_modes = ("pickaxe", "grep", "log-grep") - if mode not in valid_modes: - return _add_status({"error": f"mode must be one of {valid_modes}, got {mode!r}"}, full=False) - if not query: - return _add_status({"error": "query must not be empty."}, full=False) - if ref != "--all" and ref.startswith("-"): - return _add_status( - {"error": f"ref must not start with '-' (got {ref!r}). Use a branch name like 'rawhide'."}, - full=False, - ) - - repo_dir, err = _ensure_repo(package, auto_clean, base) - if err: - return _add_status({"error": err}, full=False) - - git_dir = _git_dir(package, base) - - # Build the git command - if mode == "pickaxe": - ref_args = ["--all"] if ref == "--all" else [ref] - cmd = [ - "git", - "--git-dir", - git_dir, - "log", - "--oneline", - "-20", - f"-S{query}", - *ref_args, - "--", - ] - elif mode == "grep": - if ref == "--all": + with _tool_lock: + if override_base_url: + base, err = validate_base_url(override_base_url) + if err: + return _add_status({"error": err}, full=False) + else: + base = _base_url + + valid_modes = ("pickaxe", "grep", "log-grep") + if mode not in valid_modes: + return _add_status({"error": f"mode must be one of {valid_modes}, got {mode!r}"}, full=False) + if not query: + return _add_status({"error": "query must not be empty."}, full=False) + if ref != "--all" and ref.startswith("-"): return _add_status( - {"error": "--all is not supported for grep mode; specify a single ref (e.g. 'rawhide')."}, + {"error": f"ref must not start with '-' (got {ref!r}). Use a branch name like 'rawhide'."}, full=False, ) - cmd = [ - "git", - "--git-dir", - git_dir, - "grep", - "-n", - "-i", - "-e", - query, - ref, - "--", - ] - elif mode == "log-grep": - ref_args = ["--all"] if ref == "--all" else [ref] - cmd = [ - "git", - "--git-dir", - git_dir, - "log", - "--oneline", - "-20", - f"--grep={query}", - *ref_args, - ] - try: - result = subprocess.run( - cmd, - capture_output=True, - text=True, - timeout=30, - ) - except subprocess.TimeoutExpired: - return _add_status({"error": "Search timed out after 30s."}, full=False) - except Exception as e: - return _add_status({"error": f"running git: {e}"}, full=False) - - output = result.stdout - if result.returncode != 0 and not output: - # git grep returns 1 for "no match" — that's expected - if mode == "grep" and result.returncode == 1: + repo_dir, err = _ensure_repo(package, auto_clean, base) + if err: + return _add_status({"error": err}, full=False) + + git_dir = _git_dir(package, base) + + # Build the git command + if mode == "pickaxe": + ref_args = ["--all"] if ref == "--all" else [ref] + cmd = [ + "git", + "--git-dir", + git_dir, + "log", + "--oneline", + "-20", + f"-S{query}", + *ref_args, + "--", + ] + elif mode == "grep": + if ref == "--all": + return _add_status( + {"error": "--all is not supported for grep mode; specify a single ref (e.g. 'rawhide')."}, + full=False, + ) + cmd = [ + "git", + "--git-dir", + git_dir, + "grep", + "-n", + "-i", + "-e", + query, + ref, + "--", + ] + elif mode == "log-grep": + ref_args = ["--all"] if ref == "--all" else [ref] + cmd = [ + "git", + "--git-dir", + git_dir, + "log", + "--oneline", + "-20", + f"--grep={query}", + *ref_args, + ] + + try: + result = subprocess.run( + cmd, + capture_output=True, + text=True, + timeout=30, + ) + except subprocess.TimeoutExpired: + return _add_status({"error": "Search timed out after 30s."}, full=False) + except Exception as e: + return _add_status({"error": f"running git: {e}"}, full=False) + + output = result.stdout + if result.returncode != 0 and not output: + # git grep returns 1 for "no match" — that's expected + if mode == "grep" and result.returncode == 1: + return _add_status( + {"output": f"No matches found for {query!r} in {package} at {ref}."}, + full=False, + ) + stderr = result.stderr.strip() + return _add_status({"error": f"git exited with {result.returncode}: {stderr}"}, full=False) + + if not output.strip(): return _add_status( - {"output": f"No matches found for {query!r} in {package} at {ref}."}, + { + "output": f"No matches found for {query!r} in {package} ({mode} on {ref}).", + "repo_dir": repo_dir, + }, full=False, ) - stderr = result.stderr.strip() - return _add_status({"error": f"git exited with {result.returncode}: {stderr}"}, full=False) - if not output.strip(): - return _add_status( - { - "output": f"No matches found for {query!r} in {package} ({mode} on {ref}).", - "repo_dir": repo_dir, - }, - full=False, + lines = output.count("\n") + written = write_output( + output, + output_dir=_fetch_dir, + prefix=f"distgit_{package}_{mode}_", + extra_msg=f"Found {lines} result(s). Repo cloned at: {repo_dir}", ) - - lines = output.count("\n") - written = write_output( - output, - output_dir=_fetch_dir, - prefix=f"distgit_{package}_{mode}_", - extra_msg=f"Found {lines} result(s). Repo cloned at: {repo_dir}", - ) - return _add_status({"output": written, "repo_dir": repo_dir}, full=False) + return _add_status({"output": written, "repo_dir": repo_dir}, full=False) @mcp.tool() @@ -441,52 +450,53 @@ def distgit_show( override_base_url: If provided, clone from this dist-git instance instead of the default. """ - if override_base_url: - base, err = validate_base_url(override_base_url) + with _tool_lock: + if override_base_url: + base, err = validate_base_url(override_base_url) + if err: + return _add_status({"error": err}, full=False) + else: + base = _base_url + + # Validate commit is a hex SHA (prevents argument injection when commit + # appears before "--" in the arg list) + if not re.match(r"^[a-fA-F0-9]{4,40}$", commit): + return _add_status({"error": "commit must be a hex SHA hash (4-40 chars)."}, full=False) + + repo_dir, err = _ensure_repo(package, auto_clean, base) if err: return _add_status({"error": err}, full=False) - else: - base = _base_url - # Validate commit is a hex SHA (prevents argument injection when commit - # appears before "--" in the arg list) - if not re.match(r"^[a-fA-F0-9]{4,40}$", commit): - return _add_status({"error": "commit must be a hex SHA hash (4-40 chars)."}, full=False) + git_dir = _git_dir(package, base) - repo_dir, err = _ensure_repo(package, auto_clean, base) - if err: - return _add_status({"error": err}, full=False) + try: + result = subprocess.run( + ["git", "--git-dir", git_dir, "show", "--stat", "--patch", commit, "--"], + capture_output=True, + text=True, + timeout=30, + ) + except subprocess.TimeoutExpired: + return _add_status({"error": "git show timed out after 30s."}, full=False) + except Exception as e: + return _add_status({"error": f"running git: {e}"}, full=False) - git_dir = _git_dir(package, base) + if result.returncode != 0: + stderr = result.stderr.strip() + return _add_status( + {"error": f"git show failed (exit {result.returncode}): {stderr}"}, + full=False, + ) - try: - result = subprocess.run( - ["git", "--git-dir", git_dir, "show", "--stat", "--patch", commit, "--"], - capture_output=True, - text=True, - timeout=30, - ) - except subprocess.TimeoutExpired: - return _add_status({"error": "git show timed out after 30s."}, full=False) - except Exception as e: - return _add_status({"error": f"running git: {e}"}, full=False) + output = result.stdout - if result.returncode != 0: - stderr = result.stderr.strip() - return _add_status( - {"error": f"git show failed (exit {result.returncode}): {stderr}"}, - full=False, + written = write_output( + output, + output_dir=_fetch_dir, + prefix=f"distgit_{package}_show_", + extra_msg=f"Repo cloned at: {repo_dir}", ) - - output = result.stdout - - written = write_output( - output, - output_dir=_fetch_dir, - prefix=f"distgit_{package}_show_", - extra_msg=f"Repo cloned at: {repo_dir}", - ) - return _add_status({"output": written, "repo_dir": repo_dir}, full=False) + return _add_status({"output": written, "repo_dir": repo_dir}, full=False) @mcp.tool() @@ -497,34 +507,35 @@ def distgit_cleanup(remove_repos: bool = True) -> StatusDict: remove_repos: If true (default), also remove all cached git repos. Set to false to only clean fetched files while preserving clones. """ - removed_files = 0 - removed_bytes = 0 - - # Clean fetched files - if os.path.isdir(_fetch_dir): - for entry in os.scandir(_fetch_dir): - if entry.is_file(): - removed_bytes += entry.stat().st_size - Path(entry.path).unlink() - removed_files += 1 - - # Clean repos - removed_repos_count = 0 - if remove_repos and os.path.isdir(_repos_dir): - # Count actual repos (hostname/package) before bulk-removing the tree. - removed_repos_count = len(_cached_repos()) - for entry in os.scandir(_repos_dir): - if entry.is_dir(): - shutil.rmtree(entry.path, ignore_errors=True) - - return _add_status( - { - "files_removed": removed_files, - "bytes_reclaimed": removed_bytes, - "repos_removed": removed_repos_count, - }, - full=False, - ) + with _tool_lock: + removed_files = 0 + removed_bytes = 0 + + # Clean fetched files + if os.path.isdir(_fetch_dir): + for entry in os.scandir(_fetch_dir): + if entry.is_file(): + removed_bytes += entry.stat().st_size + Path(entry.path).unlink() + removed_files += 1 + + # Clean repos + removed_repos_count = 0 + if remove_repos and os.path.isdir(_repos_dir): + # Count actual repos (hostname/package) before bulk-removing the tree. + removed_repos_count = len(_cached_repos()) + for entry in os.scandir(_repos_dir): + if entry.is_dir(): + shutil.rmtree(entry.path, ignore_errors=True) + + return _add_status( + { + "files_removed": removed_files, + "bytes_reclaimed": removed_bytes, + "repos_removed": removed_repos_count, + }, + full=False, + ) if __name__ == "__main__": diff --git a/scripts/mcps/koji-mcp.py b/scripts/mcps/koji-mcp.py index 8e67fe9a177..c1e066bc2be 100755 --- a/scripts/mcps/koji-mcp.py +++ b/scripts/mcps/koji-mcp.py @@ -23,10 +23,11 @@ import urllib.error import urllib.request from pathlib import Path +from threading import Lock from urllib.parse import urlparse from _mcp_utils import ( - FastMCP, + MCPServer, StatusDict, check_ssrf, load_env, @@ -34,7 +35,11 @@ write_output, ) -mcp = FastMCP("koji") +mcp = MCPServer("koji") + +# MCP 2 runs synchronous tools in worker threads. Preserve the serialized +# state and filesystem access that these tools relied on under MCP 1. +_tool_lock = Lock() # Load .env config — may set KOJI_BASE_URL and KOJI_INSECURE_URLS load_env() @@ -101,7 +106,8 @@ def koji_status() -> StatusDict: This returns a snapshot of the current state of the MCP server, including the URL configuration. """ - return _add_status({}, full=True) + with _tool_lock: + return _add_status({}, full=True) @mcp.tool() @@ -116,14 +122,15 @@ def set_koji_url(base_url: str) -> StatusDict: allow resetting it at runtime. """ global _base_url - old_url = _base_url - normalized, err = validate_base_url(base_url) - if err: - return _add_status({"error": err}, full=False) + with _tool_lock: + old_url = _base_url + normalized, err = validate_base_url(base_url) + if err: + return _add_status({"error": err}, full=False) - _base_url = normalized + _base_url = normalized - return _add_status({"old_url": old_url}, full=False) + return _add_status({"old_url": old_url}, full=False) @mcp.tool() @@ -137,33 +144,34 @@ def koji_allow_insecure(override_base_url: str | None = None) -> StatusDict: DO NOT call this tool without first confirming with the user that they want to allow insecure connections, and that they understand the security implications. """ - if override_base_url: - url, err = validate_base_url(override_base_url) - if err: - return _add_status({"error": err}, full=False) - else: - url = _base_url - - if not url: - return _add_status( - {"error": "No Koji URL available. Pass override_base_url or call set_koji_url first."}, - full=False, - ) - - if url not in _ssl_errors_seen: - return _add_status( - { - "error": ( - "Cannot enable insecure mode — no SSL error has been " - f"observed for {url}. Call koji_fetch first; if it " - "fails with a certificate error, then call this tool." - ) - }, - full=False, - ) + with _tool_lock: + if override_base_url: + url, err = validate_base_url(override_base_url) + if err: + return _add_status({"error": err}, full=False) + else: + url = _base_url + + if not url: + return _add_status( + {"error": "No Koji URL available. Pass override_base_url or call set_koji_url first."}, + full=False, + ) + + if url not in _ssl_errors_seen: + return _add_status( + { + "error": ( + "Cannot enable insecure mode — no SSL error has been " + f"observed for {url}. Call koji_fetch first; if it " + "fails with a certificate error, then call this tool." + ) + }, + full=False, + ) - _insecure_urls.add(url) - return _add_status({"allowed_url": url}, full=True) + _insecure_urls.add(url) + return _add_status({"allowed_url": url}, full=True) @mcp.tool() @@ -180,101 +188,102 @@ def koji_fetch(path: str, override_base_url: str | None = None) -> StatusDict: Agents can then use read_file, grep_search, shell(tail), shell(head), shell(grep) etc. to inspect specific parts without bloating the LLM context. """ - if override_base_url: - base, err = validate_base_url(override_base_url) - if err: - return _add_status({"error": err}, full=False) - else: - base = _base_url - - if not base: - return _add_status( - {"error": "No Koji URL available. Pass override_base_url or call set_koji_url first."}, - full=False, - ) - - if not path.startswith("/"): - return _add_status({"error": "path must start with '/'"}, full=False) - - url = base + path - - # Guard against SSRF via URL authority tricks (e.g. path="@evil.com/..." or ":8080/...") - ssrf_err = check_ssrf(base, url) - if ssrf_err: - return _add_status({"error": ssrf_err}, full=False) - - parsed_url = urlparse(url) - - # SSL: verify certs by default, only disable if the user explicitly opted in - ssl_ctx = None - if parsed_url.scheme == "https" and base in _insecure_urls: - ssl_ctx = ssl.create_default_context() - ssl_ctx.check_hostname = False - ssl_ctx.verify_mode = ssl.CERT_NONE - - req = urllib.request.Request(url, headers={"User-Agent": "koji-mcp/1.0"}) - try: - with urllib.request.urlopen(req, context=ssl_ctx, timeout=10) as resp: - data = resp.read() - except urllib.error.URLError as e: - # urllib wraps SSL errors inside URLError.reason - if isinstance(e.reason, (ssl.SSLCertVerificationError, ssl.SSLError)): - _ssl_errors_seen.add(base) + with _tool_lock: + if override_base_url: + base, err = validate_base_url(override_base_url) + if err: + return _add_status({"error": err}, full=False) + else: + base = _base_url + + if not base: + return _add_status( + {"error": "No Koji URL available. Pass override_base_url or call set_koji_url first."}, + full=False, + ) + + if not path.startswith("/"): + return _add_status({"error": "path must start with '/'"}, full=False) + + url = base + path + + # Guard against SSRF via URL authority tricks (e.g. path="@evil.com/..." or ":8080/...") + ssrf_err = check_ssrf(base, url) + if ssrf_err: + return _add_status({"error": ssrf_err}, full=False) + + parsed_url = urlparse(url) + + # SSL: verify certs by default, only disable if the user explicitly opted in + ssl_ctx = None + if parsed_url.scheme == "https" and base in _insecure_urls: + ssl_ctx = ssl.create_default_context() + ssl_ctx.check_hostname = False + ssl_ctx.verify_mode = ssl.CERT_NONE + + req = urllib.request.Request(url, headers={"User-Agent": "koji-mcp/1.0"}) + try: + with urllib.request.urlopen(req, context=ssl_ctx, timeout=10) as resp: + data = resp.read() + except urllib.error.URLError as e: + # urllib wraps SSL errors inside URLError.reason + if isinstance(e.reason, (ssl.SSLCertVerificationError, ssl.SSLError)): + _ssl_errors_seen.add(base) + return _add_status( + { + "error": ( + f"SSL certificate verification failed for {url}: " + f"{e.reason}. " + "The server **may** be using a self-signed " + "certificate (don't assume — it could be a " + "misconfiguration or an attack). " + "You **MUST** inform the user about the security " + "implications of allowing insecure connections, " + "then offer the user a selection of two options: " + "proceed or abort (use 'ask_questions/ask_user' " + "tools if available, with 'no' as the " + "default/first option). " + "If the user chooses to proceed, call the " + "koji_allow_insecure tool. " + "DO NOT proceed without explicit user approval " + "for the SPECIFIC URL." + ) + }, + full=False, + ) return _add_status( { "error": ( - f"SSL certificate verification failed for {url}: " - f"{e.reason}. " - "The server **may** be using a self-signed " - "certificate (don't assume — it could be a " - "misconfiguration or an attack). " - "You **MUST** inform the user about the security " - "implications of allowing insecure connections, " - "then offer the user a selection of two options: " - "proceed or abort (use 'ask_questions/ask_user' " - "tools if available, with 'no' as the " - "default/first option). " - "If the user chooses to proceed, call the " - "koji_allow_insecure tool. " - "DO NOT proceed without explicit user approval " - "for the SPECIFIC URL." + f"can't fetch {url}: {e}. " + "NOTE: Koji is typically only accessible via a secure connection " + "(e.g., VPN or corporate network). If you are seeing connection " + "errors or timeouts, please verify that you are connected to the " + "appropriate network before retrying." + ) + }, + full=False, + ) + except Exception as e: + return _add_status( + { + "error": ( + f"can't fetch {url}: {e}. " + "NOTE: Koji is typically only accessible via a secure connection " + "(e.g., VPN or corporate network). If you are seeing connection " + "errors or timeouts, please verify that you are connected to the " + "appropriate network before retrying." ) }, full=False, ) - return _add_status( - { - "error": ( - f"can't fetch {url}: {e}. " - "NOTE: Koji is typically only accessible via a secure connection " - "(e.g., VPN or corporate network). If you are seeing connection " - "errors or timeouts, please verify that you are connected to the " - "appropriate network before retrying." - ) - }, - full=False, - ) - except Exception as e: - return _add_status( - { - "error": ( - f"can't fetch {url}: {e}. " - "NOTE: Koji is typically only accessible via a secure connection " - "(e.g., VPN or corporate network). If you are seeing connection " - "errors or timeouts, please verify that you are connected to the " - "appropriate network before retrying." - ) - }, - full=False, - ) - try: - text = data.decode("utf-8") - except UnicodeDecodeError: - text = data.decode("latin-1") + try: + text = data.decode("utf-8") + except UnicodeDecodeError: + text = data.decode("latin-1") - output = write_output(text, output_dir=_output_dir, prefix="koji_") - return _add_status({"output": output}, full=False) + output = write_output(text, output_dir=_output_dir, prefix="koji_") + return _add_status({"output": output}, full=False) @mcp.tool() @@ -284,18 +293,19 @@ def koji_cleanup() -> StatusDict: Call this to reclaim disk space after a triage session, or when starting a fresh investigation. """ - if not _output_dir.is_dir(): - return _add_status({"files_removed": 0}, full=False) - - count = 0 - total_bytes = 0 - for entry in os.scandir(_output_dir): - if entry.is_file(): - total_bytes += entry.stat().st_size - Path(entry.path).unlink() - count += 1 - - return _add_status({"files_removed": count, "bytes_reclaimed": total_bytes}, full=False) + with _tool_lock: + if not _output_dir.is_dir(): + return _add_status({"files_removed": 0}, full=False) + + count = 0 + total_bytes = 0 + for entry in os.scandir(_output_dir): + if entry.is_file(): + total_bytes += entry.stat().st_size + Path(entry.path).unlink() + count += 1 + + return _add_status({"files_removed": count, "bytes_reclaimed": total_bytes}, full=False) if __name__ == "__main__": diff --git a/scripts/mcps/requirements.txt b/scripts/mcps/requirements.txt index 44cbacdb185..c124b91adce 100644 --- a/scripts/mcps/requirements.txt +++ b/scripts/mcps/requirements.txt @@ -1,2 +1,2 @@ -mcp==1.28.1 +mcp==2.0.0 python-dotenv==1.2.2 diff --git a/scripts/mcps/tests/__init__.py b/scripts/mcps/tests/__init__.py new file mode 100644 index 00000000000..1a0964f7c74 --- /dev/null +++ b/scripts/mcps/tests/__init__.py @@ -0,0 +1,3 @@ +# Copyright (c) 2026 Microsoft Corporation. +# Licensed under the MIT License. +"""Tests for the Azure Linux MCP servers.""" diff --git a/scripts/mcps/tests/conftest.py b/scripts/mcps/tests/conftest.py new file mode 100644 index 00000000000..7bc83f37987 --- /dev/null +++ b/scripts/mcps/tests/conftest.py @@ -0,0 +1,10 @@ +# Copyright (c) 2026 Microsoft Corporation. +# Licensed under the MIT License. +"""Configure imports for MCP server tests.""" + +from __future__ import annotations + +import sys +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) diff --git a/scripts/mcps/tests/requirements.txt b/scripts/mcps/tests/requirements.txt new file mode 100644 index 00000000000..e2c1b76505f --- /dev/null +++ b/scripts/mcps/tests/requirements.txt @@ -0,0 +1,2 @@ +-r ../requirements.txt +pytest==9.0.3 diff --git a/scripts/mcps/tests/test_distgit_concurrency.py b/scripts/mcps/tests/test_distgit_concurrency.py new file mode 100644 index 00000000000..32d4d050e04 --- /dev/null +++ b/scripts/mcps/tests/test_distgit_concurrency.py @@ -0,0 +1,95 @@ +# Copyright (c) 2026 Microsoft Corporation. +# Licensed under the MIT License. +"""Concurrency regression tests for the Fedora dist-git MCP server.""" + +from __future__ import annotations + +import asyncio +import importlib.util +import subprocess +import tempfile +import threading +from pathlib import Path +from typing import TYPE_CHECKING + +import _mcp_utils +import pytest +from mcp import Client + +if TYPE_CHECKING: + from types import ModuleType + + from mcp.types import CallToolResult + +_MCP_DIR = Path(__file__).resolve().parents[1] +_REPO_ROOT = Path(__file__).resolve().parents[3] + + +def _load_distgit_module(monkeypatch: pytest.MonkeyPatch) -> ModuleType: + """Load the hyphenated MCP entrypoint without starting stdio.""" + monkeypatch.setattr(_mcp_utils, "load_env", lambda: None) + script_path = _MCP_DIR / "fedora-distgit-mcp.py" + spec = importlib.util.spec_from_file_location("_test_fedora_distgit_mcp", script_path) + if spec is None or spec.loader is None: + pytest.fail(f"Unable to load module spec for {script_path}") + + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_concurrent_search_clones_package_once(monkeypatch: pytest.MonkeyPatch) -> None: + """Serialize concurrent searches that populate the same cache entry.""" + scratch_parent = _REPO_ROOT / "base" / "build" / "work" / "scratch" + scratch_parent.mkdir(parents=True, exist_ok=True) + + with tempfile.TemporaryDirectory(prefix="distgit-concurrency-", dir=scratch_parent) as work_dir: + distgit = _load_distgit_module(monkeypatch) + monkeypatch.setattr(distgit, "_repos_dir", str(Path(work_dir) / "repos")) + + first_clone_started = threading.Event() + second_clone_started = threading.Event() + clone_count_lock = threading.Lock() + clone_count = 0 + + def fake_run(command: list[str], **_kwargs: object) -> subprocess.CompletedProcess[str]: + nonlocal clone_count + + if len(command) > 1 and command[1] == "clone": + with clone_count_lock: + clone_count += 1 + current_clone = clone_count + + if current_clone == 1: + first_clone_started.set() + second_clone_started.wait(timeout=0.5) + else: + second_clone_started.set() + + (Path(command[-1]) / ".git").mkdir(parents=True, exist_ok=True) + return subprocess.CompletedProcess(command, 0, "", "") + + if "grep" in command: + return subprocess.CompletedProcess(command, 1, "", "") + + return subprocess.CompletedProcess(command, 0, "", "") + + monkeypatch.setattr(distgit.subprocess, "run", fake_run) + + async def run_searches() -> list[CallToolResult]: + async with Client(distgit.mcp, raise_exceptions=True) as client: + arguments = {"package": "example", "query": "needle", "mode": "grep"} + first = asyncio.create_task(client.call_tool("distgit_search", arguments)) + if not await asyncio.to_thread(first_clone_started.wait, 1): + pytest.fail("First search did not begin cloning") + + second = asyncio.create_task(client.call_tool("distgit_search", arguments)) + return list(await asyncio.wait_for(asyncio.gather(first, second), timeout=3)) + + results = asyncio.run(run_searches()) + + if clone_count != 1: + pytest.fail(f"Expected one clone for concurrent searches, got {clone_count}") + for result in results: + if result.is_error: + pytest.fail(f"Concurrent search failed: {result}")