diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index aad76d7..c5801dc 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -8,6 +8,8 @@ on: - 'test/**' - 'pyproject.toml' - 'uv.lock' + - 'server.json' + - 'scripts/bump_version.py' - '.github/workflows/test.yml' pull_request: branches: ['*'] @@ -16,36 +18,48 @@ on: - 'test/**' - 'pyproject.toml' - 'uv.lock' + - 'server.json' + - 'scripts/bump_version.py' - '.github/workflows/test.yml' permissions: contents: read jobs: + versions: + name: Check version consistency + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v7 + + - name: Check server.json matches pyproject.toml + run: python3 scripts/bump_version.py --check + test: runs-on: ubuntu-latest strategy: matrix: python-version: ['3.12', '3.13', '3.14'] fail-fast: false # Continue testing other versions if one fails - + steps: - name: Checkout code uses: actions/checkout@v7 - + - name: Install uv uses: astral-sh/setup-uv@v8.2.0 with: enable-cache: true cache-suffix: ${{ matrix.python-version }} - + - name: Set up Python ${{ matrix.python-version }} run: uv python install ${{ matrix.python-version }} - + - name: Install dependencies run: | uv sync --all-groups --locked --python ${{ matrix.python-version }} - name: Run tests run: | - uv run --python ${{ matrix.python-version }} pytest -v \ No newline at end of file + uv run --python ${{ matrix.python-version }} pytest -v diff --git a/contributing.md b/contributing.md index 3b1e45f..64749a6 100644 --- a/contributing.md +++ b/contributing.md @@ -11,8 +11,9 @@ Please note we have a code of conduct, please follow it in all your interactions build. 2. Update the readme.md with details of changes to the interface; this includes new environment variables, exposed ports, useful file locations, and container parameters. -3. Increase the version numbers in any example files and pyproject.toml to the new version that this - Pull Request would represent. The versioning scheme we use is [SemVer](http://semver.org/). +3. Bump the version to the one this Pull Request would represent with `scripts/bump_version.py` + (`major`, `minor`, `patch`, or an explicit version). It updates pyproject.toml, uv.lock and + server.json together. The versioning scheme we use is [SemVer](http://semver.org/). 4. You may merge the Pull Request in once you have the sign-off of two other developers, or if you do not have permission to do that, you may request the second reviewer to merge it for you. diff --git a/docs/config-reload.md b/docs/config-reload.md index e89d76e..32d1207 100644 --- a/docs/config-reload.md +++ b/docs/config-reload.md @@ -33,7 +33,7 @@ When a configuration change is detected, Magg: - `MAGG_AUTO_RELOAD`: Enable/disable automatic config reloading (default: `true`) - `MAGG_RELOAD_POLL_INTERVAL`: File check interval in seconds when polling (default: `1.0`) -- `MAGG_RELOAD_USE_WATCHDOG`: Force watchdog on/off, or auto-detect (default: `null` for auto) +- `MAGG_RELOAD_USE_WATCHDOG`: `true` to use file system notifications, `false` to always poll (default: auto-detect) - `MAGG_READ_ONLY`: When `true`, Magg cannot modify config but can still reload external changes ### Example @@ -203,11 +203,12 @@ INFO: Configuration reload complete The logs will show which validation failed: ``` -ERROR: Duplicate prefix 'test' found in servers 'server1' and 'server2' ERROR: Server 'myserver' has neither command nor uri ``` -Fix the configuration issue and save again. +A file that isn't valid JSON is not applied, and Magg won't save over it (so a typo can't +wipe your configuration). Server entries that fail validation are skipped when loading but +kept in the file when Magg saves. Fix the configuration issue and save again. ### Server not responding after reload diff --git a/docs/index.md b/docs/index.md index aa92a55..bfdc642 100644 --- a/docs/index.md +++ b/docs/index.md @@ -239,7 +239,7 @@ Add a new MCP server to the configuration. - `name` (str, required): Unique server name - `source` (str, required): URL or URI of the server package/repository (can be a local path as well) - `command` (str, optional): Full command to run (e.g., "npx @playwright/mcp@latest") -- `prefix` (str, optional): Tool prefix (defaults to server name) +- `prefix` (str, optional): Tool prefix (default: none, tools keep their original names) - `uri` (str, optional): URI for HTTP servers - `env` (dict|str, optional): Environment variables (can be a dict or JSON string) - `cwd` (str, optional): Working directory @@ -345,7 +345,7 @@ Perform health checks on configured servers with optional remediation actions. **Parameters:** - `action` (str, optional): Action to perform - "report" (default), "remount", "unmount", or "disable" -- `timeout` (float, optional): Health check timeout in seconds (default: 0.5) +- `timeout` (float, optional): Health check timeout in seconds (default: 2.5) **Actions:** - `report`: Check server health and return status report @@ -565,9 +565,10 @@ Kits are a way to bundle related MCP servers together for easy installation and ### Kit Discovery -Magg looks for kits in these locations: -1. `$MAGG_KITD_PATH` (defaults to `~/.magg/kit.d`) -2. `.magg/kit.d` in the same directory as your `config.json` +Magg looks for kits in these locations (the first kit found with a given name wins): +1. `kit.d` in the same directory as your `config.json` +2. `kit.d` in each directory on `MAGG_PATH` (colon-separated, default: `./.magg`, then `~/.magg`) +3. Example kits bundled with Magg ### Creating Kits @@ -738,29 +739,32 @@ When configuration changes are detected, Magg: - `MAGG_AUTO_RELOAD` (default: `true`) - Enable/disable automatic configuration reloading - `MAGG_RELOAD_POLL_INTERVAL` (default: `1.0`) - Polling interval in seconds when watchdog unavailable -- `MAGG_RELOAD_USE_WATCHDOG` (default: auto-detect) - Force watchdog on/off or let it auto-detect +- `MAGG_RELOAD_USE_WATCHDOG` (default: auto-detect) - `true` to require file system notifications, `false` to always poll (useful for network filesystems and Docker Desktop bind mounts) - `MAGG_STDERR_SHOW` (default: `false`) - Show stderr output from subprocess MCP servers -- `MAGG_PREFIX_SEP` (default: `_`) - Separator between prefix and tool name +- `MAGG_PREFIX_SEP` (default: `_`) - Separator between Magg's prefix and its own tool names +- `MAGG_BACKEND_INIT_TIMEOUT` (default: `30`) - Seconds to wait for a backend server to start, `0` to wait forever - `MAGG_SELF_PREFIX` (default: `magg`) - Prefix for Magg's own tools ### Environment Variable Inheritance By default, stdio-based MCP servers run with isolated environments. While the recommended approach is to configure environment variables in the server configuration (see Advanced Configuration), you can also pass environment variables at runtime to all stdio subprocesses: +These are global options, so they go before the `serve` command. + **Pass current environment to servers:** ```bash -magg serve --env-pass +magg --env-pass serve ``` **Set specific environment variables:** ```bash -magg serve --env-set API_KEY mykey --env-set LANG en_US.UTF-8 +magg --env-set API_KEY mykey --env-set LANG en_US.UTF-8 serve ``` **Combined usage:** ```bash # Pass current env and add specific variables -magg serve --env-pass --env-set DEBUG true +magg --env-pass --env-set DEBUG true serve ``` This is useful for: @@ -771,6 +775,7 @@ This is useful for: **Note:** - Environment inheritance only affects stdio-based servers. HTTP/URI servers do not receive inherited environment variables. +- A server's own `env` values take precedence over inherited ones. - The preferred method is to set environment variables in the server configuration's `env` field for better reproducibility. - The `mbro` CLI tool also supports these same `--env-pass` and `--env-set` flags when connecting to servers. @@ -996,7 +1001,7 @@ Magg organizes tools from multiple servers using a prefix system. Every tool nam Where: - `prefix` is the namespace for a server (configurable per server) -- `separator` is configurable via `prefix_sep` field or `MAGG_PREFIX_SEP` env var (default: `_`) +- `separator` is `_` (`MAGG_PREFIX_SEP` changes it for Magg's own tools only) - `tool_name` is the original tool name from the server Examples: @@ -1019,7 +1024,7 @@ When adding a server, specify the `prefix` field: Special cases: - `"prefix": ""` or `"prefix": null` - No prefix, tools keep original names -- Omitting prefix field - Server name is used as default prefix +- Omitting the prefix field is the same as `null` - Multiple servers can share the same prefix (tools are merged) #### Configuring Magg's Prefix diff --git a/docs/kits.md b/docs/kits.md index 37a26fc..a270161 100644 --- a/docs/kits.md +++ b/docs/kits.md @@ -13,9 +13,10 @@ When you load a kit into Magg, all its servers are added to your configuration. ## Kit Discovery -Magg looks for kits in these locations: -1. `$MAGG_KITD_PATH` (defaults to `~/.magg/kit.d`) -2. `.magg/kit.d` in the same directory as your `config.json` +Magg looks for kits in these locations (the first kit found with a given name wins): +1. `kit.d` in the same directory as your `config.json` +2. `kit.d` in each directory on `MAGG_PATH` (colon-separated, default: `./.magg`, then `~/.magg`) +3. Example kits bundled with Magg Kit files must have a `.json` extension and follow the kit schema. @@ -37,7 +38,7 @@ Kit files must have a `.json` extension and follow the kit schema. "source": "https://github.com/example/mcp-calc-server", "command": "python", "args": ["-m", "mcp_calc_server"], - "notes": "Basic calculator server", + "notes": "Basic calculator server" } } } diff --git a/docs/mbro.md b/docs/mbro.md index 103d2b9..adaa06d 100644 --- a/docs/mbro.md +++ b/docs/mbro.md @@ -61,13 +61,18 @@ mbro connect calc npx -y @modelcontextprotocol/server-calculator \; tools # Multiple commands separated by semicolons mbro connect magg http://localhost:8080 \; call magg_status -# Read commands from stdin +# Run commands and exit instead of dropping into interactive mode +mbro -n connect calc npx -y @modelcontextprotocol/server-calculator \; call add a=5 b=3 + +# Read commands from stdin (runs non-interactively) echo "connect calc npx calculator; tools" | mbro - # Execute a script file mbro -x setup.mbro ``` +In non-interactive mode (`-n`, `-X`, or `-`), mbro exits with status 1 if any command failed (unknown command, failed connection, no active connection, tool error, invalid arguments, ...). Commands keep running after a failure unless `--fail-fast` is given. + ### Authentication When connecting to HTTP servers that require bearer token authentication, mbro automatically checks for JWT tokens in these environment variables (in order): @@ -123,7 +128,8 @@ mbro:memory> prompt summarize topic=recent_memories max_length=500 | `connections` | List all connections | `connections` | | `connect ` | Connect to a server | `connect calc npx -y @modelcontextprotocol/server-calculator` | | `switch ` | Switch to another connection | `switch calc` | -| `disconnect` | Disconnect from current server | `disconnect` | +| `disconnect ` | Disconnect from a server | `disconnect calc` | +| `status` | Show the current connection's tool, resource, and prompt counts | `status` | ### Tool Operations @@ -152,13 +158,17 @@ mbro:memory> prompt summarize topic=recent_memories max_length=500 |---------|-------------|---------| | `search ` | Search tools, resources, and prompts | `search add` | | `info ` | Get detailed info | `info tool add` | -| `status` | Show connection status | `status` | ### Utility Commands | Command | Description | Example | |---------|-------------|---------| -| `exit` / `quit` | Exit mbro | `exit` | +| `script run ` | Run a script by name or path | `script run setup` | +| `script list [filter]` | List discovered scripts | `script list` | +| `script search ` | Search script names and contents | `script search calc` | +| `script dump ` | Show (or edit and run) a script | `script dump setup` | +| `help` | Show available commands | `help` | +| `exit` / `quit` | Exit mbro (also stops a running script) | `exit` | ## Advanced Features @@ -205,20 +215,21 @@ mbro -x setup.mbro -n - **Comments**: Lines starting with `#` are ignored - **Empty lines**: Ignored for readability - **Line continuation**: Use backslash `\` at end of line +- **Multi-line JSON**: A JSON object argument can span lines until its braces balance - **All commands supported**: Any interactive command works in scripts -- **Error handling**: Scripts continue on errors unless fatal +- **Error handling**: Scripts continue after a failed command (or stop with `--fail-fast`); `quit` stops the script #### Script Discovery mbro can discover scripts in configured paths: ```bash # List available scripts -mbro scripts +mbro -n script list -# Scripts are searched in: -# - Current directory: ./.mbro/ -# - Home directory: ~/.mbro/ -# - MAGG_PATH directories +# Scripts (*.mbro, searched recursively) are found in the Magg path: +# - Project directory: ./.magg/ +# - Home directory: ~/.magg/ +# - Or the directories in MAGG_PATH (colon-separated) ``` #### Example Scripts @@ -247,7 +258,7 @@ call get_test_results format=json # Cleanup call cleanup_test_data -disconnect +disconnect test ``` ### Modern CLI Features @@ -304,12 +315,12 @@ Tab completion provides rich parameter information: - **ESC to cancel**: Exit completion without selecting - **Smart parsing**: Detects existing parameters to avoid duplicates -#### 💡 Intelligent Error Suggestions -When errors occur, get helpful suggestions: +#### 💡 Tool Name Suggestions +Calling an unknown tool suggests close matches: ```bash mbro> call wether {"city": "London"} -Error: Tool 'wether' not found. -Did you mean: weather, whether_tool? +Error: Failed to call tool 'wether': Unknown tool: 'wether' +Did you mean: weather? ``` #### ⚡ Search @@ -335,10 +346,11 @@ mbro supports multiple ways to provide tool arguments: #### JSON Format (Traditional) ```bash -# In interactive mode, use JSON directly (no surrounding quotes) +# In interactive mode and scripts, use JSON directly (surrounding single quotes are optional) call search {"query": "python tutorials", "limit": 10} +call search '{"query": "python tutorials", "limit": 10}' -# Complex nested structures +# Complex nested structures (can span multiple lines) call create_task { "title": "Review PR", "details": { @@ -348,10 +360,12 @@ call create_task { "tags": ["urgent", "review"] } -# From command line, quotes may be needed for shell parsing -mbro --call-tool search '{"query": "python tutorials"}' +# From the command line, quote the JSON so the shell keeps its double quotes +mbro -n connect s python server.py \; call search '{"query": "python tutorials"}' ``` +Prompts accept the same formats: `prompt greet name=Bob` or `prompt greet {"name": "Bob"}`. + #### Shell-Style Key=Value Format (New!) ```bash # Simple key=value pairs (no quotes needed for single words) @@ -367,6 +381,8 @@ call configure debug=true port=8080 timeout=30.5 call create_task title="Review PR" details='{"repo": "myproject", "pr_number": 123}' ``` +Values are decoded as JSON (numbers, `true`/`false`, `null`, arrays, objects) and anything that isn't valid JSON stays a string, so `code=007` and `range=10-20` are passed as strings. If the tool's input schema declares a parameter as a string, its value is always passed as-is (`id=12345` stays `"12345"`). Prompt arguments are always strings. + #### Multiline Support ```bash # Press Enter to continue on next line @@ -400,6 +416,10 @@ mbro connect myserver python -m mypackage.mcp_server \; tools mbro connect api http://localhost:3000 \; status ``` +A URL without a path (like `http://localhost:3000`) connects to the default `/mcp/` endpoint; URLs with a path (`/mcp`, `/sse`, ...) are used as given. + +Connecting fails with the underlying reason (for example, a missing command or a refused connection). If the server doesn't finish initializing within 30 seconds, the connection attempt times out; use `--timeout SECONDS` to change this (`0` disables it). + ### Running Magg in stdio Mode A unique feature of mbro is the ability to run Magg directly in stdio mode for quick inspection: @@ -446,16 +466,17 @@ mbro --repl ``` In the Python REPL, you have access to: -- `current_connection`: The active MCP connection object +- `current_connection`: The connection that was active when the REPL started (e.g. from a `-x` script), or `None` - `self`: The MCPBrowserCLI instance for executing commands - All standard Python functionality with async/await support Example REPL session: ```python >>> # Direct access to the current connection ->>> tools = await current_connection.list_tools() ->>> print(tools[0].name) -'add' +>>> conn = self.browser.get_current_connection() +>>> tools = await conn.get_tools() +>>> print(tools[0]["name"]) +add >>> # Execute mbro commands via self >>> await self.handle_command('tools') @@ -464,7 +485,7 @@ Available tools: ... >>> # Call a tool directly ->>> result = await current_connection.call_tool('add', {'a': 5, 'b': 3}) +>>> result = await conn.call_tool('add', {'a': 5, 'b': 3}) >>> print(result) [TextContent(type='text', text='8')] ``` @@ -483,7 +504,7 @@ When used with Magg, mbro can browse the aggregated server: magg serve --http --port 8000 # In another terminal, connect mbro to Magg -mbro --connect magg http://localhost:8000 +mbro connect magg http://localhost:8000/mcp ``` This allows you to use mbro to explore all tools from all servers managed by Magg through a single interface. @@ -592,7 +613,7 @@ $ cat > weather_check.mbro << 'EOF' # Weather check script connect weather npx -y @modelcontextprotocol/server-weather call get_forecast location="San Francisco" days=3 -disconnect +disconnect weather quit EOF @@ -680,10 +701,14 @@ mbro -q connect calc npx -y @modelcontextprotocol/server-calculator \; call add - `-n` / `--no-interactive` - Don't drop into interactive mode after commands - `-x SCRIPT` / `--execute-script SCRIPT` - Execute .mbro script file (can be used multiple times) - `-X SCRIPT` / `--execute-script-n SCRIPT` - Execute script in non-interactive mode (equivalent to -n -x) + - `--status-bar` - Show status bar with keyboard shortcuts + - `--fail-fast` - Stop running commands and scripts at the first failure + - `--timeout SECONDS` - Server initialization timeout when connecting (0 to disable, default: 30) - `--help` - Show help message Special command line usage: - - Use `-` as command to read from stdin + - Use `-` as command to read commands from stdin (non-interactive) + - In non-interactive mode, the exit status is 1 if any command failed - Use `\;` to separate multiple commands (escape semicolon) - Scripts provide reusable command sequences - Minimal quoting: only quote values with spaces diff --git a/docs/mcp-registry.md b/docs/mcp-registry.md index 7828129..41d7352 100644 --- a/docs/mcp-registry.md +++ b/docs/mcp-registry.md @@ -44,9 +44,9 @@ Note: building from source with `go install .../cmd/publisher@latest` produces a not on `PATH` by default — the prebuilt binary above is the simpler route. **Release checklist**: the `version` fields in `server.json` (both the top-level version -and the pypi package version) must be bumped to match `pyproject.toml` before -publishing, and the PyPI release must already exist — the registry verifies that the -referenced package version is published. +and the pypi package version) must match `pyproject.toml`. `scripts/bump_version.py` keeps +them in sync when bumping, and CI fails if they drift. The PyPI release must already exist +before publishing — the registry verifies that the referenced package version is published. **Ownership verification**: the registry proves control of the PyPI package by requiring the line `mcp-name: io.github.sitbon/magg` in the package README as published on PyPI diff --git a/magg/__main__.py b/magg/__main__.py index d78936a..4f9e80a 100644 --- a/magg/__main__.py +++ b/magg/__main__.py @@ -1,7 +1,3 @@ -from magg import process - -process.setup() - from magg.cli import main main() diff --git a/magg/auth.py b/magg/auth.py index b43cd7c..cedb78b 100644 --- a/magg/auth.py +++ b/magg/auth.py @@ -1,6 +1,7 @@ """Authentication support for Magg.""" import logging +import os import time from functools import cached_property @@ -60,7 +61,7 @@ def generate_keys(self) -> None: private_key = self._generate_keypair() if private_key is None: - raise RuntimeError("Failed to generate keypair") + raise RuntimeError(f"Failed to generate keypair in {self.bearer_config.key_path} (see log for details)") self._private_key = private_key self._public_key = self._derive_public_key(private_key) @@ -86,11 +87,12 @@ def _generate_keypair(self) -> rsa.RSAPrivateKey | None: try: private_key = rsa.generate_private_key(public_exponent=65537, key_size=2048, backend=default_backend()) - self.bearer_config.key_path.mkdir(mode=0o700, exist_ok=True) + self.bearer_config.key_path.mkdir(mode=0o700, parents=True, exist_ok=True) private_path = self.bearer_config.private_key_path - with private_path.open("wb") as f: + # Create owner-only from the start so the key is never readable by others + with os.fdopen(os.open(private_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600), "wb") as f: f.write( private_key.private_bytes( encoding=serialization.Encoding.PEM, @@ -98,7 +100,6 @@ def _generate_keypair(self) -> rsa.RSAPrivateKey | None: encryption_algorithm=serialization.NoEncryption(), ) ) - private_path.chmod(0o600) ssh_public_path = self.bearer_config.public_key_path public_key = private_key.public_key() diff --git a/magg/cli.py b/magg/cli.py index 9205c35..feb27f5 100644 --- a/magg/cli.py +++ b/magg/cli.py @@ -8,6 +8,7 @@ from pathlib import Path from cryptography.hazmat.primitives import serialization +from pydantic import ValidationError from . import __version__, process from .auth import BearerAuthManager @@ -27,7 +28,10 @@ print_warning, ) -process.setup(source=__name__) +try: + process.setup(source=__name__) +except ValidationError: + pass # Reported by main() without a traceback logger: logging.Logger = logging.getLogger(__name__) @@ -737,7 +741,12 @@ async def cmd_auth(args) -> int: auth_manager = BearerAuthManager(auth_config.bearer) - auth_manager.generate_keys() + try: + auth_manager.generate_keys() + except RuntimeError as e: + print_error(str(e)) + return 1 + print_success(f"Generated new RSA keypair for audience '{auth_config.bearer.audience}'") print_text( f"Private key: {auth_config.bearer.key_path}/{auth_config.bearer.audience}.key\n" @@ -748,6 +757,7 @@ async def cmd_auth(args) -> int: if ( auth_config.bearer.issuer != default_config.issuer or auth_config.bearer.audience != default_config.audience + or auth_config.bearer.key_path != default_config.key_path ): if config_manager.save_auth_config(auth_config): print_info(f"Auth config saved to: {config_manager.auth_config_path}") @@ -775,9 +785,6 @@ async def cmd_auth(args) -> int: if auth_config.bearer.public_key_exists: print_info(f"SSH public key exists: {auth_config.bearer.public_key_path}") - - if auth_config.bearer.private_key_env: - print_info("Private key also available via MAGG_PRIVATE_KEY env var") else: print_info("Authentication is DISABLED") print_text("Run 'magg auth init' to enable authentication") @@ -843,7 +850,7 @@ async def cmd_auth(args) -> int: if args.export: single_line = pem.replace("\n", "\\n") - print(f"export MAGG_PRIVATE_KEY={single_line}") + print(f"export MAGG_PRIVATE_KEY={shlex.quote(single_line)}") elif args.oneline: single_line = pem.replace("\n", "\\n") print(single_line) @@ -865,6 +872,9 @@ def create_parser() -> argparse.ArgumentParser: prog="magg", description="Magg - MCP Aggregator: Manage and aggregate MCP servers", epilog='Use "magg --help" for more information about a command.', + # Some Python versions (e.g. 3.12.3) match subcommand options like "server update --env" + # against abbreviations of global options, which fails as ambiguous + allow_abbrev=False, ) parser.add_argument( @@ -1057,10 +1067,26 @@ async def run(): exit(1) +def format_validation_error(error: ValidationError) -> str: + """Describe a settings validation error in a line per field, pointing at the env var for MaggConfig.""" + lines = [] + for err in error.errors(): + field = ".".join(str(part) for part in err["loc"]) + line = f"Invalid setting {field!r}: {err['msg']} (got {err['input']!r})" + if error.title == "MaggConfig" and len(err["loc"]) == 1: + line += f". Check MAGG_{field.upper()} in your environment or .env file." + lines.append(line) + return "\n".join(lines) + + def main(): """Run the CLI.""" - process.setup() - asyncio.run(run()) + try: + process.setup() + asyncio.run(run()) + except ValidationError as e: + print_error(format_validation_error(e)) + exit(1) if __name__ == "__main__": diff --git a/magg/discovery/metadata.py b/magg/discovery/metadata.py index 3c6b653..12a0c11 100644 --- a/magg/discovery/metadata.py +++ b/magg/discovery/metadata.py @@ -237,7 +237,7 @@ async def _collect_github_metadata(self, url: str) -> dict[str, Any]: owner, repo = path_parts[0], path_parts[1] # Fetch from GitHub API - async with aiohttp.ClientSession() as session: + async with aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=15)) as session: api_url = f"https://api.github.com/repos/{owner}/{repo}" async with session.get(api_url) as response: diff --git a/magg/discovery/search.py b/magg/discovery/search.py index 24cd4e3..8ee1d04 100644 --- a/magg/discovery/search.py +++ b/magg/discovery/search.py @@ -3,6 +3,7 @@ TODO: Add support for mcpservers.org. """ +import asyncio import logging from dataclasses import dataclass from typing import Any @@ -35,7 +36,8 @@ def __init__(self): self.session: aiohttp.ClientSession | None = None async def __aenter__(self): - self.session = aiohttp.ClientSession() + # Bounded so an unreachable source can't stall a search, and honor proxy env vars + self.session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=15), trust_env=True) return self async def __aexit__(self, exc_type, exc_val, exc_tb): @@ -92,7 +94,7 @@ def _parse_registry_results(cls, data: dict[str, Any]) -> list[ToolSearchResult] result = ToolSearchResult( name=server.get("name", ""), - description=server.get("description", ""), + description=server.get("description") or "", source="mcp-registry", url=url, tags=tags, @@ -168,7 +170,7 @@ def _parse_glama_results(self, data: dict[str, Any]) -> list[ToolSearchResult]: # Convert glama result to our format result = ToolSearchResult( name=server.get("name", ""), - description=server.get("description", ""), + description=server.get("description") or "", source="glama", url=server.get("url", ""), tags=self._extract_tags(server), @@ -282,14 +284,14 @@ def _parse_github_results(cls, data: dict[str, Any]) -> list[ToolSearchResult]: for item in data.get("items", []): # Try to determine if this is an MCP server is_mcp_server = any( - keyword in item.get("description", "").lower() + keyword in (item.get("description") or "").lower() for keyword in ["mcp", "model context protocol", "mcp server"] ) if is_mcp_server: result = ToolSearchResult( name=item.get("name", ""), - description=item.get("description", ""), + description=item.get("description") or "", source="github", url=item.get("html_url"), tags=item.get("topics", []), @@ -338,14 +340,14 @@ def _parse_npm_results(cls, data: dict[str, Any]) -> list[ToolSearchResult]: result = ToolSearchResult( name=package.get("name", ""), - description=package.get("description", ""), + description=package.get("description") or "", source="npm", url=f"https://www.npmjs.com/package/{package.get('name')}", tags=package.get("keywords", []), install_command=f"npm install {package.get('name')}", metadata={ "version": package.get("version"), - "author": package.get("author", {}).get("name"), + "author": (package.get("author") or {}).get("name"), "license": package.get("license"), }, ) @@ -362,13 +364,15 @@ async def search_all(self, query: str, limit_per_source: int = 5) -> dict[str, l ("npm", self.search_npm(query, limit_per_source)), ] + outcomes = await asyncio.gather(*(task for _, task in tasks), return_exceptions=True) + results = {} - for source, task in tasks: - try: - results[source] = await task - except Exception as e: - self.logger.error("Error searching %s: %s", source, e) + for (source, _), outcome in zip(tasks, outcomes): + if isinstance(outcome, BaseException): + self.logger.error("Error searching %s: %s", source, outcome) results[source] = [] + else: + results[source] = outcome return results @@ -384,7 +388,7 @@ def calculate_score(result: ToolSearchResult) -> float: score += result.rating * 10 # Bonus for certain sources - source_bonus = {"mcp-registry": 6.0, "glama.ai": 5.0, "github": 3.0, "npm": 2.0} + source_bonus = {"mcp-registry": 6.0, "glama": 5.0, "github": 3.0, "npm": 2.0} score += source_bonus.get(result.source, 0.0) # Bonus for having install command diff --git a/magg/kit.py b/magg/kit.py index 819fa42..8a0a655 100644 --- a/magg/kit.py +++ b/magg/kit.py @@ -5,18 +5,17 @@ from pathlib import Path from typing import Any -from pydantic import Field, field_validator -from pydantic_settings import BaseSettings, SettingsConfigDict +from pydantic import BaseModel, ConfigDict, Field, field_validator from .settings import ConfigManager, KitInfo, MaggConfig, ServerConfig logger = logging.getLogger(__name__) -class KitConfig(BaseSettings): +class KitConfig(BaseModel): """Configuration for a kit - a bundle of related MCP servers.""" - model_config = SettingsConfigDict( + model_config = ConfigDict( extra="allow", validate_assignment=True, ) @@ -70,6 +69,10 @@ def __init__(self, config_manager: ConfigManager, kitd_paths: list[Path] | None else: config = MaggConfig() self.kitd_paths = config.get_kitd_paths() + # MAGG_CONFIG_PATH or --config can put the config outside the search path + config_kitd = config_manager.config_path.parent / "kit.d" + if config_kitd.is_dir() and all(config_kitd.resolve() != p.resolve() for p in self.kitd_paths): + self.kitd_paths.insert(0, config_kitd) self._kits: dict[str, KitConfig] = {} def discover_kits(self) -> dict[str, Path]: diff --git a/magg/mbro/cli.py b/magg/mbro/cli.py index 2bcc46d..3568266 100644 --- a/magg/mbro/cli.py +++ b/magg/mbro/cli.py @@ -2,6 +2,8 @@ import argparse import asyncio +import json +import shlex import sys from asyncio import CancelledError from functools import cached_property @@ -22,12 +24,12 @@ arepl = None from .. import __version__, process -from .client import BrowserClient +from .client import DEFAULT_CONNECT_TIMEOUT, BrowserClient from .command import Command from .completers import create_improved_completer from .formatter import OutputFormatter from .multiline import MultilineInputHandler -from .parser import CommandParser, JsonArgParser +from .parser import CommandParser from .validator import InputValidator @@ -72,8 +74,10 @@ def __init__( env_pass: bool = False, env_vars: dict[str, str] | None = None, status_bar: bool = False, + fail_fast: bool = False, + timeout: float | None = DEFAULT_CONNECT_TIMEOUT, ): - self.browser = BrowserClient(env_pass=env_pass, env_vars=env_vars) + self.browser = BrowserClient(env_pass=env_pass, env_vars=env_vars, timeout=timeout) self.running = True self.formatter = OutputFormatter(json_only=json_only, use_rich=use_rich, indent=indent) self.verbose = verbose @@ -81,10 +85,11 @@ def __init__( self.env_pass = env_pass self.env_vars = env_vars self.status_bar = status_bar + self.fail_fast = fail_fast + self.failed = False # Set when a command run by run_commands fails self.command = Command(self) if not json_only: - self.json_parser = JsonArgParser() self.multiline_handler = MultilineInputHandler(self.formatter) self._multiline_buffer = [] @@ -112,6 +117,7 @@ def _completer(self): "status", "search", "info", + "script", ], meta_dict={ "help": "Show this help message", @@ -131,6 +137,7 @@ def _completer(self): "prompt": "Get a prompt by name with optional arguments", "search": "Search tools, resources, and prompts by term", "info": "Show detailed info about a tool/resource/prompt", + "script": "Run or manage .mbro scripts", }, ) @@ -295,15 +302,21 @@ def _create_input_validator(self): return InputValidator(self) @classmethod - def parse_shell_args(cls, args: list[str]) -> dict: + def parse_shell_args(cls, args: list[str], schema: dict | None = None, *, strings: bool = False) -> dict: """Parse shell-style key=value arguments. + Values are decoded as JSON where possible, otherwise kept as strings. Values for + parameters the input schema declares as strings are kept as-is, as are all values + when `strings` is set. + Examples: name="test" -> {"name": "test"} count=42 -> {"count": 42} enabled=true -> {"enabled": true} name="my server" count=5 -> {"name": "my server", "count": 5} + ids=[1,2] code=007 range=10-20 -> {"ids": [1, 2], "code": "007", "range": "10-20"} """ + properties = (schema or {}).get("properties", {}) result = {} for arg in args: @@ -316,23 +329,33 @@ def parse_shell_args(cls, args: list[str]) -> dict: if not key: continue - if value.lower() == "true": - result[key] = True - elif value.lower() == "false": - result[key] = False - elif value.replace(".", "", 1).replace("-", "", 1).isdigit(): - if "." in value: - result[key] = float(value) - else: - result[key] = int(value) + if (value.startswith('"') and value.endswith('"')) or (value.startswith("'") and value.endswith("'")): + result[key] = value[1:-1] + elif strings or cls._is_string_param(properties.get(key)): + result[key] = value + elif value.lower() in ("true", "false"): + result[key] = value.lower() == "true" else: - if (value.startswith('"') and value.endswith('"')) or (value.startswith("'") and value.endswith("'")): - result[key] = value[1:-1] - else: + try: + result[key] = json.loads(value) + except json.JSONDecodeError: result[key] = value return result + @staticmethod + def _is_string_param(prop: dict | None) -> bool: + """Check whether a JSON schema property only accepts strings (or null).""" + if not prop: + return False + + types = set() + for option in prop.get("anyOf") or [prop]: + option_type = option.get("type", "any") + types.update(option_type if isinstance(option_type, list) else [option_type]) + + return types - {"null"} == {"string"} + async def refresh_completer_cache(self): """Refresh the completer cache after connection changes.""" if not self.formatter.json_only and hasattr(self, "_completer"): @@ -350,6 +373,7 @@ async def start(self, repl: bool = False): if repl: if arepl is None: self.formatter.print("REPL mode is only available with Python 3.13+") + return self.formatter.print( "Entering REPL mode. `await self.handle_command(command)` to execute commands.", file=sys.stderr @@ -419,20 +443,6 @@ async def handle_command(self, command: str): cmd = parts[0].lower() args = parts[1:] - if args and any("{" in arg for arg in args): - json_start_idx = None - for i, arg in enumerate(args): - if "{" in arg: - json_start_idx = i - break - - if json_start_idx is not None: - json_part = " ".join(args[json_start_idx:]) - - if json_part.count("{") == json_part.count("}"): - if cmd in ["call", "prompt", "get-prompt"]: - args = args[:json_start_idx] + [json_part] - cmd = self.ALIASES.get(cmd, cmd) if cmd not in self.COMMANDS: @@ -440,17 +450,49 @@ async def handle_command(self, command: str): self.formatter.format_info("Type 'help' for available commands") return - match cmd: - case "help": - self.show_help() - case "quit": - self.running = False - case _: - await getattr(self.command, cmd)(args) + try: + match cmd: + case "help": + self.show_help() + case "quit": + self.running = False + case _: + await getattr(self.command, cmd)(args) + + except Exception as e: + self.formatter.format_error("Unexpected error in command handling", e) if not self.formatter.json_only: self.formatter.print() + async def run_commands(self, commands: list[str]) -> bool: + """Run commands in order, stopping at `quit` (or the first failure with fail_fast). + + A command fails if it reports an error. + + Returns: + True if every command that ran succeeded + """ + ok = True + + for command in commands: + if not self.running: + break + + if self.verbose: + self.formatter.format_info(f"> {command}") + + errors = self.formatter.error_count + await self.handle_command(command) + + if self.formatter.error_count != errors: + ok = False + self.failed = True + if self.fail_fast: + break + + return ok + def show_help(self): """Show help text.""" self.formatter.format_help(enhanced=not self.formatter.json_only) @@ -474,10 +516,7 @@ async def handle_commands(cli: MCPBrowserCLI, args) -> bool: if not commands_to_run: return False - for command in commands_to_run: - if command.strip(): - await cli.handle_command(command) - + await cli.run_commands(commands_to_run) return True @@ -534,6 +573,16 @@ async def main_async(): help="Execute script in non-interactive mode", ) parser.add_argument("--status-bar", action="store_true", help="Show status bar with keyboard shortcuts") + parser.add_argument( + "--fail-fast", action="store_true", help="Stop running commands and scripts at the first failure" + ) + parser.add_argument( + "--timeout", + type=float, + default=DEFAULT_CONNECT_TIMEOUT, + metavar="SECONDS", + help=f"Server initialization timeout (0 to disable, default: {DEFAULT_CONNECT_TIMEOUT:g})", + ) parser.add_argument( "commands", @@ -555,21 +604,29 @@ async def main_async(): env_pass=args.env_pass, env_vars=dict(args.env_set) if args.env_set else None, status_bar=args.status_bar, + fail_fast=args.fail_fast, + timeout=args.timeout, ) + # Commands piped through stdin run non-interactively + if args.commands and args.commands[0] == "-": + args.no_interactive = True + try: if hasattr(args, "script_order") and args.script_order: has_non_interactive_script = any(is_non_interactive for _, is_non_interactive in args.script_order) if has_non_interactive_script: args.no_interactive = True - for script_path, _ in args.script_order: - await cli.handle_command(f"script run {script_path}") + await cli.run_commands([f"script run {shlex.quote(path)}" for path, _ in args.script_order]) commands_executed = False - if args.commands: + if args.commands and not (cli.failed and args.fail_fast): commands_executed = await handle_commands(cli, args) + if cli.failed and (args.no_interactive or args.fail_fast): + sys.exit(1) + if not args.no_interactive: await cli.start(repl=args.repl) elif not commands_executed and not hasattr(args, "script_order"): diff --git a/magg/mbro/client.py b/magg/mbro/client.py index 154fb9e..62b7e3b 100644 --- a/magg/mbro/client.py +++ b/magg/mbro/client.py @@ -3,6 +3,7 @@ import logging import os from typing import Any +from urllib.parse import urlparse from fastmcp import Client from fastmcp.client import BearerAuth @@ -23,6 +24,8 @@ logger = logging.getLogger(__name__) +DEFAULT_CONNECT_TIMEOUT = 30.0 + class BrowserConnection: """Represents a connection to an MCP server.""" @@ -80,23 +83,33 @@ async def get_prompts(self) -> list[dict[str, Any]]: return prompts_data - async def connect(self, env_pass: bool = False, env_vars: dict[str, str] | None = None) -> bool: - """Connect to the MCP server using FastMCP Client.""" + @staticmethod + def normalize_url(url: str) -> str: + """Point a bare server URL (no path) at the default /mcp/ endpoint.""" + parsed = urlparse(url) + if parsed.path in ("", "/"): + return parsed._replace(path="/mcp/").geturl() + return url + + async def connect( + self, env_pass: bool = False, env_vars: dict[str, str] | None = None, timeout: float | None = None + ) -> None: + """Connect to the MCP server using FastMCP Client. + + Raises the underlying error if the server can't be reached or doesn't initialize within `timeout`. + """ jwt = os.getenv("MAGG_JWT", os.getenv("MBRO_JWT", os.getenv("MCP_JWT", None))) auth = BearerAuth(jwt) if jwt else None if is_connection_string_url(self.connection_string): - url = self.connection_string - if not url.endswith("/mcp/"): - url = url.rstrip("/") + "/mcp/" - client = Client(url, auth=auth) + client = Client(self.normalize_url(self.connection_string), auth=auth, init_timeout=timeout) else: from ..util.system import get_subprocess_environment env = get_subprocess_environment(inherit=env_pass, provided=env_vars) transport = get_transport_for_command_string(self.connection_string, env=env) - client = Client(transport, auth=auth) + client = Client(transport, auth=auth, init_timeout=timeout) try: async with client as conn: @@ -106,12 +119,16 @@ async def connect(self, env_pass: bool = False, env_vars: dict[str, str] | None logger.warning("Connected to %r but ping failed", client) except Exception as e: - logger.error("Failed to connect to MCP server: %s", e) - return False + # fastmcp's error doesn't say that initialization timed out + cause = e.__cause__ + while cause is not None and not isinstance(cause, TimeoutError): + cause = cause.__cause__ + if timeout and cause is not None: + raise TimeoutError(f"server did not initialize within {timeout:g}s") from e + raise self.client = client self.connected = True - return True async def call_tool( self, tool_name: str, arguments: dict[str, Any] = None @@ -221,17 +238,27 @@ class BrowserClient: current_connection: str | None env_pass: bool env_vars: dict[str, str] | None - - def __init__(self, env_pass: bool = False, env_vars: dict[str, str] | None = None): + timeout: float | None + + def __init__( + self, + env_pass: bool = False, + env_vars: dict[str, str] | None = None, + timeout: float | None = DEFAULT_CONNECT_TIMEOUT, + ): self.connections: dict[str, BrowserConnection] = {} self.current_connection: str | None = None self.env_pass = env_pass self.env_vars = env_vars + self.timeout = timeout + + async def add_connection(self, name: str, connection_string: str) -> None: + """Add a new MCP connection using FastMCP Client connection string. - async def add_connection(self, name: str, connection_string: str) -> bool: - """Add a new MCP connection using FastMCP Client connection string.""" + Raises ValueError if the name is taken, or the connection error if connecting fails. + """ if name in self.connections: - return False + raise ValueError(f"connection name {name!r} already exists") if is_connection_string_url(connection_string): connection_type = "http" @@ -239,15 +266,11 @@ async def add_connection(self, name: str, connection_string: str) -> bool: connection_type = "command" connection = BrowserConnection(name, connection_type, connection_string) - success = await connection.connect(env_pass=self.env_pass, env_vars=self.env_vars) + await connection.connect(env_pass=self.env_pass, env_vars=self.env_vars, timeout=self.timeout) - if success: - self.connections[name] = connection - if not self.current_connection: - self.current_connection = name - return True - else: - return False + self.connections[name] = connection + if not self.current_connection: + self.current_connection = name async def switch_connection(self, name: str) -> bool: """Switch to a different connection.""" diff --git a/magg/mbro/command.py b/magg/mbro/command.py index f483142..ca4693b 100644 --- a/magg/mbro/command.py +++ b/magg/mbro/command.py @@ -1,5 +1,6 @@ """Command handlers for mbro CLI.""" +import difflib import json from typing import TYPE_CHECKING @@ -62,20 +63,23 @@ async def connect(self, args: list): if connection_string.startswith('"') and connection_string.endswith('"'): connection_string = connection_string[1:-1] - success = await self.browser.add_connection(name, connection_string) - if success: - conn = self.browser.connections[name] - tools = await conn.get_tools() - resources = await conn.get_resources() - prompts = await conn.get_prompts() - if not self.cli.quiet: - self.formatter.format_success( - f"Connected to '{name}' (Tools: {len(tools)}, Resources: {len(resources)}, Prompts: {len(prompts)})" - ) + try: + await self.browser.add_connection(name, connection_string) + except Exception as e: + error_args = (e,) if self.cli.verbose else () + self.formatter.format_error(f"Failed to connect to {name!r}: {e}", *error_args) + return - await self.cli.refresh_completer_cache() - else: - self.formatter.format_error(f"Failed to connect to {name!r}") + conn = self.browser.connections[name] + tools = await conn.get_tools() + resources = await conn.get_resources() + prompts = await conn.get_prompts() + if not self.cli.quiet: + self.formatter.format_success( + f"Connected to '{name}' (Tools: {len(tools)}, Resources: {len(resources)}, Prompts: {len(prompts)})" + ) + + await self.cli.refresh_completer_cache() async def switch(self, args: list): """Switch to a different connection.""" @@ -101,15 +105,21 @@ async def disconnect(self, args: list): if not success: self.formatter.format_error(f"Connection {name!r} not found") - async def status(self): + async def status(self, args: list): """Get status of the current connection. This includes counts of tools, resources, and prompts. """ conn = self.browser.get_current_connection() + if not conn: + self.formatter.format_error("No active connection.") + return + tools = await conn.get_tools() resources = await conn.get_resources() prompts = await conn.get_prompts() - self.formatter.format_json({"tools": len(tools), "resources": len(resources), "prompts": len(prompts)}) + self.formatter.format_json( + {"connection": conn.name, "tools": len(tools), "resources": len(resources), "prompts": len(prompts)} + ) async def tools(self, args: list): """List available tools.""" @@ -122,7 +132,11 @@ async def tools(self, args: list): tools = await conn.get_tools() if filter_term: - tools = [t for t in tools if filter_term in t["name"].lower() or filter_term in t["description"].lower()] + tools = [ + t + for t in tools + if filter_term in t["name"].lower() or filter_term in (t.get("description") or "").lower() + ] if not tools: self.formatter.format_info( @@ -169,7 +183,9 @@ async def prompts(self, args: list): prompts = await conn.get_prompts() if filter_term: prompts = [ - p for p in prompts if filter_term in p["name"].lower() or filter_term in p["description"].lower() + p + for p in prompts + if filter_term in p["name"].lower() or filter_term in (p.get("description") or "").lower() ] if not prompts: @@ -201,44 +217,16 @@ async def call(self, args: list): return tool_name = args[0] - arguments = {} - - if len(args) > 1: - args_str = " ".join(args[1:]) - - if args_str.strip().startswith("{"): - try: - arguments = json.loads(args_str) - except json.JSONDecodeError as e: - self.formatter.format_error(f"Invalid JSON arguments: {e}") - if not self.formatter.json_only: - self.formatter.format_info( - "\nJSON formatting tips:\n" - ' - Use double quotes for strings: {"key": "value"}\n' - ' - Numbers don\'t need quotes: {"count": 42}\n' - ' - Booleans: {"enabled": true}\n' - " - Don't quote the entire JSON object\n" - ' - Example: call tool {"param": "value"}' - ) - return - else: - has_positional = False - for arg in args[1:]: - if "=" not in arg and not arg.startswith("{"): - has_positional = True - break - - if has_positional: - self.formatter.format_error("Positional arguments are not supported. Use key=value syntax.") - self.formatter.format_info(f"Example: call {tool_name} a=1 b=2") - return - - arguments = self.cli.parse_shell_args(args[1:]) tools = await conn.get_tools() tool = next((t for t in tools if t["name"] == tool_name), None) + schema = tool.get("inputSchema", {}) if tool else {} + + arguments = self._parse_arguments(f"call {tool_name}", args[1:], schema) + if arguments is None: + return + if tool: - schema = tool.get("inputSchema", {}) required = schema.get("required", []) if required: missing = [param for param in required if param not in arguments] @@ -271,6 +259,8 @@ async def call(self, args: list): except Exception as e: error_args = (e,) if self.cli.verbose else () self.formatter.format_error(str(e), *error_args) + if not tool and (matches := difflib.get_close_matches(tool_name, [t["name"] for t in tools])): + self.formatter.format_info(f"Did you mean: {', '.join(matches)}?") async def resource(self, args: list): """Get a resource.""" @@ -295,12 +285,13 @@ async def resource(self, args: list): self.formatter.format_resource(result[0]) except Exception as e: - self.formatter.format_error(f"Error getting resource: {e}", e) + error_args = (e,) if self.cli.verbose else () + self.formatter.format_error(f"Error getting resource: {e}", *error_args) async def prompt(self, args: list): """Get a prompt.""" if not args: - self.formatter.format_error("Usage: prompt [json_arguments]") + self.formatter.format_error("Usage: prompt [arguments]") return conn = self.browser.get_current_connection() @@ -309,14 +300,11 @@ async def prompt(self, args: list): return name = args[0] - arguments = {} - if len(args) > 1: - try: - arguments = json.loads(" ".join(args[1:])) - except json.JSONDecodeError as e: - self.formatter.format_error(f"Invalid JSON arguments: {e}") - return + # Prompt arguments are strings in MCP, so key=value values are passed through as-is + arguments = self._parse_arguments(f"prompt {name}", args[1:], strings=True) + if arguments is None: + return try: result = await conn.get_prompt(name, arguments) @@ -324,7 +312,8 @@ async def prompt(self, args: list): self.formatter.format_prompt_result(result) except Exception as e: - self.formatter.format_error(f"Error getting prompt: {e}", e) + error_args = (e,) if self.cli.verbose else () + self.formatter.format_error(f"Error getting prompt: {e}", *error_args) async def search(self, args: list): """Search tools, resources, and prompts.""" @@ -345,8 +334,8 @@ async def search(self, args: list): def matches_item(item, search_term): """Check if item matches search term with word splitting.""" - name = item.get("name", "").lower() - desc = item.get("description", "").lower() + name = (item.get("name") or "").lower() + desc = (item.get("description") or "").lower() # Direct substring match if search_term in name or search_term in desc: @@ -417,6 +406,40 @@ async def script(self, args: list): """Handle script commands.""" await self.script_manager.handle_script_command(args) + def _parse_arguments( + self, usage: str, args: list, schema: dict | None = None, *, strings: bool = False + ) -> dict | None: + """Parse call/prompt arguments given as a JSON object or key=value pairs. + + Returns None if the arguments are invalid (the error has already been reported). + """ + if not args: + return {} + + args_str = " ".join(args) + + if args_str.strip().startswith("{"): + try: + return json.loads(args_str) + except json.JSONDecodeError as e: + self.formatter.format_error(f"Invalid JSON arguments: {e}") + if not self.formatter.json_only: + self.formatter.format_info( + "\nJSON formatting tips:\n" + ' - Use double quotes for strings: {"key": "value"}\n' + ' - Numbers don\'t need quotes: {"count": 42}\n' + ' - Booleans: {"enabled": true}\n' + f' - Example: {usage} {{"param": "value"}}' + ) + return None + + if any("=" not in arg for arg in args): + self.formatter.format_error("Positional arguments are not supported. Use key=value syntax.") + self.formatter.format_info(f"Example: {usage} a=1 b=2") + return None + + return self.cli.parse_shell_args(args, schema, strings=strings) + async def _handle_proxy_query_result(self, tool_name: str, result: list) -> bool: """ Handle the ProxyMCP tool's [list, info] actions on [tool, resource, prompt]. diff --git a/magg/mbro/formatter.py b/magg/mbro/formatter.py index 3c6afb8..99c33e8 100644 --- a/magg/mbro/formatter.py +++ b/magg/mbro/formatter.py @@ -20,6 +20,8 @@ def __init__(self, json_only: bool = False, use_rich: bool | None = True, indent self.console_stdout = Console(file=sys.stdout) if self.use_rich else None self.json_only = json_only self.indent = indent if indent > 0 else None + # Lets callers tell whether a command reported a failure + self.error_count = 0 def format_json(self, data: Any) -> None: """Format and print JSON data.""" @@ -32,6 +34,8 @@ def format_json(self, data: Any) -> None: def format_error(self, message: str, exception: Exception | None = None) -> None: """Format and print an error message.""" + self.error_count += 1 + if self.json_only: error_data = {"error": message} if exception: @@ -455,12 +459,16 @@ def format_search_results(self, term: str, tools: list, resources: list, prompts results = { "query": term, "total_matches": total_matches, - "tools": [{"name": t["name"], "description": t["description"]} for t in tools], + "tools": [{"name": t["name"], "description": t.get("description")} for t in tools], "resources": [ - {"name": r["name"], "uri": r.get("uri", r.get("uriTemplate", "")), "description": r["description"]} + { + "name": r["name"], + "uri": r.get("uri", r.get("uriTemplate", "")), + "description": r.get("description"), + } for r in resources ], - "prompts": [{"name": p["name"], "description": p["description"]} for p in prompts], + "prompts": [{"name": p["name"], "description": p.get("description")} for p in prompts], } self.format_json(results) return @@ -525,7 +533,7 @@ def format_help(self, *, enhanced: bool = False) -> None: {"command": "connections, conns", "description": "List all connections"}, {"command": "switch ", "description": "Switch to a different connection"}, {"command": "disconnect ", "description": "Disconnect from a server"}, - {"command": "status", "description": "Show current status"}, + {"command": "status", "description": "Show current connection status"}, ], "server_exploration": [ {"command": "tools [filter]", "description": "List available tools"}, @@ -539,6 +547,14 @@ def format_help(self, *, enhanced: bool = False) -> None: {"command": "resource ", "description": "Get a resource"}, {"command": "prompt [args]", "description": "Get a prompt"}, ], + "other": [ + { + "command": "script ...", + "description": "Run or manage .mbro scripts", + }, + {"command": "help", "description": "Show this help"}, + {"command": "quit, exit", "description": "Exit mbro"}, + ], } } self.format_json(help_data) @@ -552,7 +568,7 @@ def format_help(self, *, enhanced: bool = False) -> None: connections, conns - List all connections switch - Switch to a different connection disconnect - Disconnect from a server - refresh - Refresh capabilities for current connection + status - Show current connection status Server Exploration: tools [filter] - List available tools @@ -566,6 +582,11 @@ def format_help(self, *, enhanced: bool = False) -> None: resource - Get a resource prompt [args] - Get a prompt +Other: + script ... - Run or manage .mbro scripts + help - Show this help + quit, exit - Exit mbro + Examples: connect magg "http://localhost:8080" connect calc "npx @wrtnlabs/calculator-mcp" @@ -573,6 +594,7 @@ def format_help(self, *, enhanced: bool = False) -> None: call magg_status call magg_search_tools {"query": "calculator", "limit": 3} call add a=5 b=3 + prompt greet name=Bob search calculator info tool magg_status diff --git a/magg/mbro/parser.py b/magg/mbro/parser.py index 688b4ec..a8d33ef 100644 --- a/magg/mbro/parser.py +++ b/magg/mbro/parser.py @@ -1,121 +1,10 @@ """Command line and argument parsing for mbro.""" -import json import re import shlex -from typing import Any - -class JsonArgParser: - """Parse and validate JSON and shell-style arguments.""" - - @staticmethod - def parse_command(text: str) -> tuple[str, list[str]]: - """Parse a command string into command and arguments.""" - parts = [] - current = [] - in_json = False - brace_count = 0 - - for char in text: - if char == "{" and not in_json: - if current: - parts.extend("".join(current).split()) - current = [] - in_json = True - brace_count = 1 - current.append(char) - elif in_json: - current.append(char) - if char == "{": - brace_count += 1 - elif char == "}": - brace_count -= 1 - if brace_count == 0: - parts.append("".join(current)) - current = [] - in_json = False - else: - current.append(char) - - if current: - if in_json: - parts.append("".join(current)) - else: - parts.extend("".join(current).split()) - - if not parts: - return "", [] - - command = parts[0].lower() - args = parts[1:] - - args = JsonArgParser._process_arguments(command, args) - - return command, args - - @staticmethod - def _process_arguments(command: str, args: list[str]) -> list[str]: - """Process and enhance arguments based on command context.""" - if not args: - return args - - if command == "call" and len(args) > 1: - tool_name = args[0] - remaining = " ".join(args[1:]) - - if not remaining.strip().startswith("{"): - converted = JsonArgParser._convert_to_json(remaining) - if converted: - return [tool_name, converted] - - return args - - @staticmethod - def _convert_to_json(arg_string: str) -> str | None: - """Try to convert various argument formats to JSON.""" - if arg_string.strip().startswith("{"): - return arg_string - - if "=" in arg_string: - args_dict = {} - pairs = re.split(r"\s+", arg_string) - - for pair in pairs: - if "=" in pair: - key, value = pair.split("=", 1) - args_dict[key] = JsonArgParser._infer_type(value) - - if args_dict: - return json.dumps(args_dict) - - return None - - @staticmethod - def _infer_type(value: str) -> Any: - """Infer the type of a string value.""" - value = value.strip() - - if (value.startswith('"') and value.endswith('"')) or (value.startswith("'") and value.endswith("'")): - return value[1:-1] - - if value.lower() in ("true", "false"): - return value.lower() == "true" - - if value.isdigit() or (value.startswith("-") and value[1:].isdigit()): - return int(value) - - try: - if "." in value: - return float(value) - except ValueError: - pass - - if "," in value: - items = [JsonArgParser._infer_type(item.strip()) for item in value.split(",")] - return items - - return value +# call/prompt followed by a JSON object: the object is kept as raw text rather than shell-split +JSON_ARGS_PATTERN = re.compile(r"(call|prompt)\s+(\S+)\s+(\{.*)", re.DOTALL | re.IGNORECASE) class CommandParser: @@ -137,6 +26,10 @@ def parse_command_line(line: str) -> list[str]: if not cleaned: return [] + # shlex would strip the double quotes inside a JSON object + if match := JSON_ARGS_PATTERN.fullmatch(cleaned.strip()): + return list(match.groups()) + try: return shlex.split(cleaned) except ValueError: @@ -195,16 +88,54 @@ def split_commands(text: str) -> list[str]: i += 1 commands = [] + pending = "" for line in merged_lines: line = CommandParser._remove_comments(line) + if pending: + line = pending + "\n" + line + pending = "" + if not line.strip(): continue + # A JSON object spanning lines: keep reading until its braces balance + if CommandParser._brace_depth(line) > 0: + pending = line + continue + parts = CommandParser._split_by_semicolon(line) commands.extend(parts) + if pending: + commands.extend(CommandParser._split_by_semicolon(pending)) + return [cmd.strip() for cmd in commands if cmd.strip()] + @staticmethod + def _brace_depth(text: str) -> int: + """Count braces opened but not closed, ignoring those in quotes.""" + depth = 0 + in_single_quote = False + in_double_quote = False + escaped = False + + for char in text: + if escaped: + escaped = False + elif char == "\\": + escaped = True + elif char == '"' and not in_single_quote: + in_double_quote = not in_double_quote + elif char == "'" and not in_double_quote: + in_single_quote = not in_single_quote + elif not in_single_quote and not in_double_quote: + if char == "{": + depth += 1 + elif char == "}": + depth -= 1 + + return depth + @staticmethod def _split_by_semicolon(text: str) -> list[str]: """Split text by semicolons that aren't in quotes.""" diff --git a/magg/mbro/scripts.py b/magg/mbro/scripts.py index c66d076..376aff2 100644 --- a/magg/mbro/scripts.py +++ b/magg/mbro/scripts.py @@ -102,7 +102,7 @@ async def run_script(self, args: List[str]): script_path = self.find_script(script_ref) if not script_path: self.formatter.format_error(f"Script not found: {script_ref!r}") - exit(1) + return try: script_content = script_path.read_text() @@ -110,13 +110,7 @@ async def run_script(self, args: List[str]): self.formatter.format_error(f"Failed to read script: {e}") return - commands = CommandParser.split_commands(script_content) - - for command in commands: - if command.strip(): - if self.cli.verbose: - self.formatter.format_info(f"> {command}") - await self.cli.handle_command(command) + await self.cli.run_commands(CommandParser.split_commands(script_content)) async def list_scripts(self, args: List[str]): """List available scripts with optional filter.""" @@ -203,7 +197,7 @@ async def dump_script(self, args: List[str]): script_path = self.find_script(script_ref) if not script_path: self.formatter.format_error(f"Script not found: {script_ref!r}") - exit(1) + return try: script_content = script_path.read_text() @@ -219,12 +213,7 @@ async def dump_script(self, args: List[str]): ) if edited is not None: - commands = CommandParser.split_commands(edited) - for command in commands: - if command.strip(): - if self.cli.verbose: - self.formatter.format_info(f"> {command}") - await self.cli.handle_command(command) + await self.cli.run_commands(CommandParser.split_commands(edited)) else: self.formatter.format_info(f"Script: {script_path.name}") self.formatter.print(script_content) diff --git a/magg/proxy/mixin.py b/magg/proxy/mixin.py index 792a907..f5c9a84 100644 --- a/magg/proxy/mixin.py +++ b/magg/proxy/mixin.py @@ -84,7 +84,7 @@ async def _proxy_tool( str | None, Field( description="Name or URI of the specific tool/resource/prompt (with FastMCP prefixing).\n" - "Not allowed for 'list' and 'info' actions.", + "Required for 'info' and 'call' actions; not allowed for 'list'.", # validation_alias=AliasChoices("name", "uri"), ), ] = None, diff --git a/magg/proxy/server.py b/magg/proxy/server.py index 5cb189c..60a1700 100644 --- a/magg/proxy/server.py +++ b/magg/proxy/server.py @@ -7,7 +7,10 @@ from fastmcp import Client, FastMCP from fastmcp.client import FastMCPTransport from fastmcp.client.messages import MessageHandler -from fastmcp.tools import FunctionTool +from fastmcp.prompts import Prompt +from fastmcp.resources import Resource, ResourceTemplate +from fastmcp.tools import FunctionTool, Tool +from fastmcp.utilities.versions import VersionSpec from ..messaging import MessageRouter, ServerMessageCoordinator from .mixin import ProxyMCP @@ -67,6 +70,34 @@ def _proxy_backend_client(self) -> Client: transport = FastMCPTransport(self) return Client(transport) + # FastMCP resolves a component by querying every provider in parallel, so one slow or hung + # (unprefixed) backend would delay every lookup. Resolve our own components first; local + # components take precedence over providers anyway. Auth-guarded ones go the normal route. + + async def _get_tool(self, name: str, version: VersionSpec | None = None) -> Tool | None: + tool = await self.local_provider.get_tool(name, version) + if tool is not None and tool.auth is None: + return tool + return await super()._get_tool(name, version) + + async def _get_resource(self, uri: str, version: VersionSpec | None = None) -> Resource | None: + resource = await self.local_provider.get_resource(uri, version) + if resource is not None and resource.auth is None: + return resource + return await super()._get_resource(uri, version) + + async def _get_resource_template(self, uri: str, version: VersionSpec | None = None) -> ResourceTemplate | None: + template = await self.local_provider.get_resource_template(uri, version) + if template is not None and template.auth is None: + return template + return await super()._get_resource_template(uri, version) + + async def _get_prompt(self, name: str, version: VersionSpec | None = None) -> Prompt | None: + prompt = await self.local_provider.get_prompt(name, version) + if prompt is not None and prompt.auth is None: + return prompt + return await super()._get_prompt(name, version) + def _register_proxy_tool(self): tool = FunctionTool.from_function( self._proxy_tool, diff --git a/magg/reload.py b/magg/reload.py index ffb5601..22a5385 100644 --- a/magg/reload.py +++ b/magg/reload.py @@ -3,19 +3,16 @@ from __future__ import annotations import asyncio -import json import logging +import os from dataclasses import dataclass, field from pathlib import Path -from typing import TYPE_CHECKING, Callable, Coroutine, TypeAlias +from typing import Callable, Coroutine, TypeAlias from watchdog.events import FileSystemEventHandler from watchdog.observers import Observer -from .settings import MaggConfig, ServerConfig - -if TYPE_CHECKING: - from .settings import ConfigManager +from .settings import ConfigFileError, ConfigManager, MaggConfig, ServerConfig logger = logging.getLogger(__name__) @@ -78,25 +75,41 @@ def __init__(self, config_path: Path, reload_event: asyncio.Event): # Not in an async context, get the event loop self._loop = asyncio.get_event_loop() - def on_modified(self, event): - """Handle file modification events.""" - if not event.is_directory and Path(event.src_path) == self.config_path: + def _notify(self, path: str | bytes) -> None: + if Path(os.fsdecode(path)) == self.config_path: # Schedule the reload event in the asyncio loop self._loop.call_soon_threadsafe(self.reload_event.set) + def on_modified(self, event): + """Handle file modification events.""" + if not event.is_directory: + self._notify(event.src_path) + + def on_created(self, event): + """Handle file creation, e.g. an editor writing a new copy of the file.""" + if not event.is_directory: + self._notify(event.src_path) + + def on_moved(self, event): + """Handle a file renamed over the config, as editors and atomic writes do.""" + if not event.is_directory: + self._notify(event.dest_path) + class ConfigReloader: """Manages configuration reloading with file watching and diff detection.""" - def __init__(self, config_path: Path, reload_callback: ReloadCallback): + def __init__(self, config_path: Path, reload_callback: ReloadCallback, config_manager: ConfigManager | None = None): """Initialize the config reloader. Args: config_path: Path to the configuration file reload_callback: Async callback to handle config changes + config_manager: Manager used to read the config file (default: one for config_path) """ self.config_path = config_path self.reload_callback = reload_callback + self._config_manager = config_manager or ConfigManager(config_path) self._last_mtime: float | None = None self._last_config: MaggConfig | None = None self._watch_task: asyncio.Task | None = None @@ -107,11 +120,12 @@ def __init__(self, config_path: Path, reload_callback: ReloadCallback): self._observer: Observer | None = None self._watchdog_handler: WatchdogHandler | None = None - async def start_watching(self, poll_interval: float = 1.0) -> None: + async def start_watching(self, poll_interval: float = 1.0, use_watchdog: bool | None = None) -> None: """Start watching the config file for changes. Args: poll_interval: How often to check for changes (seconds) - only used for polling mode + use_watchdog: Use file system notifications (True), polling (False), or auto-detect (None) """ if self._watch_task and not self._watch_task.done(): logger.warning("Config watcher already running") @@ -120,17 +134,20 @@ async def start_watching(self, poll_interval: float = 1.0) -> None: self._shutdown_event.clear() self._reload_event.clear() - # Try to start watchdog observer - try: - self._observer = Observer() - self._watchdog_handler = WatchdogHandler(self.config_path, self._reload_event) - self._observer.schedule(self._watchdog_handler, str(self.config_path.parent), recursive=False) - self._observer.start() - logger.debug("Started config file watcher using file system notifications (watchdog)") - except Exception as e: - logger.warning("Failed to start watchdog observer: %s. Falling back to polling mode.", e) - self._observer = None - self._watchdog_handler = None + # Try to start watchdog observer, unless polling was requested + if use_watchdog is not False: + try: + self._observer = Observer() + self._watchdog_handler = WatchdogHandler(self.config_path, self._reload_event) + self._observer.schedule(self._watchdog_handler, str(self.config_path.parent), recursive=False) + self._observer.start() + logger.debug("Started config file watcher using file system notifications (watchdog)") + except Exception as e: + logger.warning("Failed to start watchdog observer: %s. Falling back to polling mode.", e) + self._observer = None + self._watchdog_handler = None + + if not self._observer: logger.debug("Using polling mode (interval: %.1fs)", poll_interval) # Start the main watch loop @@ -295,24 +312,9 @@ async def reload_config(self) -> ConfigChange | None: def _load_config(self) -> MaggConfig | None: """Load configuration from disk.""" try: - with self.config_path.open("r") as f: - data = json.load(f) - - servers = {} - for name, server_data in data.get("servers", {}).items(): - try: - server_data["name"] = name - servers[name] = ServerConfig.model_validate(server_data) - except Exception as e: - logger.error("Error loading server '%s': %s", name, e) - continue - - config = MaggConfig() - config.servers = servers - return config - - except Exception as e: - logger.error("Error loading config file: %s", e) + return self._config_manager.read_config() + except ConfigFileError as e: + logger.error("%s", e) return None def _detect_changes(self, old_config: MaggConfig, new_config: MaggConfig) -> ConfigChange: @@ -367,7 +369,7 @@ def _validate_config(self, config: MaggConfig) -> bool: """Validate that the configuration is valid.""" try: for name, server in config.servers.items(): - if not server.command and not server.uri: + if server.enabled and not server.command and not server.uri: logger.error("Server '%s' has neither command nor uri", name) return False @@ -410,9 +412,13 @@ async def setup(self, reload_callback: ReloadCallback) -> None: if config.auto_reload and not self._config_reloader: if self.config_manager.config_path.exists(): self._config_reloader = ConfigReloader( - config_path=self.config_manager.config_path, reload_callback=reload_callback + config_path=self.config_manager.config_path, + reload_callback=reload_callback, + config_manager=self.config_manager, + ) + await self._config_reloader.start_watching( + poll_interval=config.reload_poll_interval, use_watchdog=config.reload_use_watchdog ) - await self._config_reloader.start_watching(poll_interval=config.reload_poll_interval) async def stop(self) -> None: """Stop config file watching.""" @@ -436,7 +442,9 @@ async def reload(self) -> bool: return False reloader = ConfigReloader( - config_path=self.config_manager.config_path, reload_callback=self._reload_callback + config_path=self.config_manager.config_path, + reload_callback=self._reload_callback, + config_manager=self.config_manager, ) change = await reloader.reload_config() return change is not None diff --git a/magg/server/defaults.py b/magg/server/defaults.py index e880edd..162facd 100644 --- a/magg/server/defaults.py +++ b/magg/server/defaults.py @@ -124,7 +124,7 @@ Can be provided as a dict or JSON string (automatically parsed). path (string | null) (optional) Name or URI of the specific tool/resource/prompt (with FastMCP prefixing). - Not allowed for 'list' and 'info' actions. + Required for 'info' and 'call' actions; not allowed for 'list'. limit (integer | null) (optional) Maximum number of items to return (for 'list' action only). Default: 100, Max: 1000 offset (integer | null) (optional) diff --git a/magg/server/manager.py b/magg/server/manager.py index ccfb7cf..56b89e9 100644 --- a/magg/server/manager.py +++ b/magg/server/manager.py @@ -5,10 +5,14 @@ import os from dataclasses import dataclass from functools import cached_property +from pathlib import Path from typing import Annotated from fastmcp import Client, FastMCP +from fastmcp.client.transports import StdioTransport from fastmcp.server import create_proxy +from fastmcp.server.providers import FastMCPProvider, Provider +from fastmcp.server.transforms import Namespace from pydantic import Field from ..auth import BearerAuthManager @@ -16,7 +20,6 @@ from ..proxy.server import BackendMessageHandler, ProxyFastMCP from ..reload import ConfigChange, ServerChange from ..settings import ConfigManager, MaggConfig, ServerConfig -from ..util.stdio_patch import patch_stdio_transport_stderr from ..util.transport import get_transport_for_command, get_transport_for_uri from .defaults import MAGG_INSTRUCTIONS from .response import MaggResponse @@ -30,6 +33,7 @@ class MountedServer: proxy: FastMCP client: Client + provider: Provider | None = None class ServerManager: @@ -38,7 +42,9 @@ class ServerManager: config_manager: ConfigManager mcp: ProxyFastMCP mounted_servers: dict[str, MountedServer] + mount_errors: dict[str, str] subprocess_env: dict | None = None + auth_error: str | None = None def __init__(self, config_manager: ConfigManager, *, env: dict | None = None): self.config_manager = config_manager @@ -52,8 +58,12 @@ def __init__(self, config_manager: ConfigManager, *, env: dict | None = None): try: auth_provider = auth_manager.provider logger.debug("Authentication enabled (bearer)") - except RuntimeError as e: - logger.warning("Authentication disabled: %s", e) + except RuntimeError: + bearer = auth_config.bearer + key_source = "MAGG_PRIVATE_KEY" if bearer.private_key_env else str(bearer.private_key_path) + self.auth_error = f"Private key from {key_source} could not be loaded (invalid or passphrase-protected)" + # Only fatal when serving HTTP (see require_http_auth); stdio does not use auth + logger.warning("%s; bearer authentication is unavailable", self.auth_error) self.mcp = ProxyFastMCP( name=self.self_prefix, @@ -61,6 +71,12 @@ def __init__(self, config_manager: ConfigManager, *, env: dict | None = None): auth=auth_provider, ) self.mounted_servers = {} + self.mount_errors = {} + + def require_http_auth(self) -> None: + """Refuse to serve HTTP when a configured private key failed to load (fail closed).""" + if self.auth_error: + raise RuntimeError(f"{self.auth_error}; refusing to serve HTTP without authentication") @property def config(self) -> MaggConfig: @@ -94,6 +110,7 @@ async def mount_server(self, server: ServerConfig) -> bool: return False try: + config = self.config message_handler = BackendMessageHandler(server_id=server.name, coordinator=self.mcp.message_coordinator) if server.command: @@ -102,12 +119,13 @@ async def mount_server(self, server: ServerConfig) -> bool: if server.env or self.subprocess_env: env = {} - if server.env: - env.update(server.env) - + # Per-server env takes precedence over the inherited/provided environment if self.subprocess_env: env.update(self.subprocess_env) + if server.env: + env.update(server.env) + transport = get_transport_for_command( command=server.command, args=server.args or [], @@ -116,91 +134,57 @@ async def mount_server(self, server: ServerConfig) -> bool: transport_config=server.transport, ) - if not self.config.stderr_show: - transport = patch_stdio_transport_stderr(transport) - - client = Client(transport, message_handler=message_handler) + if not config.stderr_show and isinstance(transport, StdioTransport): + transport.log_file = Path(os.devnull) elif server.uri: transport = get_transport_for_uri(uri=server.uri, transport_config=server.transport) - client = Client(transport, message_handler=message_handler) - else: logger.error("No command or URI specified for %s", server.name) + self.mount_errors[server.name] = "No command or URI specified" return False + # Bound connect/initialize so a hung backend can't block list operations indefinitely + init_timeout = config.backend_init_timeout if config.backend_init_timeout > 0 else None + client = Client(transport, message_handler=message_handler, init_timeout=init_timeout) + + # Build the provider that FastMCP.mount() would add, so we can remove it on unmount proxy_server = create_proxy(client, name=server.name) - self.mcp.mount(server=proxy_server, namespace=server.prefix) + provider: Provider = FastMCPProvider(proxy_server) + if server.prefix: + provider = provider.wrap_transform(Namespace(server.prefix)) + self.mcp.add_provider(provider) - self.mounted_servers[server.name] = MountedServer(proxy=proxy_server, client=client) + self.mounted_servers[server.name] = MountedServer(proxy=proxy_server, client=client, provider=provider) + self.mount_errors.pop(server.name, None) logger.debug("Mounted server %s with prefix %r", server.name, server.prefix) return True except Exception as e: logger.error("Failed to mount server %s: %s", server.name, e) + self.mount_errors[server.name] = str(e) return False - def _unmount_from_fastmcp(self, server_name: str) -> bool: - """Remove a mounted server from FastMCP's internal structures. - - This is a workaround until FastMCP provides an official unmount method. - Returns True if server was found and removed. - """ - # We need to find and remove the MountedServer object from all managers - found = False - - # Check tool manager - if hasattr(self.mcp, "_tool_manager") and hasattr(self.mcp._tool_manager, "_mounted_servers"): - mounted_servers = self.mcp._tool_manager._mounted_servers - for i, ms in enumerate(mounted_servers): - if hasattr(ms, "server") and hasattr(ms.server, "name") and ms.server.name == server_name: - mounted_servers.pop(i) - found = True - logger.debug("Removed server %s from tool manager", server_name) - break - - # Check resource manager - if hasattr(self.mcp, "_resource_manager") and hasattr(self.mcp._resource_manager, "_mounted_servers"): - mounted_servers = self.mcp._resource_manager._mounted_servers - for i, ms in enumerate(mounted_servers): - if hasattr(ms, "server") and hasattr(ms.server, "name") and ms.server.name == server_name: - mounted_servers.pop(i) - found = True - logger.debug("Removed server %s from resource manager", server_name) - break - - # Check prompt manager - if hasattr(self.mcp, "_prompt_manager") and hasattr(self.mcp._prompt_manager, "_mounted_servers"): - mounted_servers = self.mcp._prompt_manager._mounted_servers - for i, ms in enumerate(mounted_servers): - if hasattr(ms, "server") and hasattr(ms.server, "name") and ms.server.name == server_name: - mounted_servers.pop(i) - found = True - logger.debug("Removed server %s from prompt manager", server_name) - break - - return found - async def unmount_server(self, name: str) -> bool: """Unmount a server.""" if name in self.mounted_servers: - unmounted = self._unmount_from_fastmcp(name) - if unmounted: - logger.debug("Unmounted server %s from FastMCP", name) + server_info = self.mounted_servers.pop(name) + + if server_info.provider is not None and server_info.provider in self.mcp.providers: + self.mcp.providers.remove(server_info.provider) + logger.debug("Removed provider for server %s from FastMCP", name) else: - logger.debug("Server %s was not found in FastMCP's mounted servers", name) + logger.debug("Server %s had no provider registered with FastMCP", name) - server_info = self.mounted_servers.get(name) - if server_info and server_info.client: + if server_info.client: try: await server_info.client.close() logger.debug("Closed client for server %s", name) except Exception as e: logger.warning("Error closing client for server %s: %s", name, e) - del self.mounted_servers[name] logger.debug("Unmounted server %s", name) return True @@ -429,12 +413,14 @@ async def run_stdio(self): async def run_http(self, host: str = "localhost", port: int = 8000, log_level: str | None = None): """Run Magg in HTTP mode.""" + self.server_manager.require_http_auth() log_level = log_level or os.getenv("FASTMCP_LOG_LEVEL", "CRITICAL").upper() or "CRITICAL" await self.setup() await self.mcp.run_http_async(host=host, port=port, log_level=log_level, show_banner=False) async def run_hybrid(self, host: str = "localhost", port: int = 8000, log_level: str | None = None): """Run Magg in hybrid mode - both stdio and HTTP simultaneously.""" + self.server_manager.require_http_auth() log_level = log_level or os.getenv("FASTMCP_LOG_LEVEL", "CRITICAL").upper() or "CRITICAL" await self.setup() @@ -491,6 +477,10 @@ async def load_kit( """Load a kit and its servers into the configuration.""" try: config = self.config + + if config.read_only: + return MaggResponse.error("Cannot load kit in read-only mode") + success, message = self.kit_manager.load_kit_to_config(name, config) if success: @@ -516,6 +506,9 @@ async def unload_kit( try: config = self.config + if config.read_only: + return MaggResponse.error("Cannot unload kit in read-only mode") + servers_before = set(config.servers.keys()) success, message = self.kit_manager.unload_kit_from_config(name, config) diff --git a/magg/server/runner.py b/magg/server/runner.py index 72d52db..78a9594 100644 --- a/magg/server/runner.py +++ b/magg/server/runner.py @@ -19,12 +19,19 @@ class MaggRunner: """Manages Magg server lifecycle with proper signal handling.""" + # How long to wait for the server task to finish after cancelling it. The stdio transport + # reads stdin in a worker thread that can't be interrupted, so its task may never finish. + SHUTDOWN_TIMEOUT: float = 1.0 + _server: MaggServer _shutdown_event: asyncio.Event _reload_event: asyncio.Event - _original_sigint: Callable | None - _original_sigterm: Callable | None - _original_sighup: Callable | None + _original_sigint: Callable | int | None + _original_sigterm: Callable | int | None + _original_sighup: Callable | int | None + _signal_loop: asyncio.AbstractEventLoop | None = None + _shutdown_signal: int | None = None + _abandoned_task: bool = False _hook_signals: bool _hooked_signals: bool = False @@ -63,18 +70,27 @@ async def __aexit__(self, exc_type, exc_value, traceback): await self._server.__aexit__(exc_type, exc_value, traceback) # noinspection PyUnusedLocal - def _handle_signal(self, signum, frame): - """Handle shutdown signals gracefully.""" + def _handle_signal(self, signum, frame=None): + """Handle shutdown signals gracefully; a second signal forces exit.""" signame = signal.Signals(signum).name + + if self._shutdown_event.is_set(): + logger.warning("Received %s again, forcing exit", signame) + self._exit_by_signal(signum) + return + logger.debug("Received signal %s, shutting down gracefully...", signame) + self._shutdown_signal = signum self._shutdown_event.set() - # Prevent multiple signal handlers from firing - signal.signal(signal.SIGINT, signal.SIG_IGN) - signal.signal(signal.SIGTERM, signal.SIG_IGN) + @staticmethod + def _exit_by_signal(signum): + """Terminate the process with the signal's default action.""" + signal.signal(signum, signal.SIG_DFL) + signal.raise_signal(signum) # noinspection PyUnusedLocal - def _handle_reload_signal(self, signum, frame): + def _handle_reload_signal(self, signum, frame=None): """Handle reload signal (SIGHUP).""" logger.debug("Received SIGHUP, triggering config reload...") self._reload_event.set() @@ -82,22 +98,43 @@ def _handle_reload_signal(self, signum, frame): def _setup_signal_handlers(self): """Setup signal handlers for graceful shutdown and reload.""" if self._hook_signals and self._hooked_signals is False: - self._original_sigint = signal.signal(signal.SIGINT, self._handle_signal) - self._original_sigterm = signal.signal(signal.SIGTERM, self._handle_signal) + handlers = {signal.SIGINT: self._handle_signal, signal.SIGTERM: self._handle_signal} # SIGHUP for config reload (Unix-like systems only) if hasattr(signal, "SIGHUP"): - self._original_sighup = signal.signal(signal.SIGHUP, self._handle_reload_signal) + handlers[signal.SIGHUP] = self._handle_reload_signal + + self._original_sigint = signal.getsignal(signal.SIGINT) + self._original_sigterm = signal.getsignal(signal.SIGTERM) + if hasattr(signal, "SIGHUP"): + self._original_sighup = signal.getsignal(signal.SIGHUP) + + loop = asyncio.get_running_loop() + try: + # Loop handlers run as loop callbacks, so they also wake an idle loop + for signum, handler in handlers.items(): + loop.add_signal_handler(signum, handler, signum) + self._signal_loop = loop + except (NotImplementedError, RuntimeError): + # No loop signal support (e.g. Windows): hand the signal to the loop thread-safely + for signum, handler in handlers.items(): + signal.signal(signum, lambda s, _f, h=handler: loop.call_soon_threadsafe(h, s)) self._hooked_signals = True def _restore_signal_handlers(self): """Restore original signal handlers.""" if self._hook_signals and self._hooked_signals: - if self._original_sigint: - signal.signal(signal.SIGINT, self._original_sigint) - if self._original_sigterm: - signal.signal(signal.SIGTERM, self._original_sigterm) - if hasattr(signal, "SIGHUP") and self._original_sighup: - signal.signal(signal.SIGHUP, self._original_sighup) + originals = {signal.SIGINT: self._original_sigint, signal.SIGTERM: self._original_sigterm} + if hasattr(signal, "SIGHUP"): + originals[signal.SIGHUP] = self._original_sighup + + for signum, original in originals.items(): + if self._signal_loop is not None: + self._signal_loop.remove_signal_handler(signum) + # SIG_DFL is 0, so check against None rather than truthiness + if original is not None: + signal.signal(signum, original) + + self._signal_loop = None self._hooked_signals = False @asynccontextmanager @@ -111,6 +148,12 @@ async def _server_context(self): finally: self._restore_signal_handlers() + # asyncio.run() would wait forever on an abandoned server task, so now that cleanup + # is done, finish the signal-requested shutdown with the signal's default action + if self._abandoned_task and self._shutdown_signal is not None: + logger.debug("Magg server stopped, exiting with abandoned server task") + self._exit_by_signal(self._shutdown_signal) + async def _serve(self, coro: Coroutine): server_task = asyncio.create_task(coro) shutdown_task = asyncio.create_task(self._shutdown_event.wait()) @@ -127,10 +170,12 @@ async def _serve(self, coro: Coroutine): for task in pending: task.cancel() - try: - await task - except asyncio.CancelledError: - pass + + if pending: + _, stuck = await asyncio.wait(pending, timeout=self.SHUTDOWN_TIMEOUT) + if stuck: + logger.debug("Server task did not stop within %ss, abandoning it", self.SHUTDOWN_TIMEOUT) + self._abandoned_task = True for task in done: if task is server_task and task.exception(): @@ -165,6 +210,8 @@ async def run_stdio(self): async def run_http(self, host: str = "localhost", port: int = 8000): """Run server in HTTP mode with proper signal handling.""" + self._server.server_manager.require_http_auth() + try: async with self._server_context() as server: logger.debug("Starting Magg HTTP server on %s:%s", host, port) @@ -175,6 +222,8 @@ async def run_http(self, host: str = "localhost", port: int = 8000): async def run_hybrid(self, host: str = "localhost", port: int = 8000): """Run server in hybrid mode (stdio + HTTP) with proper signal handling.""" + self._server.server_manager.require_http_auth() + try: async with self._server_context() as server: logger.debug("Starting Magg hybrid server (stdio + HTTP on %s:%s)", host, port) diff --git a/magg/server/server.py b/magg/server/server.py index 83764dc..041438b 100644 --- a/magg/server/server.py +++ b/magg/server/server.py @@ -250,7 +250,9 @@ async def add_server( self, name: Annotated[str, Field(description="Unique server name")], source: Annotated[str, Field(description="URL of the server package/repository")], - prefix: Annotated[str | None, Field(description="Tool prefix (defaults to conformed server name)")] = None, + prefix: Annotated[ + str | None, Field(description="Tool prefix (default: none, tools keep their original names)") + ] = None, command: Annotated[ str | None, Field(description="Full command to run (e.g., 'python server.py', 'npx @playwright/mcp@latest')"), @@ -273,9 +275,15 @@ async def add_server( try: config = self.config + if config.read_only: + return MaggResponse.error("Cannot add server in read-only mode") + if name in config.servers: return MaggResponse.error(f"Server '{name}' already exists") + if not command and not uri: + return MaggResponse.error(f"Server '{name}' needs a way to run: provide either command or uri") + actual_command = command actual_args = None if command: @@ -300,7 +308,7 @@ async def add_server( prefix=prefix, command=actual_command, args=actual_args, - uri=uri, + uri=str(uri) if uri else None, env=json_to_dict(env), cwd=cwd, notes=notes, @@ -316,11 +324,14 @@ async def add_server( mount_success = await self.server_manager.mount_server(server) if not mount_success: - return MaggResponse.error(f"Failed to mount server '{name}'") + reason = self.server_manager.mount_errors.get(name) + return MaggResponse.error(f"Failed to mount server '{name}'" + (f": {reason}" if reason else "")) config.add_server(server) if not self.save_config(config): + if mount_success: + await self.server_manager.unmount_server(name) return MaggResponse.error(f"Failed to save configuration for added server '{name}'") return MaggResponse.success( @@ -355,6 +366,9 @@ async def remove_server( try: config = self.config + if config.read_only: + return MaggResponse.error("Cannot remove server in read-only mode") + if name in config.servers: config.remove_server(name) @@ -412,6 +426,9 @@ async def enable_server( try: config = self.config + if config.read_only: + return MaggResponse.error("Cannot enable server in read-only mode") + if name not in config.servers: return MaggResponse.error(f"Server '{name}' not found") @@ -440,6 +457,9 @@ async def disable_server( try: config = self.config + if config.read_only: + return MaggResponse.error("Cannot disable server in read-only mode") + if name not in config.servers: return MaggResponse.error(f"Server '{name}' not found") @@ -786,12 +806,16 @@ async def check( timeout: Annotated[float, Field(description="Timeout in seconds for health check per server")] = 2.5, ) -> MaggResponse: """Check health of all mounted servers and handle unresponsive ones.""" + # Remounting and unmounting only affect this process; disabling writes the config + if action == "disable" and self.config.read_only: + return MaggResponse.error("Check action 'disable' is not allowed in read-only mode") + try: results = {} unresponsive_servers = [] - for server_name, server_info in self.server_manager.mounted_servers.items(): - client = server_info.get("client") + for server_name, server_info in list(self.server_manager.mounted_servers.items()): + client = server_info.client if not client: results[server_name] = {"status": "error", "reason": "No client found"} unresponsive_servers.append(server_name) diff --git a/magg/settings.py b/magg/settings.py index 903a239..0bfd536 100644 --- a/magg/settings.py +++ b/magg/settings.py @@ -3,6 +3,8 @@ import json import logging import os +import stat +import tempfile from functools import cached_property from pathlib import Path from typing import TYPE_CHECKING, Any, Callable, Coroutine @@ -10,13 +12,22 @@ if TYPE_CHECKING: from .reload import ConfigChange -from pydantic import AnyUrl, BaseModel, Field, field_validator, model_validator +from pydantic import AnyUrl, BaseModel, ConfigDict, Field, field_validator, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict from .util.paths import get_contrib_paths from .util.system import get_project_root -__all__ = "ServerConfig", "MaggConfig", "ConfigManager", "AuthConfig", "BearerAuthConfig", "ClientSettings", "KitInfo" +__all__ = ( + "ServerConfig", + "MaggConfig", + "ConfigManager", + "ConfigFileError", + "AuthConfig", + "BearerAuthConfig", + "ClientSettings", + "KitInfo", +) logger = logging.getLogger(__name__) @@ -48,10 +59,10 @@ class ClientSettings(BaseSettings): jwt: str | None = Field(default=None, description="JWT token for authentication (env: MAGG_JWT)") -class BearerAuthConfig(BaseSettings): +class BearerAuthConfig(BaseModel): """Bearer token authentication configuration.""" - model_config = SettingsConfigDict( + model_config = ConfigDict( extra="allow", validate_assignment=True, ) @@ -108,20 +119,20 @@ def public_key_exists(self) -> bool: return self.public_key_path.exists() -class AuthConfig(BaseSettings): +class AuthConfig(BaseModel): """Top-level authentication configuration.""" - model_config = SettingsConfigDict( + model_config = ConfigDict( extra="allow", validate_assignment=True, ) bearer: BearerAuthConfig = Field(default_factory=BearerAuthConfig, description="Bearer token authentication config") -class ServerConfig(BaseSettings): +class ServerConfig(BaseModel): """Server configuration - defines how to run an MCP server.""" - model_config = SettingsConfigDict( + model_config = ConfigDict( extra="allow", validate_assignment=True, arbitrary_types_allowed=True, @@ -210,9 +221,19 @@ class MaggConfig(BaseSettings): reload_poll_interval: float = Field( default=1.0, description="Config file poll interval in seconds (env: MAGG_RELOAD_POLL_INTERVAL)" ) + reload_use_watchdog: bool | None = Field( + default=None, + description="Use file system notifications (true) or polling (false) to detect config changes, " + "default auto-detect (env: MAGG_RELOAD_USE_WATCHDOG)", + ) stderr_show: bool = Field( default=False, description="Show stderr output from subprocess MCP servers (env: MAGG_STDERR_SHOW)" ) + backend_init_timeout: float = Field( + default=30.0, + description="Seconds to wait for a backend server to connect and initialize, 0 to wait forever " + "(env: MAGG_BACKEND_INIT_TIMEOUT)", + ) servers: dict[str, ServerConfig] = Field( default_factory=dict, description="Servers configuration (loaded from config_path)" ) @@ -305,6 +326,44 @@ def get_enabled_servers(self) -> dict[str, ServerConfig]: return {name: server for name, server in self.servers.items() if server.enabled} +class ConfigFileError(ValueError): + """The config file exists but couldn't be read or parsed.""" + + +def _is_valid_server(name: str, server_data: Any) -> bool: + try: + ServerConfig.model_validate({**server_data, "name": name}) + return True + except Exception: + return False + + +def _write_json_atomic(path: Path, data: Any) -> None: + """Write JSON through a temp file and rename, so a crash can't leave a truncated file.""" + target = Path(os.path.realpath(path)) # Replace a symlink's target, not the link + fd, tmp_path = tempfile.mkstemp(dir=target.parent, prefix=f".{target.name}.", suffix=".tmp") + + try: + with os.fdopen(fd, "w") as f: + json.dump(data, f, indent=2) + f.flush() + os.fsync(f.fileno()) + + try: + os.chmod(tmp_path, stat.S_IMODE(target.stat().st_mode)) + except FileNotFoundError: + pass # New file: keep mkstemp's owner-only permissions + + os.replace(tmp_path, target) + + except BaseException: + try: + os.unlink(tmp_path) + except OSError: + pass + raise + + class ConfigManager: """Manages Magg configuration persistence.""" @@ -341,94 +400,137 @@ def load_config(self) -> MaggConfig: if cached: return cached + try: + return self.read_config() + except ConfigFileError as e: + self.logger.error("%s", e) + return MaggConfig() + + def read_config(self) -> MaggConfig: + """Read configuration from disk, bypassing the reload cache. + + Entries that fail validation are logged and skipped. Raises ConfigFileError + if the file exists but can't be parsed. + """ config = MaggConfig() + data = self._read_config_data() - if not self.config_path.exists(): + if data is None: return config - try: - with self.config_path.open("r") as f: - data = json.load(f) + servers = {} + + for name, server_data in data.pop("servers", {}).items(): + try: + servers[name] = ServerConfig.model_validate({**server_data, "name": name}) + except Exception as e: + self.logger.error("Error loading server %r: %s", name, e) + + config.servers = servers + + # Handle both old format (list of strings) and new format (dict) + kits_data = data.pop("kits", {}) + if isinstance(kits_data, list): + kits_data = {kit_name: {"name": kit_name, "source": "legacy"} for kit_name in kits_data} + + kits = {} + + for name, kit_data in kits_data.items(): + try: + kits[name] = KitInfo.model_validate(kit_data) if isinstance(kit_data, dict) else KitInfo(name=name) + except Exception as e: + self.logger.error("Error loading kit %r: %s", name, e) - servers = {} - - for name, server_data in data.pop("servers", {}).items(): - try: - server_data["name"] = name - servers[name] = ServerConfig.model_validate(server_data) - except Exception as e: - self.logger.error("Error loading server %r: %s", name, e) - continue - - config.servers = servers - - if "kits" in data: - # Handle both old format (list of strings) and new format (dict) - kits_data = data.pop("kits", {}) - if isinstance(kits_data, list): - # Convert old format to new format - config.kits = {kit_name: KitInfo(name=kit_name, source="legacy") for kit_name in kits_data} - else: - # Load new format - config.kits = { - name: KitInfo.model_validate(kit_data) if isinstance(kit_data, dict) else KitInfo(name=name) - for name, kit_data in kits_data.items() - } - - for key, value in data.items(): - if not hasattr(config, key): - self.logger.warning("Setting unknown config key %r in %s.", key, self.config_path) + config.kits = kits + + for key, value in data.items(): + if not hasattr(config, key): + self.logger.warning("Setting unknown config key %r in %s.", key, self.config_path) + try: setattr(config, key, value) + except Exception as e: + self.logger.error("Error loading config key %r: %s", key, e) - return config + return config - except Exception as e: - self.logger.error("Error loading config: %s", e) - return config + def _read_config_data(self) -> dict | None: + """Read the raw config file, or return None if it doesn't exist.""" + try: + text = self.config_path.read_text() + if not text.strip(): + return None + data = json.loads(text) + except FileNotFoundError: + return None + except (OSError, ValueError) as e: + raise ConfigFileError(f"Error loading config {self.config_path}: {e}") from e + + if ( + not isinstance(data, dict) + or not isinstance(data.get("servers", {}), dict) + or not isinstance(data.get("kits", {}), dict | list) + ): + raise ConfigFileError( + f'Error loading config {self.config_path}: expected a JSON object with a "servers" object' + ) + + return data def save_config(self, config: MaggConfig) -> bool: - """Save configuration to disk.""" - if config.read_only: + """Save configuration to disk. + + Settings and server entries in the file that couldn't be loaded are written back + unchanged, and a file that can't be parsed is never overwritten. + """ + if config.read_only or self.read_only: self.logger.warning("Config is read-only, not saving.") return False - if self.read_only: - raise RuntimeError("Config read_only value cannot be changed after initialization.") - try: - # Notify the reloader to ignore the next file change since we're making it - if self._reload_manager: - self._reload_manager.ignore_next_change() + data = self._read_config_data() or {} + except ConfigFileError as e: + self.logger.error("%s - not saving to avoid losing its contents. Fix or remove the file first.", e) + return False - data = { - "servers": { - name: server.model_dump( - mode="json", - exclude_unset=True, - exclude_none=True, - exclude_defaults=True, - by_alias=True, - exclude={"name"}, - ) - for name, server in config.servers.items() - } + try: + existing_servers = data.get("servers", {}) + + data["servers"] = { + name: server.model_dump( + mode="json", + exclude_unset=True, + exclude_none=True, + exclude_defaults=True, + by_alias=True, + exclude={"name"}, + ) + for name, server in config.servers.items() } + # An entry that failed validation was never loaded, so it isn't missing because it was removed + for name, server_data in existing_servers.items(): + if name not in data["servers"] and not _is_valid_server(name, server_data): + self.logger.warning("Keeping invalid server %r in %s as-is", name, self.config_path) + data["servers"][name] = server_data + if config.kits: data["kits"] = { name: kit_info.model_dump(mode="json", exclude_unset=True, exclude_none=True, exclude_defaults=True) for name, kit_info in config.kits.items() } + else: + data.pop("kits", None) if not self.config_path.parent.exists(): self.logger.warning("Creating new directory: %s", self.config_path.parent) self.config_path.parent.mkdir(parents=True, exist_ok=True) - with self.config_path.open("w") as f: - json.dump(data, f, indent=2) + _write_json_atomic(self.config_path, data) - # Update the reload manager's cached config to stay in sync + # Notify the reloader to ignore the change we just made if self._reload_manager: + self._reload_manager.ignore_next_change() + # Update the reload manager's cached config to stay in sync self._reload_manager.update_cached_config(config) return True @@ -504,8 +606,7 @@ def save_auth_config(self, auth_config: AuthConfig) -> bool: data = auth_config.model_dump(mode="json", exclude_none=True) - with self.auth_config_path.open("w") as f: - json.dump(data, f, indent=2) + _write_json_atomic(self.auth_config_path, data) self.auth_config = auth_config return True diff --git a/magg/util/stdio_patch.py b/magg/util/stdio_patch.py deleted file mode 100644 index 81bc3f0..0000000 --- a/magg/util/stdio_patch.py +++ /dev/null @@ -1,43 +0,0 @@ -"""Utilities for patching stdio transports to control stderr behavior.""" - -import os -from contextlib import asynccontextmanager -from pathlib import Path - -__all__ = ("patch_stdio_transport_stderr",) - - -def patch_stdio_transport_stderr(transport): - """Patch a stdio transport to suppress stderr output. - - This monkey-patches the transport's connect method to redirect stderr to /dev/null. - Only works for StdioTransport and its subclasses. - """ - from fastmcp.client.transports import StdioTransport - - if not isinstance(transport, StdioTransport): - return transport - - original_connect = transport.connect - - async def patched_connect(**session_kwargs): - from mcp.client import stdio - - original_stdio_client = stdio.stdio_client - - @asynccontextmanager - async def silent_stdio_client(server_params, errlog=None): - with Path(os.devnull).open("w") as devnull: - async with original_stdio_client(server_params, errlog=devnull) as result: - yield result - - stdio.stdio_client = silent_stdio_client - - try: - return await original_connect(**session_kwargs) - - finally: - stdio.stdio_client = original_stdio_client - - transport.connect = patched_connect - return transport diff --git a/pyproject.toml b/pyproject.toml index eecdd86..2f437c6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "magg" -version = "1.2.1" +version = "1.3.0" requires-python = ">=3.12" description = "MCP Aggregator" authors = [{ name = "Phillip Sitbon", email = "phillip.sitbon@gmail.com"}] diff --git a/readme.md b/readme.md index 154ec4c..2fd7cb6 100644 --- a/readme.md +++ b/readme.md @@ -330,7 +330,8 @@ Magg supports several environment variables for configuration: - `MAGG_RELOAD_POLL_INTERVAL` - Config polling interval in seconds (default: 1.0) - `MAGG_READ_ONLY=true` - Run in read-only mode - `MAGG_SELF_PREFIX` - Prefix for Magg tools (default: "magg"). Tools will be named as `{prefix}{sep}{tool}` (e.g., `magg_list_servers`) -- `MAGG_PREFIX_SEP` - Separator between prefix and tool name (default: "_") +- `MAGG_PREFIX_SEP` - Separator between Magg's prefix and its tool names (default: "_"). Backend server tools always use `_` +- `MAGG_BACKEND_INIT_TIMEOUT` - Seconds to wait for a backend server to start before giving up on it (default: 30, `0` waits forever) Example configuration: ```json diff --git a/scripts/bump_version.py b/scripts/bump_version.py new file mode 100755 index 0000000..1340b17 --- /dev/null +++ b/scripts/bump_version.py @@ -0,0 +1,77 @@ +#!/usr/bin/env python3 +"""Bump Magg's version, or check that the versions in the repo agree. + + scripts/bump_version.py minor # major, minor, patch, or an explicit version like 1.3.0 + scripts/bump_version.py --check # exit 1 if server.json doesn't match pyproject.toml + +Bumping runs `uv version`, which updates pyproject.toml and re-locks uv.lock, then copies +the new version into server.json (its top-level version and each package version). +""" + +import argparse +import json +import subprocess +import sys +import tomllib +from pathlib import Path + +ROOT = Path(__file__).resolve().parent.parent +PYPROJECT = ROOT / "pyproject.toml" +SERVER_JSON = ROOT / "server.json" + +# Values `uv version --bump` accepts; anything else is passed as an explicit version +BUMPS = ("major", "minor", "patch", "stable", "alpha", "beta", "rc", "post", "dev") + + +def project_version() -> str: + with PYPROJECT.open("rb") as f: + return tomllib.load(f)["project"]["version"] + + +def manifest_versions(manifest: dict) -> list[str]: + return [manifest["version"], *(package["version"] for package in manifest.get("packages", []))] + + +def check() -> int: + version = project_version() + versions = manifest_versions(json.loads(SERVER_JSON.read_text())) + + if any(v != version for v in versions): + print( + f"server.json versions {versions} don't match pyproject.toml version {version}. " + f"Run: scripts/bump_version.py {version}", + file=sys.stderr, + ) + return 1 + + print(f"server.json matches pyproject.toml version {version}") + return 0 + + +def bump(value: str) -> int: + args = ["--bump", value] if value in BUMPS else [value] + subprocess.run(["uv", "version", "--no-sync", *args], cwd=ROOT, check=True) + version = project_version() + + manifest = json.loads(SERVER_JSON.read_text()) + manifest["version"] = version + for package in manifest.get("packages", []): + package["version"] = version + SERVER_JSON.write_text(json.dumps(manifest, indent=2) + "\n") + + print(f"Updated server.json to {version}") + return 0 + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__.splitlines()[0]) + group = parser.add_mutually_exclusive_group(required=True) + group.add_argument("version", nargs="?", help=f"one of {', '.join(BUMPS)}, or an explicit version") + group.add_argument("--check", action="store_true", help="only check that server.json matches pyproject.toml") + args = parser.parse_args() + + return check() if args.check else bump(args.version) + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/server.json b/server.json index ddd17b0..95b0763 100644 --- a/server.json +++ b/server.json @@ -7,14 +7,14 @@ "url": "https://github.com/sitbon/magg", "source": "github" }, - "version": "1.2.1", + "version": "1.3.0", "websiteUrl": "https://github.com/sitbon/magg", "packages": [ { "registryType": "pypi", "registryBaseUrl": "https://pypi.org", "identifier": "magg", - "version": "1.2.1", + "version": "1.3.0", "runtimeHint": "uvx", "transport": { "type": "stdio" diff --git a/test/magg/test_cli_fixes.py b/test/magg/test_cli_fixes.py new file mode 100644 index 0000000..a0de3f9 --- /dev/null +++ b/test/magg/test_cli_fixes.py @@ -0,0 +1,146 @@ +"""Tests for auth CLI and server search robustness fixes.""" + +import asyncio +import json +import os +import subprocess +import sys + +import pytest + +from magg.auth import BearerAuthManager +from magg.cli import cmd_auth, create_parser +from magg.discovery.search import ToolSearchEngine +from magg.settings import BearerAuthConfig + + +class TestAuthCLI: + """magg auth commands.""" + + @pytest.fixture + def auth_env(self, tmp_path, monkeypatch): + """Point HOME and the config at a temp dir with no ~/.ssh.""" + home = tmp_path / "home" + home.mkdir() + monkeypatch.setenv("HOME", str(home)) + monkeypatch.delenv("MAGG_PRIVATE_KEY", raising=False) + return tmp_path / "config" / "config.json" + + async def run_auth(self, config_path, *argv): + args = create_parser().parse_args(["--config", str(config_path), "auth", *argv]) + return await cmd_auth(args) + + def test_generate_keypair_creates_missing_parents(self, tmp_path): + key_path = tmp_path / "home" / ".ssh" / "magg" + manager = BearerAuthManager(BearerAuthConfig(key_path=key_path)) + + manager.generate_keys() + + private_path = key_path / "magg.key" + assert private_path.exists() + assert oct(private_path.stat().st_mode)[-3:] == "600" + + @pytest.mark.asyncio + async def test_init_twice_reports_error(self, auth_env, tmp_path, capsys): + key_path = tmp_path / "keys" + + assert await self.run_auth(auth_env, "init", "--key-path", str(key_path)) == 0 + assert await self.run_auth(auth_env, "init", "--key-path", str(key_path)) == 1 + + assert "already exists" in capsys.readouterr().err + + @pytest.mark.asyncio + async def test_custom_key_path_is_used_afterwards(self, auth_env, tmp_path, capsys): + # Previously the key was generated at --key-path, but later commands looked in the default place + key_path = tmp_path / "keys" + + assert await self.run_auth(auth_env, "init", "--key-path", str(key_path)) == 0 + capsys.readouterr() + + assert await self.run_auth(auth_env, "token", "-q") == 0 + token = capsys.readouterr().out.strip() + assert token.count(".") == 2 + + manager = BearerAuthManager(BearerAuthConfig(key_path=key_path)) + manager.load_keys() + assert manager.provider is not None + assert json.loads((auth_env.parent / "auth.json").read_text())["bearer"]["key_path"] == str(key_path) + + @pytest.mark.asyncio + async def test_private_key_export_is_shell_safe(self, auth_env, tmp_path, capsys): + key_path = tmp_path / "keys" + assert await self.run_auth(auth_env, "init", "--key-path", str(key_path)) == 0 + capsys.readouterr() + + assert await self.run_auth(auth_env, "private-key", "--export") == 0 + line = capsys.readouterr().out.strip() + + result = subprocess.run( + ["sh", "-c", f'{line}; printf %s "$MAGG_PRIVATE_KEY"'], capture_output=True, text=True, check=True + ) + pem = result.stdout.replace("\\n", "\n") + assert pem == (key_path / "magg.key").read_text() + + +class TestSearch: + """ToolSearchEngine handles missing fields and slow sources.""" + + def test_github_null_description(self): + data = { + "items": [ + {"name": "no-desc", "description": None, "html_url": "https://github.com/a/b"}, + {"name": "mcp-thing", "description": "An MCP server", "html_url": "https://github.com/a/c"}, + ] + } + + results = ToolSearchEngine._parse_github_results(data) + + assert [r.name for r in results] == ["mcp-thing"] + + def test_npm_null_author_and_description(self): + data = {"objects": [{"package": {"name": "pkg", "description": None, "author": None}}]} + + results = ToolSearchEngine._parse_npm_results(data) + + assert results[0].description == "" + assert results[0].metadata["author"] is None + + @pytest.mark.asyncio + async def test_search_all_runs_sources_concurrently(self, monkeypatch): + # Each source waits until all three have started, which can only happen if they run together + all_started = asyncio.Barrier(3) + + def source(name): + async def search(self, query, limit): + await all_started.wait() + return [name] + + return search + + async def broken(self, query, limit): + raise RuntimeError("offline") + + monkeypatch.setattr(ToolSearchEngine, "search_registry", source("registry")) + monkeypatch.setattr(ToolSearchEngine, "search_glama", source("glama")) + monkeypatch.setattr(ToolSearchEngine, "search_github", source("github")) + monkeypatch.setattr(ToolSearchEngine, "search_npm", broken) + + results = await asyncio.wait_for(ToolSearchEngine().search_all("x"), 5) + + # One failing source doesn't affect the others + assert results == {"mcp-registry": ["registry"], "glama": ["glama"], "github": ["github"], "npm": []} + + +class TestInvalidSettings: + """Bad MAGG_* values produce a readable error, not a traceback.""" + + def test_invalid_env_value(self, tmp_path): + env = {**os.environ, "MAGG_AUTO_RELOAD": "maybe", "MAGG_CONFIG_PATH": str(tmp_path / "config.json")} + + result = subprocess.run( + [sys.executable, "-m", "magg", "server", "list"], capture_output=True, text=True, env=env, timeout=60 + ) + + assert result.returncode == 1 + assert "MAGG_AUTO_RELOAD" in result.stderr + assert "Traceback" not in result.stderr diff --git a/test/magg/test_config_reload.py b/test/magg/test_config_reload.py index 779d001..e6c35a4 100644 --- a/test/magg/test_config_reload.py +++ b/test/magg/test_config_reload.py @@ -9,6 +9,7 @@ import pytest from magg.reload import ConfigChange, ConfigReloader, ServerChange +from magg.server.manager import MountedServer from magg.server.runner import MaggRunner from magg.server.server import MaggServer from magg.settings import MaggConfig, ServerConfig @@ -355,8 +356,8 @@ async def test_check_with_failed_server(self, temp_config_file): mock_bad_client.list_tools.side_effect = Exception("Server session was closed unexpectedly") server.server_manager.mounted_servers = { - "good-server": {"client": mock_good_client}, - "bad-server": {"client": mock_bad_client}, + "good-server": MountedServer(proxy=None, client=mock_good_client), + "bad-server": MountedServer(proxy=None, client=mock_bad_client), } # Run check with default 0.5s timeout diff --git a/test/magg/test_config_safety.py b/test/magg/test_config_safety.py new file mode 100644 index 0000000..4201f8f --- /dev/null +++ b/test/magg/test_config_safety.py @@ -0,0 +1,339 @@ +"""Tests that saving the config never loses data the user wrote.""" + +import asyncio +import json +import os +import stat +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +from magg import reload as reload_module +from magg.kit import KitManager +from magg.reload import ConfigReloader +from magg.settings import BearerAuthConfig, ConfigFileError, ConfigManager, ServerConfig + + +@pytest.fixture +def config_path(tmp_path): + return tmp_path / "config.json" + + +def write_config(path, data): + path.write_text(json.dumps(data, indent=2)) + + +def read_config(path): + return json.loads(path.read_text()) + + +def replace_config(path, data): + """Write the config the way many editors and tools do: a new file renamed over the old one.""" + tmp_path = path.with_name(f".{path.name}.tmp") + tmp_path.write_text(json.dumps(data)) + os.replace(tmp_path, path) + + +async def wait_until(predicate, timeout=5.0): + """Poll until predicate() is true, returning whether it became true in time.""" + deadline = asyncio.get_running_loop().time() + timeout + while not predicate(): + if asyncio.get_running_loop().time() > deadline: + return False + await asyncio.sleep(0.05) + return True + + +def server_names(change): + return {server_change.name for server_change in change.server_changes} + + +class TestEnvironmentIsolation: + """Nested config models must not read unrelated environment variables.""" + + def test_server_config_ignores_env(self, monkeypatch): + # Termux sets PREFIX=/usr, POSIX shells use ENV, and URI/ENABLED are common names + monkeypatch.setenv("PREFIX", "/usr") + monkeypatch.setenv("URI", "http://evil.example/mcp") + monkeypatch.setenv("ENABLED", "false") + monkeypatch.setenv("ENV", "/home/user/.shrc") + + server = ServerConfig.model_validate({"name": "calc", "source": "https://x", "command": "echo"}) + + assert server.prefix is None + assert server.uri is None + assert server.enabled is True + assert server.env is None + + def test_bearer_auth_config_ignores_env(self, monkeypatch, tmp_path): + monkeypatch.setenv("AUDIENCE", "other") + monkeypatch.setenv("KEY_PATH", str(tmp_path)) + + config = BearerAuthConfig() + + assert config.audience == "magg" + assert config.key_path == Path.home() / ".ssh" / "magg" + + +class TestSaveConfig: + """ConfigManager.save_config preserves what it didn't load.""" + + def test_refuses_to_overwrite_unparseable_file(self, config_path): + original = '{"servers": {"a": {"source": "s", "command": "echo"}},}' + config_path.write_text(original) + manager = ConfigManager(str(config_path)) + + config = manager.load_config() + assert config.servers == {} + + config.add_server(ServerConfig(name="b", source="s", command="echo")) + assert manager.save_config(config) is False + assert config_path.read_text() == original + + def test_read_config_raises_on_unparseable_file(self, config_path): + config_path.write_text("[1, 2, 3]") + + with pytest.raises(ConfigFileError): + ConfigManager(str(config_path)).read_config() + + def test_empty_file_is_treated_as_new(self, config_path): + config_path.write_text("") + manager = ConfigManager(str(config_path)) + + config = manager.load_config() + config.add_server(ServerConfig(name="a", source="s", command="echo")) + + assert manager.save_config(config) is True + assert list(read_config(config_path)["servers"]) == ["a"] + + def test_invalid_server_is_kept(self, config_path): + write_config( + config_path, + { + "servers": { + "good": {"source": "s", "command": "echo"}, + "bad": {"source": "s", "command": "echo", "prefix": "not-valid!"}, + } + }, + ) + manager = ConfigManager(str(config_path)) + + config = manager.load_config() + assert list(config.servers) == ["good"] + + config.add_server(ServerConfig(name="new", source="s", command="echo")) + assert manager.save_config(config) is True + + servers = read_config(config_path)["servers"] + assert set(servers) == {"good", "bad", "new"} + assert servers["bad"]["prefix"] == "not-valid!" + + def test_removed_server_is_removed(self, config_path): + write_config( + config_path, + {"servers": {"a": {"source": "s", "command": "echo"}, "b": {"source": "s", "command": "echo"}}}, + ) + manager = ConfigManager(str(config_path)) + + config = manager.load_config() + config.remove_server("a") + assert manager.save_config(config) is True + + assert list(read_config(config_path)["servers"]) == ["b"] + + def test_top_level_settings_are_kept(self, config_path): + write_config( + config_path, + {"self_prefix": "hub", "servers": {"a": {"source": "s", "command": "echo"}}, "custom": {"x": 1}}, + ) + manager = ConfigManager(str(config_path)) + + config = manager.load_config() + assert config.self_prefix == "hub" + + config.remove_server("a") + assert manager.save_config(config) is True + + data = read_config(config_path) + assert list(data) == ["self_prefix", "servers", "custom"] + assert data["self_prefix"] == "hub" + assert data["custom"] == {"x": 1} + + def test_invalid_setting_does_not_discard_later_settings(self, config_path): + write_config( + config_path, + {"auto_reload": "maybe", "self_prefix": "hub", "servers": {"a": {"source": "s", "command": "echo"}}}, + ) + + config = ConfigManager(str(config_path)).load_config() + + assert config.auto_reload is True # Invalid value skipped, default kept + assert config.self_prefix == "hub" + assert list(config.servers) == ["a"] + + def test_write_preserves_mode_and_symlink(self, tmp_path): + real_path = tmp_path / "real.json" + write_config(real_path, {"servers": {}}) + os.chmod(real_path, 0o640) + link_path = tmp_path / "config.json" + link_path.symlink_to(real_path) + manager = ConfigManager(str(link_path)) + + config = manager.load_config() + config.add_server(ServerConfig(name="a", source="s", command="echo")) + assert manager.save_config(config) is True + + assert link_path.is_symlink() + assert list(read_config(real_path)["servers"]) == ["a"] + assert stat.S_IMODE(real_path.stat().st_mode) == 0o640 + assert sorted(p.name for p in tmp_path.iterdir()) == ["config.json", "real.json"] + + @pytest.mark.asyncio + async def test_kits_survive_save_with_reload_enabled(self, config_path): + write_config( + config_path, + { + "servers": {"calc": {"source": "s", "command": "echo", "enabled": False, "kits": ["mykit"]}}, + "kits": {"mykit": {"name": "mykit", "description": "d", "source": "file"}}, + }, + ) + manager = ConfigManager(str(config_path)) + + async def callback(change): + pass + + await manager.setup_config_reload(callback) + try: + await asyncio.sleep(0.1) # Let the watcher load its baseline + + config = manager.load_config() + assert list(config.kits) == ["mykit"] + + config.add_server(ServerConfig(name="other", source="s", command="echo", enabled=False)) + assert manager.save_config(config) is True + finally: + await manager.stop_config_reload() + + data = read_config(config_path) + assert list(data["kits"]) == ["mykit"] + assert set(data["servers"]) == {"calc", "other"} + + +class TestReload: + """Config reload picks up real file changes, including ones made by rename.""" + + @pytest.fixture + def changes(self): + return [] + + @pytest.fixture + def callback(self, changes): + async def callback(change): + changes.append(change) + + return callback + + @pytest.mark.asyncio + async def test_rename_over_config_triggers_reload(self, config_path, changes, callback): + write_config(config_path, {"servers": {}}) + reloader = ConfigReloader(config_path, callback) + await reloader.start_watching(poll_interval=0.1) + try: + await asyncio.sleep(0.2) # Let the watcher load its baseline + + replace_config(config_path, {"servers": {"a": {"source": "s", "command": "echo"}}}) + + assert await wait_until(lambda: changes), "rename over the config was not detected" + assert server_names(changes[0]) == {"a"} + finally: + await reloader.stop_watching() + + @pytest.mark.asyncio + async def test_own_save_is_ignored_but_next_edit_is_not(self, config_path, changes, callback): + write_config(config_path, {"servers": {}}) + manager = ConfigManager(str(config_path)) + await manager.setup_config_reload(callback) + try: + await asyncio.sleep(0.2) + + # Magg's own save replaces the file too, and must not echo back as a reload + config = manager.load_config() + config.add_server(ServerConfig(name="saved", source="s", command="echo")) + assert manager.save_config(config) is True + await asyncio.sleep(0.5) + assert changes == [] + + # The ignore must be used up by that save, so the next outside edit still reloads + data = read_config(config_path) + data["servers"]["edited"] = {"source": "s", "command": "echo"} + replace_config(config_path, data) + + assert await wait_until(lambda: changes), "edit after an internal save was not detected" + assert server_names(changes[0]) == {"edited"} + finally: + await manager.stop_config_reload() + + @pytest.mark.asyncio + async def test_polling_when_watchdog_disabled(self, config_path, changes, callback, monkeypatch): + observer = MagicMock() + monkeypatch.setattr(reload_module, "Observer", observer) + write_config(config_path, {"servers": {}}) + reloader = ConfigReloader(config_path, callback) + await reloader.start_watching(poll_interval=0.1, use_watchdog=False) + try: + await asyncio.sleep(0.2) + write_config(config_path, {"servers": {"a": {"source": "s", "command": "echo"}}}) + # Polling compares mtimes, so make sure it moves even on coarse-grained filesystems + os.utime(config_path, (config_path.stat().st_atime, config_path.stat().st_mtime + 1)) + + assert await wait_until(lambda: changes), "polling did not detect the change" + finally: + await reloader.stop_watching() + + observer.assert_not_called() + + @pytest.mark.asyncio + async def test_disabled_placeholder_does_not_block_reload(self, config_path, changes, callback): + write_config(config_path, {"servers": {}}) + reloader = ConfigReloader(config_path, callback) + await reloader.reload_config() # Establish the baseline + + write_config( + config_path, + { + "servers": { + "placeholder": {"source": "s", "enabled": False}, + "real": {"source": "s", "command": "echo"}, + } + }, + ) + assert await reloader.reload_config() is not None + + assert server_names(changes[-1]) == {"placeholder", "real"} + + @pytest.mark.asyncio + async def test_unparseable_file_is_not_applied(self, config_path, changes, callback): + write_config(config_path, {"servers": {"a": {"source": "s", "command": "echo"}}}) + reloader = ConfigReloader(config_path, callback) + await reloader.reload_config() # Establish the baseline + changes.clear() + + # A half-written or mistyped file must not look like "every server was removed" + config_path.write_text('{"servers": {') + assert await reloader.reload_config() is None + assert changes == [] + + +class TestKitDiscovery: + """Kits next to an explicitly configured config file are found.""" + + def test_kitd_next_to_config(self, tmp_path, monkeypatch): + monkeypatch.setenv("MAGG_PATH", str(tmp_path / "elsewhere")) + config_dir = tmp_path / "project" + (config_dir / "kit.d").mkdir(parents=True) + (config_dir / "kit.d" / "local.json").write_text(json.dumps({"name": "local", "servers": {}})) + + kit_manager = KitManager(ConfigManager(str(config_dir / "config.json"))) + + assert "local" in kit_manager.discover_kits() diff --git a/test/magg/test_kit_tools.py b/test/magg/test_kit_tools.py index 2106c2e..598fea4 100644 --- a/test/magg/test_kit_tools.py +++ b/test/magg/test_kit_tools.py @@ -99,6 +99,7 @@ async def test_unload_kit_success(self, server): """Test successfully unloading a kit.""" # Create a persistent config object mock_config = MagicMock() + mock_config.read_only = False from magg.settings import KitInfo mock_config.kits = {"test-kit": KitInfo(name="test-kit", source="file")} diff --git a/test/magg/test_mbro_cli_overhaul.py b/test/magg/test_mbro_cli_overhaul.py index 3587a5b..a77d6bf 100644 --- a/test/magg/test_mbro_cli_overhaul.py +++ b/test/magg/test_mbro_cli_overhaul.py @@ -77,7 +77,7 @@ class TestMBroCLIOverhaul: async def test_handle_commands_from_args(self): """Test handling commands from command line args.""" cli = MagicMock() - cli.handle_command = AsyncMock() + cli.run_commands = AsyncMock() args = MagicMock() args.commands = ["connect test python server.py", ";", "tools"] @@ -85,15 +85,13 @@ async def test_handle_commands_from_args(self): executed = await handle_commands(cli, args) assert executed is True - assert cli.handle_command.call_count == 2 - cli.handle_command.assert_any_call("connect test python server.py") - cli.handle_command.assert_any_call("tools") + cli.run_commands.assert_awaited_once_with(["connect test python server.py", "tools"]) @pytest.mark.asyncio async def test_handle_commands_from_stdin(self): """Test handling commands from stdin.""" cli = MagicMock() - cli.handle_command = AsyncMock() + cli.run_commands = AsyncMock() args = MagicMock() args.commands = ["-"] @@ -104,9 +102,7 @@ async def test_handle_commands_from_stdin(self): executed = await handle_commands(cli, args) assert executed is True - assert cli.handle_command.call_count == 2 - cli.handle_command.assert_any_call("connect test python server.py") - cli.handle_command.assert_any_call("tools") + cli.run_commands.assert_awaited_once_with(["connect test python server.py", "tools"]) def test_cli_command_parsing_with_comments(self): """Test that CLI properly handles comments.""" diff --git a/test/magg/test_mbro_commands.py b/test/magg/test_mbro_commands.py new file mode 100644 index 0000000..c726d70 --- /dev/null +++ b/test/magg/test_mbro_commands.py @@ -0,0 +1,343 @@ +"""Tests for mbro command handling: argument parsing, errors, exit codes.""" + +import asyncio +import shutil +import sys +import time +from unittest.mock import AsyncMock, patch + +import pytest +from fastmcp import Client, FastMCP + +from magg.mbro import cli as cli_module +from magg.mbro.cli import MCPBrowserCLI, main_async +from magg.mbro.client import BrowserConnection +from magg.mbro.parser import CommandParser + + +def make_server() -> FastMCP: + mcp = FastMCP("test") + + @mcp.tool(description="Echo a string value") + def echo(value: str) -> str: + return f"echo:{value!r}" + + @mcp.tool + def nodesc(n: int) -> str: + return f"n:{n!r}" + + @mcp.tool + def anyval(value: object = None) -> str: + return f"any:{value!r}" + + @mcp.prompt + def greet(name: str) -> str: + return f"Hello {name}" + + @mcp.resource("res://nodesc") + def nodesc_res() -> str: + return "resource-content" + + return mcp + + +@pytest.fixture +def cli(tmp_path, monkeypatch): + monkeypatch.setenv("MAGG_PATH", str(tmp_path)) + monkeypatch.setenv("MAGG_CONFIG_PATH", str(tmp_path / "config.json")) + return MCPBrowserCLI(use_rich=False) + + +async def run(cli: MCPBrowserCLI, *commands: str) -> bool: + """Run commands, returning whether all of them succeeded.""" + return await cli.run_commands(list(commands)) + + +def attach(cli: MCPBrowserCLI, name: str = "t") -> None: + """Attach an in-memory connection to the test server.""" + conn = BrowserConnection(name, "memory", "memory") + conn.client = Client(make_server()) + conn.connected = True + cli.browser.connections[name] = conn + cli.browser.current_connection = name + + +class TestJsonArguments: + """JSON arguments for call/prompt keep their inner quotes.""" + + def test_unquoted_json(self): + parts = CommandParser.parse_command_line('call echo {"value": "hello world"}') + assert parts == ["call", "echo", '{"value": "hello world"}'] + + def test_quoted_json(self): + parts = CommandParser.parse_command_line("""call echo '{"value": "hello world"}'""") + assert parts == ["call", "echo", '{"value": "hello world"}'] + + def test_prompt_json(self): + parts = CommandParser.parse_command_line('prompt greet {"name": "Bob"}') + assert parts == ["prompt", "greet", '{"name": "Bob"}'] + + def test_other_commands_still_shell_split(self): + assert CommandParser.parse_command_line('search "a b"') == ["search", "a b"] + + def test_multiline_json_in_script(self): + script = ( + "connect t python server.py\n" + "call echo {\n" + ' "value": "a # b; c", # trailing comment\n' + "\n" + " \"other\": 'x'\n" + "}\n" + "tools\n" + ) + assert CommandParser.split_commands(script) == [ + "connect t python server.py", + 'call echo {\n "value": "a # b; c",\n\n "other": \'x\'\n}', + "tools", + ] + + def test_braces_in_quotes_do_not_join_lines(self): + commands = CommandParser.split_commands('call echo value="{"\ntools') + assert commands == ['call echo value="{"', "tools"] + + @pytest.mark.asyncio + async def test_call_forms(self, cli, capsys): + attach(cli) + commands = [ + "call echo value=kv", + 'call echo {"value": "unquoted"}', + """call echo '{"value": "quoted"}'""", + *CommandParser.split_commands('call echo {\n "value": "multi"\n}'), + ] + assert await cli.run_commands(commands) + + out = capsys.readouterr().err + for value in ("kv", "unquoted", "quoted", "multi"): + assert f"echo:'{value}'" in out + + @pytest.mark.asyncio + async def test_invalid_json_fails(self, cli, capsys): + attach(cli) + assert not await run(cli, "call echo {value: 1}") + assert "Invalid JSON arguments" in capsys.readouterr().err + + +class TestValueParsing: + """key=value parsing for call/prompt.""" + + def test_without_schema(self): + args = ["a=007", "b=10-20", "c=5-", "d=null", "e=[1,2]", "f=1e5", "g=42", "h=-3", "i=True", "j=my server"] + assert MCPBrowserCLI.parse_shell_args(args) == { + "a": "007", + "b": "10-20", + "c": "5-", + "d": None, + "e": [1, 2], + "f": 100000.0, + "g": 42, + "h": -3, + "i": True, + "j": "my server", + } + + def test_string_params_stay_strings(self): + schema = { + "properties": { + "s": {"type": "string"}, + "opt": {"anyOf": [{"type": "string"}, {"type": "null"}]}, + "n": {"type": "integer"}, + "u": {"anyOf": [{"type": "string"}, {"type": "integer"}]}, + } + } + args = ["s=123", "opt=true", "n=5", "u=5"] + assert MCPBrowserCLI.parse_shell_args(args, schema) == {"s": "123", "opt": "true", "n": 5, "u": 5} + + def test_strings_flag(self): + assert MCPBrowserCLI.parse_shell_args(["a=5", "b=null"], strings=True) == {"a": "5", "b": "null"} + + @pytest.mark.asyncio + async def test_call_uses_schema(self, cli, capsys): + attach(cli) + assert await cli.run_commands(["call echo value=10-20", "call echo value=007", "call nodesc n=5"]) + + out = capsys.readouterr().err + assert "echo:'10-20'" in out + assert "echo:'007'" in out + assert "n:5" in out + + @pytest.mark.asyncio + async def test_prompt_key_value(self, cli, capsys): + attach(cli) + assert await run(cli, "prompt greet name=Bob") + assert "Hello Bob" in capsys.readouterr().err + + @pytest.mark.asyncio + async def test_positional_rejected(self, cli, capsys): + attach(cli) + assert not await run(cli, "prompt greet Bob") + assert "Positional arguments are not supported" in capsys.readouterr().err + + +class TestCommands: + """Individual command behavior.""" + + @pytest.mark.asyncio + async def test_status_without_connection(self, cli, capsys): + assert not await run(cli, "status") + assert "No active connection" in capsys.readouterr().err + + @pytest.mark.asyncio + async def test_status(self, cli, capsys): + attach(cli) + assert await run(cli, "status") + assert '"tools": 3' in capsys.readouterr().out + + @pytest.mark.asyncio + @pytest.mark.parametrize("command", ["tools zzz", "prompts zzz", "search zzz", "search nodesc"]) + async def test_none_descriptions(self, cli, command): + attach(cli) + assert await run(cli, command) + + @pytest.mark.asyncio + async def test_search_json_resource_without_description(self, tmp_path, monkeypatch, capsys): + monkeypatch.setenv("MAGG_PATH", str(tmp_path)) + cli = MCPBrowserCLI(json_only=True, use_rich=False) + attach(cli) + assert await run(cli, "search nodesc") + out = capsys.readouterr().out + assert '"nodesc_res"' in out + + @pytest.mark.asyncio + async def test_unknown_tool_suggestion(self, cli, capsys): + attach(cli) + assert not await run(cli, "call ech value=x") + assert "Did you mean: echo?" in capsys.readouterr().err + + @pytest.mark.asyncio + async def test_duplicate_connection_name(self, cli, capsys): + attach(cli) + assert not await run(cli, "connect t python server.py") + assert "connection name 't' already exists" in capsys.readouterr().err + + @pytest.mark.asyncio + async def test_connect_failure_reason(self, cli, capfd): + # capfd rather than capsys: the stdio transport needs a real stderr fd + assert not await run(cli, "connect nx this-command-does-not-exist-mbro") + assert "No such file or directory" in capfd.readouterr().err + + @pytest.mark.asyncio + @pytest.mark.skipif(not shutil.which("sleep"), reason="needs sleep") + async def test_connect_timeout(self): + conn = BrowserConnection("s", "command", "sleep 100") + with pytest.raises(TimeoutError, match="0.5s"): + await conn.connect(timeout=0.5) + + @pytest.mark.asyncio + @pytest.mark.skipif(not shutil.which("sleep"), reason="needs sleep") + async def test_connect_timeout_zero_disables_it(self): + # fastmcp treats an init_timeout of 0 as disabled, so --timeout 0 keeps waiting + conn = BrowserConnection("s", "command", "sleep 100") + start = time.monotonic() + with pytest.raises(TimeoutError): + await asyncio.wait_for(conn.connect(timeout=0), 1) + assert time.monotonic() - start >= 1 + + @pytest.mark.asyncio + async def test_missing_script_does_not_exit(self, cli, capsys): + assert not await run(cli, "script run nope") + assert "Script not found" in capsys.readouterr().err + assert cli.running + + @pytest.mark.asyncio + async def test_repl_without_arepl(self, cli, capsys): + with patch.object(cli_module, "arepl", None): + await cli.start(repl=True) + assert "only available" in capsys.readouterr().err + + +class TestUrlNormalization: + @pytest.mark.parametrize( + "url, expected", + [ + ("http://h:1", "http://h:1/mcp/"), + ("http://h:1/", "http://h:1/mcp/"), + ("http://h:1?x=1", "http://h:1/mcp/?x=1"), + ("http://h:1/mcp", "http://h:1/mcp"), + ("http://h:1/mcp/", "http://h:1/mcp/"), + ("http://h:1/sse", "http://h:1/sse"), + ("https://h/api/mcp", "https://h/api/mcp"), + ], + ) + def test_normalize_url(self, url, expected): + assert BrowserConnection.normalize_url(url) == expected + + +class TestBatchExecution: + """Error handling and exit codes when running command batches.""" + + @pytest.mark.asyncio + async def test_continues_after_failures(self, cli, capsys): + attach(cli) + assert not await cli.run_commands(["bogus", "call echo value=after"]) + assert cli.failed + assert "echo:'after'" in capsys.readouterr().err + + @pytest.mark.asyncio + async def test_unexpected_exception_is_contained(self, cli, capsys): + attach(cli) + with patch.object(cli.command, "tools", AsyncMock(side_effect=RuntimeError("boom"))): + assert not await cli.run_commands(["tools", "call echo value=after"]) + assert "echo:'after'" in capsys.readouterr().err + + @pytest.mark.asyncio + async def test_fail_fast(self, tmp_path, monkeypatch, capsys): + monkeypatch.setenv("MAGG_PATH", str(tmp_path)) + cli = MCPBrowserCLI(use_rich=False, fail_fast=True) + attach(cli) + assert not await cli.run_commands(["bogus", "call echo value=after"]) + assert "echo:'after'" not in capsys.readouterr().err + + @pytest.mark.asyncio + async def test_quit_stops_script(self, cli, tmp_path, capsys): + attach(cli) + script = tmp_path / "s.mbro" + script.write_text("call echo value=before\nquit\ncall echo value=after\n") + assert await cli.run_commands([f"script run {script}", "call echo value=next"]) + + out = capsys.readouterr().err + assert "echo:'before'" in out + assert "after" not in out + assert "next" not in out + + @staticmethod + async def run_main(monkeypatch, *argv: str) -> int: + monkeypatch.setattr(sys, "argv", ["mbro", "--no-rich", *argv]) + try: + await main_async() + except SystemExit as e: + return e.code + return 0 + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "command, code", + [("help", 0), ("bogus", 1), ("status", 1), ("tools", 1), ('call echo {"value": 1}', 1)], + ) + async def test_exit_codes(self, command, code, tmp_path, monkeypatch): + monkeypatch.setenv("MAGG_PATH", str(tmp_path)) + assert await self.run_main(monkeypatch, "-n", command) == code + + @pytest.mark.asyncio + async def test_script_exit_code(self, tmp_path, monkeypatch): + monkeypatch.setenv("MAGG_PATH", str(tmp_path)) + assert await self.run_main(monkeypatch, "-X", str(tmp_path / "missing.mbro")) == 1 + + @pytest.mark.asyncio + async def test_stdin_is_non_interactive(self, tmp_path, monkeypatch): + monkeypatch.setenv("MAGG_PATH", str(tmp_path)) + with ( + patch("sys.stdin.read", return_value="help\n"), + patch.object(MCPBrowserCLI, "start", AsyncMock()) as start, + ): + assert await self.run_main(monkeypatch, "-") == 0 + start.assert_not_called() diff --git a/test/magg/test_registry_search.py b/test/magg/test_registry_search.py index a8d2197..50f2053 100644 --- a/test/magg/test_registry_search.py +++ b/test/magg/test_registry_search.py @@ -1,11 +1,5 @@ """Tests for the official MCP Registry search backend.""" -import json -from pathlib import Path - -import pytest - -import magg from magg.discovery.search import ToolSearchEngine SAMPLE_RESPONSE = { @@ -106,30 +100,3 @@ def test_install_command_types(self): ) assert cmd([]) is None assert cmd([{"registryType": "npm"}]) is None - - -class TestServerManifest: - """Test Magg's own server.json registry manifest.""" - - def test_server_json_consistency(self): - root = Path(__file__).parent.parent.parent - manifest = json.loads((root / "server.json").read_text()) - - assert manifest["name"] == "io.github.sitbon/magg" - assert manifest["repository"]["url"] == "https://github.com/sitbon/magg" - - package = manifest["packages"][0] - assert package["registryType"] == "pypi" - assert package["identifier"] == "magg" - # Top-level and package versions must stay in sync for publishing - assert package["version"] == manifest["version"] - assert package["transport"]["type"] == "stdio" - - def test_server_json_version_matches_package(self): - """Fail release PRs that bump pyproject.toml but forget server.json.""" - if magg.__version__ == "unknown": - pytest.skip("magg package metadata not available") - - root = Path(__file__).parent.parent.parent - manifest = json.loads((root / "server.json").read_text()) - assert manifest["version"] == magg.__version__ diff --git a/test/magg/test_server_robustness.py b/test/magg/test_server_robustness.py new file mode 100644 index 0000000..09e811b --- /dev/null +++ b/test/magg/test_server_robustness.py @@ -0,0 +1,403 @@ +"""Robustness tests: unmounting, hung backends, read-only mode, auth, signals, and related fixes.""" + +import asyncio +import json +import os +import select +import signal +import subprocess +import sys +import time +from pathlib import Path +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from fastmcp import Client +from pydantic import AnyUrl + +from magg.reload import ConfigChange, ServerChange +from magg.server.runner import MaggRunner +from magg.server.server import MaggServer +from magg.settings import ServerConfig + +BACKEND_CODE = ''' +import sys + +from fastmcp import FastMCP + +label = sys.argv[1] if len(sys.argv) > 1 else "default" +mcp = FastMCP("backend") + + +@mcp.tool +def greet(name: str) -> str: + """Greet someone.""" + print(f"backend stderr: greeting {name}", file=sys.stderr, flush=True) + return f"{label}: hello {name}" + + +if __name__ == "__main__": + mcp.run(show_banner=False) +''' + + +@pytest.fixture +def backend_script(tmp_path) -> Path: + path = tmp_path / "backend.py" + path.write_text(BACKEND_CODE) + return path + + +def backend_config(name: str, script: Path, label: str, prefix: str | None = None, **kwds) -> ServerConfig: + return ServerConfig( + name=name, source="test", command=sys.executable, args=[str(script), label], prefix=prefix, **kwds + ) + + +def write_config(path: Path, servers: dict[str, ServerConfig]): + data = { + "servers": { + name: server.model_dump(mode="json", exclude_none=True, exclude={"name"}) + for name, server in servers.items() + } + } + path.write_text(json.dumps(data)) + + +async def tool_names(client: Client) -> list[str]: + return [tool.name for tool in await client.list_tools()] + + +def response_output(result) -> dict: + return json.loads(result.content[0].text) + + +class TestUnmount: + """Unmounting removes a backend's provider from FastMCP.""" + + @pytest.mark.asyncio + async def test_disable_enable_remove(self, tmp_path, backend_script): + config_path = tmp_path / "config.json" + write_config(config_path, {"bk": backend_config("bk", backend_script, "one", prefix="bk")}) + server = MaggServer(config_path, enable_config_reload=False) + + async with server: + providers_before = len(server.mcp.providers) + + async with Client(server.mcp) as client: + assert (await tool_names(client)).count("bk_greet") == 1 + + result = response_output(await client.call_tool("magg_disable_server", {"name": "bk"})) + assert result["errors"] is None + assert "bk_greet" not in await tool_names(client) + assert len(server.mcp.providers) == providers_before - 1 + + result = response_output(await client.call_tool("magg_enable_server", {"name": "bk"})) + assert result["output"]["mounted"] is True + assert (await tool_names(client)).count("bk_greet") == 1 + assert len(server.mcp.providers) == providers_before + + greeting = await client.call_tool("bk_greet", {"name": "x"}) + assert greeting.content[0].text == "one: hello x" + + result = response_output(await client.call_tool("magg_remove_server", {"name": "bk"})) + assert result["errors"] is None + assert "bk_greet" not in await tool_names(client) + assert len(server.mcp.providers) == providers_before - 1 + + @pytest.mark.asyncio + async def test_update_applies_new_args_and_prefix(self, tmp_path, backend_script): + config_path = tmp_path / "config.json" + old = backend_config("bk", backend_script, "old", prefix="old") + write_config(config_path, {"bk": old}) + server = MaggServer(config_path, enable_config_reload=False) + + async with server: + async with Client(server.mcp) as client: + assert (await client.call_tool("old_greet", {"name": "x"})).content[0].text == "old: hello x" + + new = backend_config("bk", backend_script, "new", prefix="new") + config = server.config + change = ConfigChange( + old_config=config, + new_config=config, + server_changes=[ServerChange(name="bk", action="update", old_config=old, new_config=new)], + ) + await server.server_manager.handle_config_reload(change) + + names = await tool_names(client) + assert "old_greet" not in names + assert names.count("new_greet") == 1 + assert (await client.call_tool("new_greet", {"name": "x"})).content[0].text == "new: hello x" + + +class TestCheck: + """magg_check works on mounted servers without breaking them.""" + + @pytest.mark.asyncio + async def test_check_mounted_server(self, tmp_path, backend_script): + config_path = tmp_path / "config.json" + write_config(config_path, {"bk": backend_config("bk", backend_script, "one", prefix="bk")}) + server = MaggServer(config_path, enable_config_reload=False) + + async with server: + async with Client(server.mcp) as client: + assert (await client.call_tool("bk_greet", {"name": "a"})).content[0].text == "one: hello a" + + result = response_output(await client.call_tool("magg_check", {"timeout": 20})) + assert result["errors"] is None + assert result["output"]["results"]["bk"]["status"] == "healthy" + assert result["output"]["healthy"] == 1 + + # The mounted proxy still works after the check used the backend client + assert (await client.call_tool("bk_greet", {"name": "b"})).content[0].text == "one: hello b" + + +class TestHungBackend: + """A backend that never initializes degrades only itself.""" + + @pytest.mark.asyncio + async def test_hung_backend(self, tmp_path, backend_script, monkeypatch): + monkeypatch.setenv("MAGG_BACKEND_INIT_TIMEOUT", "2") + config_path = tmp_path / "config.json" + hung = ServerConfig( + name="hung", source="test", command=sys.executable, args=["-c", "import time; time.sleep(1000)"] + ) + write_config(config_path, {"hung": hung, "good": backend_config("good", backend_script, "g", prefix="good")}) + server = MaggServer(config_path, enable_config_reload=False) + + async with server: + async with Client(server.mcp) as client: + # Without the init timeout this waits forever, so bound it to fail instead of hang + start = time.monotonic() + names = await asyncio.wait_for(tool_names(client), 15) + assert time.monotonic() - start < 10 + assert "magg_list_servers" in names + assert "good_greet" in names + + # Magg's own tools resolve without waiting on the hung backend + start = time.monotonic() + result = response_output(await client.call_tool("magg_list_servers", {})) + assert time.monotonic() - start < 1.5 + assert {s["name"] for s in result["output"]} == {"hung", "good"} + + assert (await client.call_tool("good_greet", {"name": "x"})).content[0].text == "g: hello x" + + +class TestTransportSetup: + """Backend env precedence and stderr handling.""" + + @pytest.mark.asyncio + async def test_server_env_overrides_passed_env(self, tmp_path): + server = MaggServer(tmp_path / "config.json", enable_config_reload=False, env={"FOO": "outer", "BAR": "b"}) + config = ServerConfig(name="s", source="t", command="true", env={"FOO": "server"}) + assert await server.server_manager.mount_server(config) + env = server.server_manager.mounted_servers["s"].client.transport.env + assert env["FOO"] == "server" + assert env["BAR"] == "b" + await server.server_manager.unmount_server("s") + + @pytest.mark.asyncio + @pytest.mark.parametrize("show", [False, True]) + async def test_backend_stderr(self, tmp_path, backend_script, monkeypatch, capfd, show): + # capfd captures at the file descriptor level, which is where the backend process writes + monkeypatch.setenv("MAGG_STDERR_SHOW", str(show).lower()) + config_path = tmp_path / "config.json" + write_config(config_path, {"bk": backend_config("bk", backend_script, "one", prefix="bk")}) + server = MaggServer(config_path, enable_config_reload=False) + + async with server: + async with Client(server.mcp) as client: + assert (await client.call_tool("bk_greet", {"name": "x"})).content[0].text == "one: hello x" + + assert ("backend stderr: greeting x" in capfd.readouterr().err) == show + + +class TestAddServer: + """add_server input handling and rollback.""" + + @pytest.mark.asyncio + async def test_add_with_uri(self, tmp_path): + server = MaggServer(tmp_path / "config.json", enable_config_reload=False) + + async with Client(server.mcp) as client: + result = response_output( + await client.call_tool( + "magg_add_server", + {"name": "web", "source": "test", "uri": "http://localhost:9/mcp", "enable": False}, + ) + ) + assert result["errors"] is None + assert server.config.servers["web"].uri == "http://localhost:9/mcp" + + response = await server.add_server(name="web2", source="test", uri=AnyUrl("http://localhost:9/mcp")) + assert response.is_success + assert "web2" in server.server_manager.mounted_servers + + @pytest.mark.asyncio + async def test_add_requires_command_or_uri(self, tmp_path): + server = MaggServer(tmp_path / "config.json", enable_config_reload=False) + + for enable in (True, False): + response = await server.add_server(name="empty", source="test", enable=enable) + assert response.is_error + assert "provide either command or uri" in response.errors[0] + assert "empty" not in server.config.servers + + @pytest.mark.asyncio + async def test_mount_failure_reason(self, tmp_path): + server = MaggServer(tmp_path / "config.json", enable_config_reload=False) + + with patch("magg.server.manager.get_transport_for_command", side_effect=ValueError("no such tool")): + response = await server.add_server(name="bad", source="test", command="whatever") + + assert response.is_error + assert "no such tool" in response.errors[0] + assert "bad" not in server.config.servers + + @pytest.mark.asyncio + async def test_add_rolls_back_mount_when_save_fails(self, tmp_path): + server = MaggServer(tmp_path / "config.json", enable_config_reload=False) + providers_before = len(server.mcp.providers) + + with patch.object(server, "save_config", return_value=False): + response = await server.add_server(name="s", source="test", command="true") + + assert response.is_error + assert "s" not in server.server_manager.mounted_servers + assert len(server.mcp.providers) == providers_before + + +class TestReadOnly: + """Mutating tools refuse up front in read-only mode.""" + + @pytest.fixture + def server(self, tmp_path, monkeypatch): + config_path = tmp_path / "config.json" + write_config(config_path, {"s": ServerConfig(name="s", source="t", command="true", enabled=False)}) + monkeypatch.setenv("MAGG_READ_ONLY", "true") + return MaggServer(config_path, enable_config_reload=False) + + @pytest.mark.asyncio + async def test_mutating_tools_refuse(self, server): + server.server_manager.mount_server = AsyncMock(return_value=True) + server.server_manager.unmount_server = AsyncMock(return_value=True) + server.kit_manager.load_kit_to_config = MagicMock() + server.kit_manager.unload_kit_from_config = MagicMock() + + responses = [ + await server.add_server(name="new", source="test", command="true"), + await server.remove_server(name="s"), + await server.enable_server(name="s"), + await server.disable_server(name="s"), + await server.load_kit(name="k"), + await server.unload_kit(name="k"), + await server.check(action="disable"), + ] + + for response in responses: + assert response.is_error + assert "read-only" in response.errors[0] + + server.server_manager.mount_server.assert_not_called() + server.server_manager.unmount_server.assert_not_called() + server.kit_manager.load_kit_to_config.assert_not_called() + server.kit_manager.unload_kit_from_config.assert_not_called() + assert set(server.config.servers) == {"s"} + + @pytest.mark.asyncio + @pytest.mark.parametrize("action", ["report", "remount", "unmount"]) + async def test_check_actions_that_do_not_write_config(self, server, action): + # Only "disable" writes the config; the others just affect this process + response = await server.check(action=action) + assert response.is_success + + +class TestAuthFailClosed: + """An unloadable private key refuses HTTP instead of serving without auth.""" + + @pytest.mark.asyncio + async def test_bad_private_key(self, tmp_path, monkeypatch): + monkeypatch.setenv("MAGG_PRIVATE_KEY", "not a key") + runner = MaggRunner(tmp_path / "config.json") + server = runner.server + + assert "MAGG_PRIVATE_KEY" in server.server_manager.auth_error + assert server.mcp.auth is None + + with patch.object(server.server_manager, "mount_all_enabled", new_callable=AsyncMock) as mount_all: + for run in (runner.run_http, runner.run_hybrid, server.run_http, server.run_hybrid): + with pytest.raises(RuntimeError, match="MAGG_PRIVATE_KEY.*refusing to serve HTTP"): + await run() + mount_all.assert_not_called() + + +class TestRunnerSignals: + """Shutdown signals wake the loop, restore handlers, and escalate on repeat.""" + + @pytest.mark.skipif(not hasattr(signal, "SIGHUP"), reason="POSIX signals required") + @pytest.mark.asyncio + async def test_sigterm_wakes_idle_loop_and_restores_default(self, tmp_path): + runner = MaggRunner(tmp_path / "config.json") + + async def idle(): + await asyncio.Event().wait() + + previous = signal.signal(signal.SIGTERM, signal.SIG_DFL) + try: + async with runner._server_context(): + serve_task = asyncio.create_task(runner._serve(idle())) + await asyncio.sleep(0.1) + os.kill(os.getpid(), signal.SIGTERM) + await asyncio.wait_for(serve_task, 5) + + # SIG_DFL is falsy, but must still be restored + assert signal.getsignal(signal.SIGTERM) == signal.SIG_DFL + finally: + signal.signal(signal.SIGTERM, previous) + + def test_second_signal_forces_exit(self, tmp_path): + runner = MaggRunner(tmp_path / "config.json") + + with patch.object(MaggRunner, "_exit_by_signal") as exit_by_signal: + runner._handle_signal(signal.SIGINT) + assert runner._shutdown_event.is_set() + exit_by_signal.assert_not_called() + + runner._handle_signal(signal.SIGINT) + exit_by_signal.assert_called_once_with(signal.SIGINT) + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX signals required") + def test_stdio_serve_exits_on_sigterm(self, tmp_path): + env = {**os.environ, "MAGG_CONFIG_PATH": str(tmp_path / "config.json"), "MAGG_LOG_LEVEL": "WARNING"} + proc = subprocess.Popen( + [sys.executable, "-m", "magg", "serve"], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.DEVNULL, + env=env, + ) + try: + init = { + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "t", "version": "1"}, + }, + } + proc.stdin.write((json.dumps(init) + "\n").encode()) + proc.stdin.flush() + assert select.select([proc.stdout], [], [], 30)[0], "server did not respond" + assert b'"id":1' in proc.stdout.readline() # Server is up; stdin stays open + + proc.send_signal(signal.SIGTERM) + start = time.monotonic() + proc.wait(timeout=15) + assert time.monotonic() - start < 5 + finally: + if proc.poll() is None: + proc.kill() + proc.wait() diff --git a/uv.lock b/uv.lock index 0478d7e..8017ff4 100644 --- a/uv.lock +++ b/uv.lock @@ -902,7 +902,7 @@ wheels = [ [[package]] name = "magg" -version = "1.2.1" +version = "1.3.0" source = { editable = "." } dependencies = [ { name = "aiohttp" },