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}")