From 475f6b9920254b17a30c7241fece70a58ac6eb44 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Wed, 5 Aug 2026 11:20:04 +0300 Subject: [PATCH 1/2] fix(agent): render .mcp.json.template at startup so declared MCP servers exist (#2007) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent guide tells template authors that Trinity replaces `${VAR}` in `.mcp.json.template` with credential-store values. For a `github:` template nothing did. The backend renderer reads `.mcp.json` — not the `.template` — and is `local:`-only anyway, while a `github:` agent's files only exist after `startup.sh` clones the repo inside the container. So the only writer of `~/.mcp.json` was `inject_trinity_mcp_if_configured()`, and every declared server was silently absent: a freshly-seeded Cornelius ships three and runs with none. The renderer therefore lives in the container, invoked from `startup.sh` after both `.env` sources (the /generated-creds copy and the decrypt-and-inject fallback), so credentials are present on either path. Substitution is confined to `env` blocks. That is not a simplification — it is the only form `mcp_validator` accepts: a `${VAR}` in `args` is rejected as a shell metacharacter and `command` must be a literal allowlist entry, so substituting there is exactly the RCE-by-config class #590 closed. Three properties make it safe to run unattended on every boot: - refuse, never blank. A placeholder that cannot be resolved withholds that server with a named reason on stdout; it is not configured with `""`. Blanking is what makes a broken config look healthy (#1929, and what laundered the rejected config in #2006). - merge, never clobber. Servers are only added when absent, so the `trinity` entry and any owner edit survive every restart, and the result is order-independent with respect to `inject_trinity_mcp_if_configured()`. - validate individually, through the vendored `mcp_validator` (byte-identical to the backend copy, Invariant #5), so one bad server never costs the agent the good ones. The guide is corrected in both directions: `env`-only, the `${VAR:-default}` form, and what withholding looks like. Requires a base-image rebuild; existing agents pick it up on recreate. Closes #2007 Co-Authored-By: Claude Opus 5 (1M context) --- .../base-image/agent_server/mcp_template.py | 286 ++++++++ .../base-image/agent_server/mcp_validator.py | 628 ++++++++++++++++++ docker/base-image/startup.sh | 19 + docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md | 32 +- docs/memory/architecture.md | 2 + tests/registry.json | 13 + tests/unit/test_2007_mcp_template_render.py | 371 +++++++++++ 7 files changed, 1349 insertions(+), 2 deletions(-) create mode 100644 docker/base-image/agent_server/mcp_template.py create mode 100644 docker/base-image/agent_server/mcp_validator.py create mode 100644 tests/unit/test_2007_mcp_template_render.py diff --git a/docker/base-image/agent_server/mcp_template.py b/docker/base-image/agent_server/mcp_template.py new file mode 100644 index 000000000..6ef46bef3 --- /dev/null +++ b/docker/base-image/agent_server/mcp_template.py @@ -0,0 +1,286 @@ +"""Render `.mcp.json.template` into `.mcp.json` at container startup (#2007). + +`docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md` tells template authors that Trinity +replaces `${VAR}` in `.mcp.json.template` with values from the credential +store. For a `github:` template nothing did: the backend's renderer +(`template_service.generate_credential_files`) reads `.mcp.json` — not the +`.template` — and is `local:`-only anyway, while a `github:` agent's files only +exist **after** `startup.sh` clones the repo *inside* the container. So the sole +writer of `~/.mcp.json` on a `github:` agent was +`inject_trinity_mcp_if_configured()`, and every declared server was silently +absent. A freshly-seeded Cornelius shipped three servers and ran with none. + +This module is that missing renderer, and it runs in-container because that is +the only place the files exist. + +## What it substitutes, and why only there + +`${VAR}` is expanded **inside `env` blocks only**. That is not a simplification +— it is the only form `mcp_validator` accepts. A `${VAR}` in `args` is rejected +outright (a bare `$` is a shell metacharacter and args are never ref-stripped), +and `command` must be a literal allowlist entry, so substituting there is the +RCE-by-config class #590 closed: a credential value becoming the executed +command. The published guide's own example only shows `env` placeholders. + +## Refuse, never blank + +A `${VAR}` with no value is **not** blanked to `""`. Blanking is what makes a +broken config look healthy — #1929's defect, and the reason the deploy path's +laundering (#2006) hid a rejected config. A server whose placeholders cannot be +resolved is withheld with a named reason on stdout (captured by Vector), and +the rest are installed. + +## Merge, never clobber + +Servers are only **added** when absent from `~/.mcp.json`. An entry already +there — the `trinity` entry this agent's server injects, or one an owner edited +— is left exactly as it is. That makes this idempotent across restarts and +order-independent with respect to `inject_trinity_mcp_if_configured()`, which +may create the file before or after this runs. + +Every candidate is validated **individually** through the vendored +`mcp_validator` (byte-identical to the backend copy, Invariant #5) before it is +allowed in, so one bad server does not cost the agent the good ones. +""" + +from __future__ import annotations + +import json +import os +import re +import sys +from pathlib import Path +from typing import Dict, List, Optional, Tuple + +try: # package import (normal) … + from .mcp_validator import McpValidationError, validate_mcp_config +except ImportError: # pragma: no cover — … or standalone, for a script run + from mcp_validator import McpValidationError, validate_mcp_config # type: ignore + +AGENT_HOME = Path("/home/developer") +TEMPLATE_FILE = AGENT_HOME / ".mcp.json.template" +CONFIG_FILE = AGENT_HOME / ".mcp.json" +ENV_FILE = AGENT_HOME / ".env" + +# `${VAR}` and `${VAR:-default}`. The default form is honoured because template +# authors use it for optional paths; an unresolvable ref is still refused +# rather than blanked. +_REF_RE = re.compile(r"\$\{([A-Za-z_][A-Za-z0-9_]*)(?::-([^}]*))?\}") + +_ENV_KEY_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") + +# Both files are agent-writable; cap the reads so a runaway file cannot be +# loaded whole at every container start. +_MAX_BYTES = 1024 * 1024 + + +def _log(message: str) -> None: + """stdout, so it lands in the container log Vector captures.""" + print(f"[mcp-template] {message}", flush=True) + + +def parse_env_file(path: Path = ENV_FILE) -> Dict[str, str]: + """Read `.env` into a dict. Never raises. + + Mirrors the writer in `routers/credentials.py` (`KEY="value"`, embedded `"` + backslash-escaped) while tolerating the hand-written shapes: `export KEY=v`, + single quotes, bare values, comments, CRLF. + """ + try: + if not path.is_file() or path.stat().st_size > _MAX_BYTES: + return {} + raw = path.read_text(encoding="utf-8", errors="replace") + except OSError: + return {} + + out: Dict[str, str] = {} + for line in raw.splitlines(): + line = line.strip() + if not line or line.startswith("#") or "=" not in line: + continue + if line.startswith("export "): + line = line[len("export "):].lstrip() + key, _, value = line.partition("=") + key = key.strip() + if not _ENV_KEY_RE.match(key): + continue + value = value.strip() + if len(value) >= 2 and value[0] == value[-1] and value[0] in ('"', "'"): + inner = value[1:-1] + value = inner.replace('\\"', '"') if value[0] == '"' else inner + out[key] = value + return out + + +def _resolve_refs(value: str, values: Dict[str, str]) -> Tuple[Optional[str], Optional[str]]: + """Expand every `${VAR}` in `value`. Returns `(resolved, None)` or `(None, reason)`. + + An empty stored value counts as unset: a credential the operator has not + filled in yet is exactly the case that must not silently produce a server + configured with `""`. + """ + missing: List[str] = [] + + def sub(match: re.Match) -> str: + name, default = match.group(1), match.group(2) + resolved = values.get(name) or "" + if resolved: + return resolved + if default is not None: + return default + missing.append(name) + return "" + + expanded = _REF_RE.sub(sub, value) + if missing: + return None, f"unresolved placeholder(s): {', '.join(sorted(set(missing)))}" + return expanded, None + + +def render_server(config: dict, values: Dict[str, str]) -> Tuple[Optional[dict], Optional[str]]: + """Render ONE server entry. Returns `(rendered, None)` or `(None, reason)`. + + Substitution is confined to `env`. A `${...}` anywhere else is reported — + not expanded — because the validator would reject the expansion anyway and + a silent expansion into `command` is the #590 class. + """ + if not isinstance(config, dict): + return None, "entry is not an object" + + rendered = json.loads(json.dumps(config)) # deep copy, JSON-only by construction + + env = rendered.get("env") + if env is not None: + if not isinstance(env, dict): + return None, "'env' is not an object" + for key, value in list(env.items()): + if not isinstance(value, str): + continue + resolved, reason = _resolve_refs(value, values) + if reason: + return None, reason + env[key] = resolved + + for field in ("command", "url"): + value = rendered.get(field) + if isinstance(value, str) and _REF_RE.search(value): + return None, ( + f"'{field}' contains a ${{VAR}} placeholder; Trinity substitutes " + f"only inside 'env' (a credential value must never become the " + f"executed command)" + ) + args = rendered.get("args") + if isinstance(args, list): + for arg in args: + if isinstance(arg, str) and _REF_RE.search(arg): + return None, ( + "'args' contains a ${VAR} placeholder; Trinity substitutes " + "only inside 'env', and the validator rejects placeholders " + "in args" + ) + + try: + validate_mcp_config(json.dumps({"mcpServers": {"_probe": rendered}})) + except McpValidationError as e: + return None, str(e) + return rendered, None + + +def render( + template_file: Path = TEMPLATE_FILE, + config_file: Path = CONFIG_FILE, + env_file: Path = ENV_FILE, +) -> Dict[str, object]: + """Render the template and merge the result into `.mcp.json`. + + Never raises: this runs on the container startup path, where an exception + would cost the agent its boot over a malformed optional file. + """ + result: Dict[str, object] = {"added": [], "skipped": {}, "status": "ok"} + + try: + if not template_file.is_file(): + result["status"] = "no_template" + return result + if template_file.stat().st_size > _MAX_BYTES: + _log(f"{template_file.name} is too large; ignoring") + result["status"] = "template_too_large" + return result + template_raw = template_file.read_text(encoding="utf-8", errors="replace") + except OSError as e: + _log(f"could not read {template_file}: {e}") + result["status"] = "unreadable" + return result + + try: + template = json.loads(template_raw) + except ValueError as e: + _log(f"{template_file.name} is not valid JSON ({e}); no servers rendered") + result["status"] = "invalid_json" + return result + + declared = template.get("mcpServers") if isinstance(template, dict) else None + if not isinstance(declared, dict) or not declared: + result["status"] = "no_servers" + return result + + existing: dict = {} + if config_file.is_file(): + try: + loaded = json.loads(config_file.read_text(encoding="utf-8", errors="replace")) + if isinstance(loaded, dict): + existing = loaded + except (OSError, ValueError) as e: + # A .mcp.json we cannot parse is not ours to repair — and merging + # into it blind would destroy whatever is there. + _log(f"existing .mcp.json is unreadable/invalid ({e}); leaving it alone") + result["status"] = "existing_unreadable" + return result + + servers = existing.setdefault("mcpServers", {}) + if not isinstance(servers, dict): + _log("existing .mcp.json has a non-object 'mcpServers'; leaving it alone") + result["status"] = "existing_unreadable" + return result + + values = parse_env_file(env_file) + added: List[str] = [] + skipped: Dict[str, str] = {} + + for name, config in declared.items(): + if name in servers: + continue # never clobber an entry that is already installed + rendered, reason = render_server(config, values) + if reason: + skipped[name] = reason + continue + servers[name] = rendered + added.append(name) + + for name, reason in sorted(skipped.items()): + _log(f"withheld MCP server '{name}': {reason}") + + if added: + try: + tmp = config_file.with_name(config_file.name + ".tmp") + tmp.write_text(json.dumps(existing, indent=2) + "\n", encoding="utf-8") + os.chmod(tmp, 0o600) + os.replace(tmp, config_file) # atomic: never a half-written config + except OSError as e: + _log(f"could not write {config_file}: {e}") + result["status"] = "write_failed" + return result + _log(f"rendered MCP server(s) into .mcp.json: {', '.join(added)}") + + result["added"] = added + result["skipped"] = skipped + return result + + +def main() -> int: + render() + return 0 # startup must continue regardless + + +if __name__ == "__main__": # pragma: no cover — exercised via startup.sh + sys.exit(main()) diff --git a/docker/base-image/agent_server/mcp_validator.py b/docker/base-image/agent_server/mcp_validator.py new file mode 100644 index 000000000..c8fdbe86d --- /dev/null +++ b/docker/base-image/agent_server/mcp_validator.py @@ -0,0 +1,628 @@ +""" +MCP server config validator (#598, Layer 2 of AISEC-C2 closure). + +Re-allows `.mcp.json` content through `POST /api/agents/{name}/credentials/inject` +ONLY when the structure passes strict validation. Layer 1 (#590) closed the +RCE-by-config bypass by removing `.mcp.json` from the inject allowlist; this +module restores the legitimate use case (owners adding/editing MCP servers +post-deploy) while keeping the attack surface closed. + +Public API: + validate_mcp_config(content: str) -> None + Raises McpValidationError on any rejection. + + class McpValidationError(ValueError): + Distinct exception for the router to surface as 400 Bad Request. + +Threat model: + - Attacker is an authenticated agent OWNER (already has the JWT). + - Goal: defense in depth against shell-injection patterns and the + AISEC-C2 exact reproduction. Does NOT prevent owners from running + malicious code via approved runtimes (npx ) — that's + Layer 3 (sandbox MCP execution). + +Architecture (SOLID at appropriate scale — single file, internal classes): + validate_mcp_config() + └─ _validate_servers_dict() schema + per-entry dispatch + └─ _ENTRY_VALIDATORS_BY_TRANSPORT[transport].validate(name, server) + ├─ _StdioValidator command + args + env + ├─ _HttpValidator url + headers + env (+ SSRF) + └─ _SseValidator subclass of _HttpValidator (semantically + distinct, same rules) +""" +from __future__ import annotations + +import ipaddress +import json +import os +import re +import socket +from typing import Mapping +from urllib.parse import urlparse + + +# --------------------------------------------------------------------------- +# Public exception +# --------------------------------------------------------------------------- + + +class McpValidationError(ValueError): + """Raised when an MCP server config fails validation. + + Routers translate this to HTTP 400. The message is included verbatim in + the response, so it must be safe to surface to the caller (no internal + paths or stack traces). + """ + + +# --------------------------------------------------------------------------- +# Constants — tunable in one place +# --------------------------------------------------------------------------- + +# Maximum size of the rendered `.mcp.json` content. 64KB is ~10x what a +# realistic config needs and keeps validation O(n) bounded. +MAX_CONTENT_BYTES = 64 * 1024 + +# Maximum number of mcpServers entries. Real configs have <10; cap defends +# against pathological JSON. +MAX_SERVER_COUNT = 32 + +# Server name (the dict key under mcpServers): conservative ASCII rule. Long +# enough for realistic identifiers, short enough to be UI-safe. +_SERVER_NAME_RE = re.compile(r"^[a-zA-Z0-9_-]{1,64}$") + +# Reserved server names — owners cannot overwrite Trinity's auto-injected +# entry with a different SHAPE (which would break agent-to-agent +# collaboration or open an RCE-by-config vector). The trinity entry IS +# allowed through if its shape matches what the agent server's +# inject_trinity_mcp_if_configured() writes — this lets owners legitimately +# update the bearer token (e.g. rotating their MCP API key) without +# tripping the reserved-name reject. See _is_canonical_trinity_entry. +RESERVED_SERVER_NAMES = frozenset({"trinity"}) + +# Canonical Trinity-MCP entry shape (mirrors +# docker/base-image/agent_server/services/trinity_mcp.py:_inject_claude_mcp). +# url is whatever the agent's TRINITY_MCP_URL env points to; we accept both +# the runtime value (so admins can override it cluster-wide) and the +# documented default for fresh installs. +_TRINITY_DEFAULT_URL = "http://mcp-server:8080/mcp" +_TRINITY_BEARER_RE = re.compile(r"^Bearer\s+trinity_mcp_[A-Za-z0-9_-]{1,200}$") + + +def _is_canonical_trinity_entry(server: dict) -> bool: + """Return True iff ``server`` looks exactly like the Trinity-injected + entry: only ``type``, ``url``, ``headers`` keys; ``type=http``; ``url`` + matches the configured Trinity MCP URL; ``headers`` contains only + ``Authorization`` with a ``Bearer trinity_mcp_…`` token of the right + shape. + + Strict allowlist — any extra key, any wrong value, any extra header, + any non-bearer auth scheme is rejected. Owners who want to redefine + trinity with a different shape (e.g. as a stdio server) hit the + reserved-name reject; the canonical shape is the only escape. + """ + if set(server.keys()) != {"type", "url", "headers"}: + return False + if server.get("type") != "http": + return False + + url = server.get("url") + if not isinstance(url, str): + return False + expected_url = os.getenv("TRINITY_MCP_URL", _TRINITY_DEFAULT_URL) + if url not in (expected_url, _TRINITY_DEFAULT_URL): + return False + + headers = server.get("headers") + if not isinstance(headers, dict) or list(headers.keys()) != ["Authorization"]: + return False + auth = headers.get("Authorization") + if not isinstance(auth, str) or not _TRINITY_BEARER_RE.match(auth): + return False + + return True + +# Allowed values for the `transport` field. `stdio` is implicit when only +# `command` is present (we set transport explicitly during validation). +ALLOWED_TRANSPORTS = frozenset({"stdio", "http", "sse"}) + +# Stdio runtime allowlist. Each entry is the EXACT command name the user can +# specify; absolute paths and alternates are rejected. `python3` listed +# separately from `python` because both are real on different distros. +COMMAND_ALLOWLIST = frozenset({ + "npx", "uvx", "python", "python3", "node", "bun", "deno", "docker", +}) + +# Per-runtime "execution flags" that turn the runtime into a shell. We block +# these as the FIRST positional arg (which would replace the script/package +# arg with inline code). Owners with a real need (`-c "import …"`) should +# package as a script and reference it instead. +_INLINE_EXEC_FLAGS_BY_COMMAND: Mapping[str, frozenset[str]] = { + "python": frozenset({"-c", "--command"}), + "python3": frozenset({"-c", "--command"}), + "node": frozenset({"-e", "--eval", "-p", "--print"}), + "bun": frozenset({"-e", "--eval"}), + "deno": frozenset({"eval"}), + # npx / uvx don't have an inline-exec flag; the first positional IS the + # package name. Likewise docker. +} + +# Shell metacharacters that have no business in any arg passed to a runtime +# allowlisted above. Each runtime invokes its target via execve, not a shell, +# so these characters never need to appear unescaped in real configs. +_SHELL_METACHARS_RE = re.compile(r"[;&|<>`$\n\r\x00]") + +# Substring patterns that indicate command substitution. `re.search` finds +# them anywhere in the arg. +_COMMAND_SUBSTITUTION_RE = re.compile(r"\$\([^)]+\)|`[^`]+`") + +# Env var names allowed as `${VAR}` references in args/env values. ASCII +# uppercase + digits + underscore, must start with a letter — standard +# POSIX shape; rejects `${PATH}` at a separate gate. +_ENV_VAR_REF_RE = re.compile(r"^[A-Z][A-Z0-9_]*$") + +# Env vars the user must NOT reference — overriding any of these from an +# attacker-controlled `.mcp.json` could change library/binary loading, +# attach an interpreter, or hijack subsequent process launches. +RESERVED_ENV_REFS = frozenset({ + "PATH", "HOME", "USER", "SHELL", + "LD_PRELOAD", "LD_LIBRARY_PATH", "DYLD_INSERT_LIBRARIES", + "PYTHONPATH", "PYTHONSTARTUP", "PYTHONHOME", + "NODE_OPTIONS", "NODE_PATH", + # Trinity-internal — the agent server reads these on startup + "TRINITY_MCP_API_KEY", "TRINITY_MCP_URL", "ADMIN_PASSWORD", + "SECRET_KEY", "INTERNAL_API_SECRET", "CREDENTIAL_ENCRYPTION_KEY", + "CLAUDE_CODE_OAUTH_TOKEN", "ANTHROPIC_API_KEY", +}) + +# Patterns that look like raw secrets accidentally pasted into a config — +# reject with a clear message rather than silently accepting them. Mirrors +# `credential_patterns` in docker/base-image/hooks/guardrails-baseline.json. +_LITERAL_SECRET_PATTERNS = ( + re.compile(r"sk-ant-[a-zA-Z0-9_-]{20,}"), + re.compile(r"sk-ant-oat01-[a-zA-Z0-9_-]{20,}"), + re.compile(r"sk-(?:proj-)?[a-zA-Z0-9]{32,}"), + re.compile(r"ghp_[a-zA-Z0-9]{30,}"), + re.compile(r"github_pat_[a-zA-Z0-9_]{40,}"), + re.compile(r"AKIA[0-9A-Z]{16}"), + re.compile(r"xox[baprs]-[0-9a-zA-Z-]{20,}"), + re.compile(r"AIza[0-9A-Za-z_-]{35}"), +) + +# Header names allowed in http/sse server entries. Limit to the small set +# real servers actually use to avoid weird header smuggling vectors. +_ALLOWED_HEADER_NAMES = frozenset({ + "authorization", "x-api-key", "user-agent", "accept", "content-type", +}) + + +# --------------------------------------------------------------------------- +# Helpers (private) +# --------------------------------------------------------------------------- + + +def _is_printable_ascii(s: str) -> bool: + """Strict ASCII printable check (defeats Unicode lookalikes + null bytes).""" + return all(0x20 <= ord(c) <= 0x7E for c in s) + + +# `${VAR}` substring used to extract refs from values like `Bearer ${TOKEN}`. +# Any reference found must satisfy `_ENV_VAR_REF_RE` shape AND not be in +# `RESERVED_ENV_REFS`. The remaining literal portion (with refs stripped) +# is then checked for shell metacharacters. +_ENV_VAR_SUBSTRING_RE = re.compile(r"\$\{([^}]*)\}") + + +def _validate_env_value(server_name: str, key: str, value: object) -> None: + """Validate one env value. + + Allowed shapes (covers real-world cases like `Bearer ${API_TOKEN}`, + `${OPENAI_BASE_URL}/v1`, plain literal URLs, plain `${VAR}` refs): + - Any number of `${VAR}` substring references, each with a valid + var name that is NOT in RESERVED_ENV_REFS + - The remaining literal portion (refs stripped) is checked for shell + metacharacters and command substitution + + Reject: + - non-string values, oversize values + - command substitution patterns (`$(…)` or backticks) + - shell metacharacters in the literal portion + - literal secrets (defense against accidental paste) + - malformed or reserved `${VAR}` references + """ + if not isinstance(value, str): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] must be a string" + ) + if len(value) > 4096: + raise McpValidationError( + f"Server '{server_name}': env['{key}'] exceeds 4096 chars" + ) + + # Pull out every `${VAR}` reference; each must be safe. + refs = _ENV_VAR_SUBSTRING_RE.findall(value) + for var_name in refs: + if not _ENV_VAR_REF_RE.match(var_name): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] references malformed " + f"variable name '{var_name}'" + ) + if var_name in RESERVED_ENV_REFS: + raise McpValidationError( + f"Server '{server_name}': env['{key}'] references reserved " + f"variable '{var_name}'" + ) + + # Strip refs to get the literal portion, then apply shell-safety checks + # to it. This way `Bearer ${TOKEN}` validates as `Bearer ` (safe). + literal = _ENV_VAR_SUBSTRING_RE.sub("", value) + + if _COMMAND_SUBSTITUTION_RE.search(literal): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] contains command " + f"substitution" + ) + if _SHELL_METACHARS_RE.search(literal): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] contains shell " + f"metacharacters" + ) + for pat in _LITERAL_SECRET_PATTERNS: + if pat.search(literal): + raise McpValidationError( + f"Server '{server_name}': env['{key}'] looks like a literal " + f"secret — store it in .env and reference as ${{VAR}}" + ) + + +def _resolves_to_private_ip(hostname: str) -> bool: + """Best-effort DNS check (mirrors SEC-179 / #179). + + Returns True if the hostname resolves to ANY private/loopback/link-local/ + multicast IP. On DNS failure: True (fail closed). Used to block IMDS, + localhost, RFC 1918, and similar SSRF targets. + """ + try: + # getaddrinfo returns a list of (family, type, proto, canonname, sockaddr). + # We only need the IP from sockaddr. + infos = socket.getaddrinfo(hostname, None) + except socket.gaierror: + return True # fail closed + for info in infos: + sockaddr = info[4] + try: + ip = ipaddress.ip_address(sockaddr[0]) + except (ValueError, IndexError): + continue + if ( + ip.is_private + or ip.is_loopback + or ip.is_link_local + or ip.is_multicast + or ip.is_reserved + or ip.is_unspecified + ): + return True + return False + + +# --------------------------------------------------------------------------- +# Transport validators +# --------------------------------------------------------------------------- + + +class _StdioValidator: + """Stdio-transport server: command + args + env.""" + + @staticmethod + def validate(server_name: str, server: dict) -> None: + if "command" not in server: + raise McpValidationError( + f"Server '{server_name}' (stdio): missing required field 'command'" + ) + command = server["command"] + if not isinstance(command, str) or not command: + raise McpValidationError( + f"Server '{server_name}': command must be a non-empty string" + ) + if "/" in command or "\\" in command: + raise McpValidationError( + f"Server '{server_name}': command must be a name, not a path " + f"(got '{command}')" + ) + if not _is_printable_ascii(command): + raise McpValidationError( + f"Server '{server_name}': command contains non-ASCII or " + f"control characters" + ) + if command not in COMMAND_ALLOWLIST: + raise McpValidationError( + f"Server '{server_name}': command '{command}' not in allowlist. " + f"Allowed: {sorted(COMMAND_ALLOWLIST)}" + ) + + args = server.get("args", []) + if not isinstance(args, list): + raise McpValidationError( + f"Server '{server_name}': args must be a list" + ) + if len(args) > 64: + raise McpValidationError( + f"Server '{server_name}': args list too long (max 64)" + ) + + # Block the inline-exec flag as the FIRST positional. Later positions + # would be bare strings the runtime treats as data. + inline_flags = _INLINE_EXEC_FLAGS_BY_COMMAND.get(command, frozenset()) + for i, arg in enumerate(args): + if not isinstance(arg, str): + raise McpValidationError( + f"Server '{server_name}': args[{i}] must be a string" + ) + if len(arg) > 1024: + raise McpValidationError( + f"Server '{server_name}': args[{i}] exceeds 1024 chars" + ) + if "\x00" in arg: + raise McpValidationError( + f"Server '{server_name}': args[{i}] contains null byte" + ) + if _COMMAND_SUBSTITUTION_RE.search(arg): + raise McpValidationError( + f"Server '{server_name}': args[{i}] contains command " + f"substitution" + ) + if _SHELL_METACHARS_RE.search(arg): + raise McpValidationError( + f"Server '{server_name}': args[{i}] contains shell " + f"metacharacters" + ) + if i == 0 and arg in inline_flags: + raise McpValidationError( + f"Server '{server_name}': inline-exec flag '{arg}' not " + f"allowed; package the code as a script and reference its path" + ) + + env = server.get("env", {}) + if not isinstance(env, dict): + raise McpValidationError( + f"Server '{server_name}': env must be an object" + ) + if len(env) > 64: + raise McpValidationError( + f"Server '{server_name}': env has too many entries (max 64)" + ) + for key, value in env.items(): + if not isinstance(key, str) or not _ENV_VAR_REF_RE.match(key): + raise McpValidationError( + f"Server '{server_name}': env key '{key}' must match " + f"^[A-Z][A-Z0-9_]*$" + ) + _validate_env_value(server_name, key, value) + + +class _HttpValidator: + """HTTP-transport server: url + headers + env.""" + + transport_label = "http" + + @classmethod + def validate(cls, server_name: str, server: dict) -> None: + if "url" not in server: + raise McpValidationError( + f"Server '{server_name}' ({cls.transport_label}): " + f"missing required field 'url'" + ) + url = server["url"] + if not isinstance(url, str) or len(url) > 2048: + raise McpValidationError( + f"Server '{server_name}': url must be a string < 2048 chars" + ) + + try: + parsed = urlparse(url) + except ValueError as e: + raise McpValidationError( + f"Server '{server_name}': invalid url ({e})" + ) + + if parsed.scheme != "https": + raise McpValidationError( + f"Server '{server_name}': url must use https (got '{parsed.scheme}')" + ) + # Reject userinfo (`https://user:pass@evil.com/...`) which can confuse + # display-only auditors and is never needed for MCP. + if "@" in (parsed.netloc or ""): + raise McpValidationError( + f"Server '{server_name}': url must not contain userinfo (@)" + ) + hostname = (parsed.hostname or "").lower() + if not hostname: + raise McpValidationError( + f"Server '{server_name}': url missing hostname" + ) + if not _is_printable_ascii(hostname): + raise McpValidationError( + f"Server '{server_name}': url hostname contains non-ASCII " + f"(possible homograph)" + ) + if _resolves_to_private_ip(hostname): + raise McpValidationError( + f"Server '{server_name}': url hostname '{hostname}' resolves " + f"to a private/loopback/link-local address (SSRF guard)" + ) + + headers = server.get("headers", {}) + if not isinstance(headers, dict): + raise McpValidationError( + f"Server '{server_name}': headers must be an object" + ) + if len(headers) > 16: + raise McpValidationError( + f"Server '{server_name}': headers has too many entries (max 16)" + ) + for key, value in headers.items(): + if not isinstance(key, str): + raise McpValidationError( + f"Server '{server_name}': header name must be a string" + ) + if key.lower() not in _ALLOWED_HEADER_NAMES: + raise McpValidationError( + f"Server '{server_name}': header '{key}' not in allowlist. " + f"Allowed: {sorted(_ALLOWED_HEADER_NAMES)}" + ) + # Header values reuse the env-value rules: ${VAR} or safe literal. + _validate_env_value(server_name, f"headers.{key}", value) + + # http/sse can also carry env (rare but allowed by the MCP spec). + env = server.get("env", {}) + if not isinstance(env, dict): + raise McpValidationError( + f"Server '{server_name}': env must be an object" + ) + for key, value in env.items(): + if not isinstance(key, str) or not _ENV_VAR_REF_RE.match(key): + raise McpValidationError( + f"Server '{server_name}': env key '{key}' must match " + f"^[A-Z][A-Z0-9_]*$" + ) + _validate_env_value(server_name, key, value) + + +class _SseValidator(_HttpValidator): + """SSE-transport server. Identical rules to HTTP; separate class for + diagnostic clarity in error messages. + """ + transport_label = "sse" + + +# Dispatch table — Open-Closed: add a new transport by adding a class and +# one entry here, no edits elsewhere. +_ENTRY_VALIDATORS_BY_TRANSPORT = { + "stdio": _StdioValidator, + "http": _HttpValidator, + "sse": _SseValidator, +} + + +# --------------------------------------------------------------------------- +# Entry / config orchestration +# --------------------------------------------------------------------------- + + +def _resolve_transport(server_name: str, server: dict) -> str: + """Determine the transport for an entry. + + The MCP config spec lets transport be implicit: + - stdio: presence of `command` + - http/sse: presence of `url` + explicit `type` field + We require the `type` field for http/sse to avoid ambiguity, and accept + `command` as an implicit stdio signal. + """ + explicit = server.get("type") + if explicit is not None: + if not isinstance(explicit, str) or explicit not in ALLOWED_TRANSPORTS: + raise McpValidationError( + f"Server '{server_name}': type must be one of " + f"{sorted(ALLOWED_TRANSPORTS)}" + ) + return explicit + # No explicit type → infer + if "command" in server: + return "stdio" + if "url" in server: + raise McpValidationError( + f"Server '{server_name}': url provided without 'type' field; " + f"set type to 'http' or 'sse'" + ) + raise McpValidationError( + f"Server '{server_name}': cannot determine transport (no command, " + f"no url, no type)" + ) + + +def _validate_entry(server_name: str, server: object) -> None: + """Validate a single MCP server entry.""" + if not isinstance(server_name, str): + raise McpValidationError("MCP server name must be a string") + if not _SERVER_NAME_RE.match(server_name): + raise McpValidationError( + f"MCP server name '{server_name}' invalid; must match " + f"^[a-zA-Z0-9_-]{{1,64}}$" + ) + if not isinstance(server, dict): + raise McpValidationError( + f"MCP server '{server_name}' must be a JSON object" + ) + if server_name in RESERVED_SERVER_NAMES: + # Allow the reserved trinity entry through ONLY if its shape + # matches the agent server's auto-inject — owners legitimately + # need to edit the bearer token to rotate their MCP API key. + # Any other shape (different url, extra headers, redefined as + # stdio, etc.) hits the reserved-name reject. + if server_name == "trinity" and _is_canonical_trinity_entry(server): + return + raise McpValidationError( + f"MCP server name '{server_name}' is reserved by Trinity" + ) + + transport = _resolve_transport(server_name, server) + validator = _ENTRY_VALIDATORS_BY_TRANSPORT[transport] + validator.validate(server_name, server) + + # Reject any unknown top-level fields. Closed schema = no surprise fields + # that future MCP versions might interpret in unexpected ways. + allowed_keys = {"command", "args", "env", "url", "headers", "type"} + extra = set(server.keys()) - allowed_keys + if extra: + raise McpValidationError( + f"Server '{server_name}': unknown field(s) {sorted(extra)}; " + f"allowed: {sorted(allowed_keys)}" + ) + + +def _validate_servers_dict(servers: dict) -> None: + """Validate the top-level mcpServers dict.""" + if len(servers) > MAX_SERVER_COUNT: + raise McpValidationError( + f"Too many MCP servers ({len(servers)}); max {MAX_SERVER_COUNT}" + ) + for name, entry in servers.items(): + _validate_entry(name, entry) + + +def validate_mcp_config(content: str) -> None: + """Validate the rendered `.mcp.json` content string. + + Raises McpValidationError with a single human-readable message on the + first failure. Routers should surface the message in the HTTP 400 body. + """ + if not isinstance(content, str): + raise McpValidationError(".mcp.json content must be a string") + if len(content.encode("utf-8")) > MAX_CONTENT_BYTES: + raise McpValidationError( + f".mcp.json content exceeds {MAX_CONTENT_BYTES} bytes" + ) + + try: + config = json.loads(content) + except json.JSONDecodeError as e: + raise McpValidationError(f".mcp.json is not valid JSON: {e.msg}") + + if not isinstance(config, dict): + raise McpValidationError(".mcp.json root must be a JSON object") + + # Allow only `mcpServers` at the root. Other top-level keys (e.g. legacy + # `inputs`, future spec fields) are rejected to keep the schema closed. + extra_root = set(config.keys()) - {"mcpServers"} + if extra_root: + raise McpValidationError( + f".mcp.json has unknown top-level field(s) {sorted(extra_root)}; " + f"only 'mcpServers' is allowed" + ) + + servers = config.get("mcpServers", {}) + if not isinstance(servers, dict): + raise McpValidationError(".mcp.json mcpServers must be an object") + + _validate_servers_dict(servers) diff --git a/docker/base-image/startup.sh b/docker/base-image/startup.sh index 00b70cd09..eb6946ffb 100644 --- a/docker/base-image/startup.sh +++ b/docker/base-image/startup.sh @@ -551,6 +551,25 @@ if [ -f ".credentials.enc" ] && [ ! -f ".env" ]; then fi fi +# === Render .mcp.json.template (#2007) === +# The agent guide promises Trinity substitutes ${VAR} in `.mcp.json.template`. +# For a `github:` template nothing did it: the backend renderer is `local:`-only +# and reads `.mcp.json`, not the `.template`, and a github: agent's files only +# exist after the clone above — in here. So declared MCP servers were silently +# absent (a freshly-seeded Cornelius shipped three and ran with none). +# +# Runs AFTER both `.env` sources (the /generated-creds copy and the +# decrypt-and-inject fallback above) so credentials are present either way, and +# merges rather than overwrites — the agent server's own `trinity` entry may be +# written before or after this point. Substitution is confined to `env` blocks, +# each rendered server is validated individually, and an unresolvable +# placeholder withholds that one server with a reason on stdout instead of +# blanking it. Never fails startup. +if [ -f "/home/developer/.mcp.json.template" ]; then + (cd /app && python3 -m agent_server.mcp_template) || \ + echo "Warning: .mcp.json.template rendering failed (continuing startup)" +fi + # === Content Folder Convention === # Create content/ directory for large generated assets (videos, audio, images, exports) # These files persist across restarts but are NOT synced to GitHub diff --git a/docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md b/docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md index 0f1737a9a..6e6be52aa 100644 --- a/docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md +++ b/docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md @@ -90,7 +90,7 @@ See [CLAUDE.md Requirements](#claudemd-requirements) for guidelines. ### 3. `.mcp.json.template` (Required if using MCP servers) -MCP server configuration with credential placeholders. Trinity replaces `${VAR}` with actual values from the credential store. +MCP server configuration with credential placeholders. At container startup Trinity renders this file into `.mcp.json`, replacing `${VAR}` with values from the credential store (#2007). ```json { @@ -108,10 +108,38 @@ MCP server configuration with credential placeholders. Trinity replaces `${VAR}` ``` **Important:** -- Use `${VAR_NAME}` syntax for credential placeholders +- Use `${VAR_NAME}` syntax for credential placeholders — **inside `env` blocks only** +- `${VAR:-default}` is also supported (the default is used when the credential is unset) - Never commit actual secrets - Server names must match `credentials.mcp_servers` keys in `template.yaml` +**Where substitution happens, and what happens when it can't** + +Trinity substitutes **only inside `env`**. A `${VAR}` in `args` or `command` is +**not** expanded, because the MCP config validator rejects both: a bare `$` in +`args` reads as a shell metacharacter, and `command` must be a literal entry +from the runtime allowlist (`npx`, `uvx`, `python`, `python3`, `node`, `bun`, +`deno`, `docker`). Letting a credential value become the executed command is the +config-injection class Trinity closed deliberately — so if you need a path, +pass it through `env` and read it in your server, or use `uvx ` rather +than an absolute interpreter path. + +Rendering is **merge-only and refuse-on-doubt**: + +- A server already present in `.mcp.json` is left untouched — including the + `trinity` entry Trinity injects, and anything you edited by hand. Re-running + is a no-op, so a restart never reverts your changes. +- A server whose placeholders cannot be resolved (no such credential, or the + value is empty) is **withheld**, not configured with a blank value. The + reason is logged to the agent's container output, one line per withheld + server. +- A server the validator rejects is withheld the same way, with the validator's + own reason — so `"command": "uv"` tells you `uv` is not in the allowlist + rather than failing later at exec time. + +The rest of the servers install normally: one bad entry never costs you the +good ones. + ### 4. `.env.example` (Recommended) Documents all required environment variables. Helps users understand what credentials are needed. diff --git a/docs/memory/architecture.md b/docs/memory/architecture.md index ff72b7e57..34ededeaf 100644 --- a/docs/memory/architecture.md +++ b/docs/memory/architecture.md @@ -338,6 +338,8 @@ The agent server also runs two loops: the 15-min git `auto_sync` heartbeat (see **Durable subscription-token override (#1089):** `startup.sh` exports `CLAUDE_CODE_OAUTH_TOKEN` from `/var/lib/trinity/oauth-token` (when present, non-empty) **before** launching the agent server, so a token rotated via hot-reload survives any plain stop+start of the same container (historically that included `routers/ops.py`'s fleet restart, which bypassed `start_agent_internal` entirely; since #1860 a fleet restart routes through `lifecycle.restart_agent_internal` — a no-drift agent keeps its container and the override survives, a drifted agent is recreated and cleanly re-bakes `Config.Env` from the DB). The path is deliberately on the writable layer, **not** under the persisted `/home/developer` volume: it survives `stop`→`start` (same container) but is wiped on recreate (fresh layer), so a DB-driven recreate cleanly re-bakes `Config.Env` from the DB and the stale override is gone — self-reconciling, no marker logic. Dir created+chowned to UID 1000 in the base-image Dockerfile. +**`.mcp.json.template` rendering (#2007):** `startup.sh` runs `agent_server/mcp_template.py` after the credential-import steps — the missing implementation of the contract `docs/TRINITY_COMPATIBLE_AGENT_GUIDE.md` publishes. It renders `~/.mcp.json.template` into `~/.mcp.json`, substituting `${VAR}` / `${VAR:-default}` from `.env` **inside `env` blocks only** (the only form `mcp_validator` accepts — a `${VAR}` in `args` is rejected as a shell metacharacter and `command` must be a literal allowlist entry, so substituting there is the #590 RCE-by-config class). It lives in-container because that is where the files are: a `github:` agent's template is cloned by `startup.sh`, so the backend renderer (`template_service.generate_credential_files`, `local:`-only and reads `.mcp.json` not the `.template`) never saw it and every declared server was silently absent. Each candidate server is validated individually through the **vendored** `mcp_validator` (byte-identical to the backend copy, Invariant #5); a server whose placeholders don't resolve, or that the validator rejects, is **withheld with a named reason on stdout** rather than blanked (the #1929 contract), and the rest still install. Merge-only-missing, so the `trinity` entry and any owner edit survive; idempotent across restarts and order-independent with respect to `inject_trinity_mcp_if_configured()`. Never fails the boot. + **Template-supplied pre-check** (SCHED-COND-001, #454): if the template ships an executable `~/.trinity/pre-check`, the backend's internal endpoint `POST /api/internal/agents/{name}/pre-check` runs it via `docker exec` before a cron-triggered chat. Language-agnostic — interpreter selected by shebang. The hook's stdout becomes the chat message; empty stdout + exit 0 records a skipped execution (Claude never invoked). Uses the same `execute_command_in_container` primitive as `git_service.py`, `ssh_service.py`, and the agent terminal — no agent-server HTTP endpoint. **Persistent chat:** all chat messages auto-saved to SQLite (`chat_sessions`, `chat_messages`) with full observability (costs, context, tool calls, execution time); sessions survive container restarts/deletions; users see only their own messages (admins see all). diff --git a/tests/registry.json b/tests/registry.json index eb177c29f..8fd9e5dbe 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -1553,6 +1553,19 @@ "parity" ], "description": "The agent server was outside ent#314's YAML sweep (#1965). utils/safe_yaml.py (PR #1961) put every author-controlled YAML reader in the backend behind one hardened loader, and its AST guard walks the whole backend with an EMPTY allowlist - but it walked _BACKEND.rglob only, so docker/base-image/agent_server/ kept six bare yaml.safe_load calls on documents the backend itself assigns REJECT: template.yaml (x2, credential_requirements_service), skill frontmatter (skill_packaging), dashboard.yaml (compatibility/static_checks) and .trinity/persistent-state.yaml. The vector is amplification at SERIALIZATION, not parse - a 416 B level-6 anchor bomb resolves in ~0.001 s and blows up to ~110 MB when something walks the graph - and the backend proxies /info and /dashboard, so the walk happens in-container then again across the wire. Covers: byte-parity of the vendored loader (the credential_paths.py shape, Invariant #5) plus proof the vendored COPY actually behaves - refuses a level-6 bomb under BUDGET, any alias under REJECT, duplicate keys, and still parses an honest document (byte parity is not behaviour parity if the file never imports); each of the four agent-authored sites on the shared loader with its backend counterpart's kind AND policy; /config/agent-config.yaml deliberately BUDGET not REJECT, stated as an exception because the platform writes it and bind-mounts it mode:'ro' so the agent cannot author it, and yaml.dump emits an anchor for any shared object reference - REJECT there would be a self-inflicted outage for no security; no bare safe_load left anywhere in the tree; and HardenedYamlError named in the except arms that previously caught only yaml.YAMLError (it is a ValueError, so without its own arm a refused bomb escapes to the generic handler and surfaces as the unnamed 500 the AC rules out - the trap static_checks._parse_yaml records backend-side). AC #4 end-to-end: a level-6 bomb in a container's template.yaml is refused by BOTH template.yaml readers with the expanded graph never reaching the response, an honest template still serves, and the metrics assertion checks the NAMED refusal rather than has_metrics:False - a bomb parses fine under bare safe_load and yields no metrics: key, so the flag alone passes against the very tree this issue reports. The AST-guard widening itself lives in test_ent314_hardened_yaml.py (both trees, still empty allowlist) rather than here, because splitting a guard across two files is how the second copy stops being run." + }, + { + "file": "unit/test_2007_mcp_template_render.py", + "feature": "#2007", + "added": "2026-08-05", + "categories": [ + "agent-server", + "unit", + "security", + "credentials", + "parity" + ], + "description": "TRINITY_COMPATIBLE_AGENT_GUIDE.md promised Trinity replaces ${VAR} in .mcp.json.template with credential-store values; for a github: template nothing did. The backend renderer (template_service.generate_credential_files) reads .mcp.json - not the .template - and is local:-only, while a github: agent's files only exist AFTER startup.sh clones the repo inside the container, so the only writer of ~/.mcp.json was inject_trinity_mcp_if_configured() and every declared server was silently absent (a freshly-seeded Cornelius shipped three and ran with none). Fix: agent_server/mcp_template.py, invoked by startup.sh after both .env sources (the /generated-creds copy and the decrypt-and-inject fallback). Substitution is confined to env blocks - the only form mcp_validator accepts, since ${VAR} in args reads as a shell metachar and command must be a literal allowlist entry, making substitution there the #590 RCE-by-config class. Unresolvable placeholders WITHHOLD the server with a named reason on stdout instead of blanking to \"\" (the #1929 contract); merge-only-missing keeps the trinity entry and any owner edit across restarts; each candidate is validated individually through the vendored validator so one bad server never costs the good ones. Covers: byte-parity of the vendored mcp_validator (Invariant #5), the Cornelius trio installing two and withholding ebook-mcp with the validator's own 'uv not in allowlist' reason, ${VAR:-default}, empty-value-counts-as-unset, placeholder-in-command asserted against the renderer's OWN named reason (a withheld-only assertion passes with the guard deleted, because the validator refuses the literal too - and would tell the operator the wrong thing to fix) plus an independent 'the credential value never appears in the output' property, idempotence, unparseable-existing-config left alone, every malformed-template shape degrading quietly, main() always returning 0, and two static wiring checks (startup.sh invokes it, and does so after the credential import)." } ] } diff --git a/tests/unit/test_2007_mcp_template_render.py b/tests/unit/test_2007_mcp_template_render.py new file mode 100644 index 000000000..356a00dda --- /dev/null +++ b/tests/unit/test_2007_mcp_template_render.py @@ -0,0 +1,371 @@ +"""#2007 — `github:` templates never rendered `.mcp.json.template`. + +`TRINITY_COMPATIBLE_AGENT_GUIDE.md` promised Trinity replaces `${VAR}` in +`.mcp.json.template` with credential-store values. Nothing did, for a `github:` +template: the backend renderer (`template_service.generate_credential_files`) +reads `.mcp.json` — not the `.template` — and is `local:`-only, while a +`github:` agent's files only exist after `startup.sh` clones the repo *inside* +the container. The only writer of `~/.mcp.json` was +`inject_trinity_mcp_if_configured()`, so every declared server was silently +absent — a freshly-seeded Cornelius shipped three servers and ran with none. + +The renderer therefore lives in the container. These tests pin the four +properties that make it safe to run unattended on every boot: + + 1. substitution is confined to `env` — the only form `mcp_validator` accepts, + and the reason a `${VAR}` must never reach `command` (#590); + 2. an unresolvable placeholder **withholds** the server with a reason, it + never blanks to `""` (#1929's defect, and what hid the #2006 residue); + 3. merge-only-missing, so an entry already installed — `trinity`, or one an + owner edited — survives every restart; + 4. one bad server never costs the agent the good ones. + +Loaded standalone by path: the agent server ships in its own image and cannot +import `src/backend` (the `test_1965_agent_server_safe_yaml.py` idiom). +""" + +from __future__ import annotations + +import importlib.util +import json +import sys +from pathlib import Path + +import pytest + +_ROOT = Path(__file__).resolve().parents[2] +_AGENT_SERVER = _ROOT / "docker" / "base-image" / "agent_server" +_MODULE = _AGENT_SERVER / "mcp_template.py" +_CANON_VALIDATOR = _ROOT / "src" / "backend" / "services" / "mcp_validator.py" +_VENDORED_VALIDATOR = _AGENT_SERVER / "mcp_validator.py" + +pytestmark = pytest.mark.unit + + +@pytest.fixture(scope="module") +def mod(): + """Import the renderer with its vendored validator resolvable. + + The module prefers the package-relative import and falls back to a flat + one; the fallback is what a `python3 -m` run inside the image exercises, so + the vendored validator is put on `sys.path` here rather than stubbed. + """ + sys.path.insert(0, str(_AGENT_SERVER)) + try: + spec = importlib.util.spec_from_file_location("_mcp_template_2007", _MODULE) + m = importlib.util.module_from_spec(spec) + spec.loader.exec_module(m) + return m + finally: + sys.path.remove(str(_AGENT_SERVER)) + + +@pytest.fixture +def home(tmp_path): + return tmp_path + + +def _paths(home): + return home / ".mcp.json.template", home / ".mcp.json", home / ".env" + + +def _render(mod, home, template: dict | str, env: str = "", existing=None): + tpl, cfg, envf = _paths(home) + tpl.write_text(template if isinstance(template, str) else json.dumps(template)) + envf.write_text(env) + if existing is not None: + cfg.write_text(json.dumps(existing)) + result = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + written = json.loads(cfg.read_text()) if cfg.exists() else None + return result, written + + +# --------------------------------------------------------------------------- +# Vendored-mirror parity (Invariant #5) +# --------------------------------------------------------------------------- + +def test_validator_copies_are_byte_identical(): + """The container validates with the SAME rules as the backend, or the two + surfaces accept different configs for the same file.""" + assert _VENDORED_VALIDATOR.exists(), "mcp_validator is not vendored (#2007)" + assert _CANON_VALIDATOR.read_bytes() == _VENDORED_VALIDATOR.read_bytes(), ( + "mcp_validator.py drifted between backend and agent-server — re-copy " + "the canonical file over the vendored one." + ) + + +# --------------------------------------------------------------------------- +# The reported bug +# --------------------------------------------------------------------------- + +class TestDeclaredServersAppear: + + def test_a_declared_server_reaches_mcp_json(self, mod, home): + """AC #1 — the whole point: a github: template's declared server is + actually configured.""" + result, written = _render( + mod, + home, + {"mcpServers": {"mermaid-diagram": { + "command": "npx", "args": ["-y", "@peng-shawn/mermaid-mcp-server"]}}}, + ) + assert result["added"] == ["mermaid-diagram"] + assert written["mcpServers"]["mermaid-diagram"]["command"] == "npx" + + def test_env_placeholder_is_substituted_from_the_credential_store(self, mod, home): + """Cornelius's `aistudio` server, verbatim.""" + result, written = _render( + mod, + home, + {"mcpServers": {"aistudio": { + "command": "npx", "args": ["-y", "aistudio-mcp-server"], + "env": {"GEMINI_API_KEY": "${GEMINI_API_KEY}"}}}}, + env='GEMINI_API_KEY="real-key-value"\n', + ) + assert result["added"] == ["aistudio"] + assert written["mcpServers"]["aistudio"]["env"] == { + "GEMINI_API_KEY": "real-key-value" + } + + def test_default_form_is_honoured(self, mod, home): + result, written = _render( + mod, home, + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "pkg"], + "env": {"P": "${MISSING:-/opt/fallback}"}}}}, + ) + assert result["added"] == ["x"] + assert written["mcpServers"]["x"]["env"]["P"] == "/opt/fallback" + + def test_output_passes_the_real_validator(self, mod, home): + """AC #2 — whatever is written must be a config the platform accepts.""" + from services.mcp_validator import validate_mcp_config + + _, written = _render( + mod, home, + {"mcpServers": {"ok": {"command": "python3", "args": ["-m", "srv"], + "env": {"K": "${K}"}}}}, + env="K=v\n", + ) + validate_mcp_config(json.dumps(written)) # raises on rejection + + +# --------------------------------------------------------------------------- +# Refuse, never blank (AC #3 — the shared contract with #1929) +# --------------------------------------------------------------------------- + +class TestRefuseNeverBlank: + + def test_unresolved_placeholder_withholds_the_server_with_a_reason(self, mod, home): + result, written = _render( + mod, home, + {"mcpServers": {"aistudio": {"command": "npx", "args": ["-y", "p"], + "env": {"GEMINI_API_KEY": "${GEMINI_API_KEY}"}}}}, + env="", + ) + assert result["added"] == [] + assert "GEMINI_API_KEY" in result["skipped"]["aistudio"] + assert written is None, "nothing should be written when nothing rendered" + + def test_an_empty_credential_counts_as_unset(self, mod, home): + """A key present but blank is the operator-hasn't-filled-it-in case — + exactly the one that must not silently produce env={'K': ''}.""" + result, _ = _render( + mod, home, + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "p"], + "env": {"K": "${K}"}}}}, + env='K=""\n', + ) + assert result["added"] == [] + assert "K" in result["skipped"]["x"] + + def test_placeholder_in_command_is_refused_not_substituted(self, mod, home): + """The #590 class: a credential value becoming the executed command. + `/api/credentials/update`'s whole-text `str.replace` did exactly this + (#2008); this renderer must not. + + Asserted against the renderer's OWN named reason, not merely "the + server was withheld": an unsubstituted `${SHELL_PATH}` is also refused + by the validator (not in COMMAND_ALLOWLIST), so a withheld-only + assertion passes with the explicit guard deleted and tells the operator + the wrong thing to fix. To be honest about what protects what: the + boundary is "never substitute outside `env`" plus the validator; this + guard exists to name the actual cause. + """ + result, written = _render( + mod, home, + {"mcpServers": {"evil": {"command": "${SHELL_PATH}", "args": []}}}, + env='SHELL_PATH="/bin/sh -c whatever"\n', + ) + assert result["added"] == [] + assert "Trinity substitutes" in result["skipped"]["evil"], ( + "expected the named placeholder-in-command reason, got: " + f"{result['skipped']['evil']}" + ) + assert written is None + + def test_a_substituted_command_never_appears_in_the_output(self, mod, home): + """The property that actually matters, stated independently of the + error text: whatever happens, the credential value must not end up as + the executed command.""" + _, written = _render( + mod, home, + {"mcpServers": { + "evil": {"command": "${SHELL_PATH}", "args": []}, + "ok": {"command": "npx", "args": ["-y", "p"]}, + }}, + env='SHELL_PATH="/bin/sh -c whatever"\n', + ) + assert "/bin/sh" not in json.dumps(written) + + def test_placeholder_in_args_is_refused(self, mod, home): + """Cornelius's `ebook-mcp` shape. The validator rejects `${VAR}` in args + (bare `$` is a shell metachar), so expanding there could only produce a + config the platform refuses.""" + result, _ = _render( + mod, home, + {"mcpServers": {"ebook-mcp": { + "command": "uvx", + "args": ["--directory", "${EBOOK_MCP_PATH:-/opt/mcp/ebook-mcp}", + "run", "ebook-mcp"]}}}, + env="", + ) + assert result["added"] == [] + assert "args" in result["skipped"]["ebook-mcp"] + + def test_a_command_outside_the_allowlist_is_withheld_with_the_validator_reason( + self, mod, home + ): + """Cornelius's real `ebook-mcp` uses `uv`, which is not in + COMMAND_ALLOWLIST — the withholding reason is the validator's own, so + the operator is told what to change (`uvx`).""" + result, _ = _render( + mod, home, + {"mcpServers": {"ebook-mcp": {"command": "uv", "args": ["run", "x"]}}}, + ) + assert result["added"] == [] + assert "allowlist" in result["skipped"]["ebook-mcp"] + + def test_one_bad_server_does_not_cost_the_good_ones(self, mod, home): + """AC #5's shape: the Cornelius trio — two install, one is withheld + with a documented reason.""" + result, written = _render( + mod, home, + {"mcpServers": { + "mermaid-diagram": {"command": "npx", "args": ["-y", "mermaid"]}, + "aistudio": {"command": "npx", "args": ["-y", "aistudio"], + "env": {"GEMINI_API_KEY": "${GEMINI_API_KEY}"}}, + "ebook-mcp": {"command": "uv", "args": ["run", "ebook-mcp"]}, + }}, + env="GEMINI_API_KEY=k\n", + ) + assert sorted(result["added"]) == ["aistudio", "mermaid-diagram"] + assert list(result["skipped"]) == ["ebook-mcp"] + assert sorted(written["mcpServers"]) == ["aistudio", "mermaid-diagram"] + + +# --------------------------------------------------------------------------- +# Merge semantics +# --------------------------------------------------------------------------- + +class TestMergeNeverClobber: + + def test_the_trinity_entry_survives(self, mod, home): + """`inject_trinity_mcp_if_configured()` may write .mcp.json before OR + after this runs; either way its entry must be intact.""" + trinity = {"type": "http", "url": "http://mcp-server:8080/mcp", + "headers": {"Authorization": "Bearer trinity_mcp_x"}} + _, written = _render( + mod, home, + {"mcpServers": {"new": {"command": "npx", "args": ["-y", "p"]}}}, + existing={"mcpServers": {"trinity": trinity}}, + ) + assert written["mcpServers"]["trinity"] == trinity + assert "new" in written["mcpServers"] + + def test_an_owner_edited_entry_is_not_reverted_on_restart(self, mod, home): + edited = {"command": "npx", "args": ["-y", "pkg@2.0.0"]} + result, written = _render( + mod, home, + {"mcpServers": {"pkg": {"command": "npx", "args": ["-y", "pkg@1.0.0"]}}}, + existing={"mcpServers": {"pkg": edited}}, + ) + assert result["added"] == [] + assert written["mcpServers"]["pkg"] == edited + + def test_rendering_twice_changes_nothing(self, mod, home): + tpl, cfg, envf = _paths(home) + tpl.write_text(json.dumps( + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "p"]}}})) + envf.write_text("") + + first = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + snapshot = cfg.read_text() + second = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + + assert first["added"] == ["x"] + assert second["added"] == [] + assert cfg.read_text() == snapshot + + def test_an_unparseable_existing_config_is_left_alone(self, mod, home): + """Not ours to repair, and merging into it blind would destroy it.""" + tpl, cfg, envf = _paths(home) + tpl.write_text(json.dumps({"mcpServers": {"x": {"command": "npx"}}})) + envf.write_text("") + cfg.write_text("{ not json") + + result = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + + assert result["status"] == "existing_unreadable" + assert cfg.read_text() == "{ not json" + + +# --------------------------------------------------------------------------- +# Never breaks the boot +# --------------------------------------------------------------------------- + +class TestStartupSafety: + + @pytest.mark.parametrize("template,status", [ + ("{ not json", "invalid_json"), + ({"mcpServers": {}}, "no_servers"), + ({"no_servers_key": 1}, "no_servers"), + ('"a string"', "no_servers"), + ]) + def test_malformed_template_degrades_quietly(self, mod, home, template, status): + result, written = _render(mod, home, template) + assert result["status"] == status + assert written is None + + def test_absent_template_is_a_no_op(self, mod, home): + tpl, cfg, envf = _paths(home) + result = mod.render(template_file=tpl, config_file=cfg, env_file=envf) + assert result["status"] == "no_template" + assert not cfg.exists() + + def test_main_always_reports_success(self, mod, monkeypatch): + """startup.sh must continue whatever happens here.""" + monkeypatch.setattr(mod, "TEMPLATE_FILE", Path("/nonexistent/.mcp.json.template")) + assert mod.main() == 0 + + +# --------------------------------------------------------------------------- +# Wiring +# --------------------------------------------------------------------------- + +def test_startup_sh_invokes_the_renderer(): + """The module is only a fix if the boot path runs it.""" + startup = (_ROOT / "docker" / "base-image" / "startup.sh").read_text() + assert "agent_server.mcp_template" in startup, ( + "startup.sh does not run the .mcp.json.template renderer (#2007)" + ) + + +def test_the_renderer_runs_after_credentials_are_available(): + """Ordering: `.env` arrives either from the /generated-creds copy or from + the decrypt-and-inject fallback. Rendering before the latter would resolve + placeholders against a file that is not there yet.""" + startup = (_ROOT / "docker" / "base-image" / "startup.sh").read_text() + assert startup.index("decrypt-and-inject") < startup.index("agent_server.mcp_template"), ( + "the renderer runs before the credential auto-import — placeholders " + "would be unresolvable on that path" + ) From f170754ed66fc2895873d949373058086f10f551 Mon Sep 17 00:00:00 2001 From: Oleksii Dolhov Date: Mon, 10 Aug 2026 11:45:14 +0300 Subject: [PATCH 2/2] fix(2007): validate before substituting, guard each entry, name the server MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review items; the first made the PR's headline outcome unreachable in production while CI stayed green. 1. Validation ran AFTER substitution, but mcp_validator was written for the UNRENDERED config: it strips ${...} refs and rejects whatever literal remains against _LITERAL_SECRET_PATTERNS and _SHELL_METACHARS_RE. Post substitution the literal IS the secret, so every real credential was withheld — AIzaSy… (Google's actual key shape), sk-ant-, sk-proj-, ghp_, github_pat_, xoxb-, AKIA, and anything containing a shell metacharacter — with the advice 'store it in .env and reference as ${VAR}', which is exactly what the operator did. It passed CI because the fixture used GEMINI_API_KEY="real-key-value", which no credential looks like. The entry is now validated with refs INTACT. That needs one wrinkle: ${VAR:-default} is a Trinity extension the vendored validator does not know, so the probe normalises each ref to ${NAME} — keeping it a REFERENCE, the shape those rules are written against — rather than teaching the vendored copy new syntax (Invariant #5) or validating a rendered secret. Trinity's own placeholder checks now run FIRST, so an operator gets "Trinity substitutes only inside 'env'" instead of the validator's generic "'${SHELL_PATH}' not in allowlist" for the same entry. 2. render() documents 'never raises' and the per-server call was unguarded: _resolves_to_private_ip catches only socket.gaierror, while getaddrinfo raises UnicodeError for an over-long hostname label. One bad entry therefore propagated out of the loop and NOTHING was written — every valid sibling lost. startup.sh's '|| echo' saved the boot, not the render. The call is now wrapped, so the failure is reported as that entry's reason. Guarding the CALL (not the one known exception) covers future validator error types too. 3. The probe key was the literal "_probe", which the validator embeds verbatim, so the message read: withheld MCP server 'aistudio': Server '_probe': … It now uses the declared name. Verified against the motivating case end-to-end: with a real-shaped GEMINI_API_KEY the aistudio server renders and the written .mcp.json carries the key; ${MISSING:-/opt/fallback} still resolves; a 80-character hostname label withholds only its own entry and 'good' is still written. Mutation-verified: validate post-substitution -> 21 failed per-server guard removed -> 2 failed probe key back to "_probe" -> 1 failed as shipped -> 36 passed Related to #2007 Co-Authored-By: Claude Opus 5 (1M context) --- .../base-image/agent_server/mcp_template.py | 99 ++++++++++++---- tests/unit/test_2007_mcp_template_render.py | 112 ++++++++++++++++++ 2 files changed, 190 insertions(+), 21 deletions(-) diff --git a/docker/base-image/agent_server/mcp_template.py b/docker/base-image/agent_server/mcp_template.py index 6ef46bef3..1718fe3b5 100644 --- a/docker/base-image/agent_server/mcp_template.py +++ b/docker/base-image/agent_server/mcp_template.py @@ -137,41 +137,70 @@ def sub(match: re.Match) -> str: return expanded, None -def render_server(config: dict, values: Dict[str, str]) -> Tuple[Optional[dict], Optional[str]]: +def _probe_copy(config: dict) -> dict: + """A validation probe with refs INTACT but written in the plain form. + + `mcp_validator` is vendored byte-identically (Invariant #5) and knows only + `${NAME}`; Trinity's `${NAME:-default}` extension trips its variable-name + check. Rewriting each ref to `${NAME}` for the probe keeps the value a + REFERENCE — which is the shape the validator's literal-secret and + shell-metachar rules are written against — without teaching the vendored + copy a new syntax or validating a rendered secret. + """ + probe = json.loads(json.dumps(config)) + env = probe.get("env") + if isinstance(env, dict): + for key, value in list(env.items()): + if isinstance(value, str): + env[key] = _REF_RE.sub(lambda m: "${%s}" % m.group(1), value) + return probe + + +def render_server( + config: dict, values: Dict[str, str], name: str = "server" +) -> Tuple[Optional[dict], Optional[str]]: """Render ONE server entry. Returns `(rendered, None)` or `(None, reason)`. Substitution is confined to `env`. A `${...}` anywhere else is reported — not expanded — because the validator would reject the expansion anyway and a silent expansion into `command` is the #590 class. + + Validation runs BEFORE substitution (#2007 review). `mcp_validator` was + written for the UNRENDERED config: it strips `${...}` refs and rejects + whatever literal remains against `_LITERAL_SECRET_PATTERNS` and + `_SHELL_METACHARS_RE`. After substitution the literal IS the secret, so + validating the rendered entry rejects every real credential — + `AIzaSy…` (Google's actual key shape), `sk-ant-`, `sk-proj-`, `ghp_`, + `github_pat_`, `xoxb-`, `AKIA`, and anything containing a shell + metacharacter — telling the operator to "store it in .env and reference as + ${VAR}", which is precisely what they did. That made the headline outcome + unreachable in production while CI passed on a fixture value + (`GEMINI_API_KEY="real-key-value"`) that no credential looks like. + + `name` is the DECLARED server name and is used in the failure reason. The + probe key used to be the literal `"_probe"`, which the validator embeds + verbatim, so the operator-facing message read + `withheld MCP server 'aistudio': Server '_probe': ...` — and the named, + actionable reason is this feature's core value claim. """ if not isinstance(config, dict): return None, "entry is not an object" - rendered = json.loads(json.dumps(config)) # deep copy, JSON-only by construction - - env = rendered.get("env") - if env is not None: - if not isinstance(env, dict): - return None, "'env' is not an object" - for key, value in list(env.items()): - if not isinstance(value, str): - continue - resolved, reason = _resolve_refs(value, values) - if reason: - return None, reason - env[key] = resolved - + # Trinity's own placeholder rules run FIRST, so the actionable reason wins + # over the validator's generic "command not in allowlist" for the same + # entry — an operator reading "Trinity substitutes only inside 'env'" knows + # what to change; one reading "'${SHELL_PATH}' not in allowlist" does not. for field in ("command", "url"): - value = rendered.get(field) + value = config.get(field) if isinstance(value, str) and _REF_RE.search(value): return None, ( f"'{field}' contains a ${{VAR}} placeholder; Trinity substitutes " f"only inside 'env' (a credential value must never become the " f"executed command)" ) - args = rendered.get("args") - if isinstance(args, list): - for arg in args: + declared_args = config.get("args") + if isinstance(declared_args, list): + for arg in declared_args: if isinstance(arg, str) and _REF_RE.search(arg): return None, ( "'args' contains a ${VAR} placeholder; Trinity substitutes " @@ -179,10 +208,28 @@ def render_server(config: dict, values: Dict[str, str]) -> Tuple[Optional[dict], "in args" ) + # Validate the entry with refs INTACT — the shape the validator was + # designed for — then substitute into the copy that gets written. + probe_key = name if isinstance(name, str) and name else "server" try: - validate_mcp_config(json.dumps({"mcpServers": {"_probe": rendered}})) + validate_mcp_config(json.dumps({"mcpServers": {probe_key: _probe_copy(config)}})) except McpValidationError as e: return None, str(e) + + rendered = json.loads(json.dumps(config)) # deep copy, JSON-only by construction + + env = rendered.get("env") + if env is not None: + if not isinstance(env, dict): + return None, "'env' is not an object" + for key, value in list(env.items()): + if not isinstance(value, str): + continue + resolved, reason = _resolve_refs(value, values) + if reason: + return None, reason + env[key] = resolved + return rendered, None @@ -250,7 +297,17 @@ def render( for name, config in declared.items(): if name in servers: continue # never clobber an entry that is already installed - rendered, reason = render_server(config, values) + # Guarded (#2007 review): `render()` documents "never raises", but + # `_resolves_to_private_ip` catches only `socket.gaierror` and + # `getaddrinfo` raises `UnicodeError` for an over-long hostname label — + # so one bad entry propagated out of an unguarded call, out of this + # unguarded loop, and cost every VALID sibling server its render. + # `startup.sh`'s `|| echo` saved the boot, not the file. + try: + rendered, reason = render_server(config, values, name) + except Exception as e: # noqa: BLE001 — one entry must never cost the rest + skipped[name] = f"could not be validated ({type(e).__name__}: {e})" + continue if reason: skipped[name] = reason continue diff --git a/tests/unit/test_2007_mcp_template_render.py b/tests/unit/test_2007_mcp_template_render.py index 356a00dda..821bde31d 100644 --- a/tests/unit/test_2007_mcp_template_render.py +++ b/tests/unit/test_2007_mcp_template_render.py @@ -369,3 +369,115 @@ def test_the_renderer_runs_after_credentials_are_available(): "the renderer runs before the credential auto-import — placeholders " "would be unresolvable on that path" ) + + +# --------------------------------------------------------------------------- +# Review follow-ups (#2013 review) +# --------------------------------------------------------------------------- + +class TestRealCredentialValuesRender: + """AC #5 must hold for values that look like real credentials. + + `mcp_validator` was written for the UNRENDERED config: it strips `${...}` + refs and rejects whatever literal remains against `_LITERAL_SECRET_PATTERNS` + and `_SHELL_METACHARS_RE`. Validating AFTER substitution means the literal + IS the secret, so every real key was withheld with advice ("store it in + .env and reference as ${VAR}") that the operator had already followed. CI + passed only because the fixture used `GEMINI_API_KEY="real-key-value"`, + which no credential looks like. + """ + + # Each is the real vendor shape, not a lookalike. + SHAPES = [ + "AIzaSyA1234567890123456789012345678901234", # Google (AIza[0-9A-Za-z_-]{35}) + "sk-ant-api03-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", # Anthropic + "sk-proj-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", # OpenAI + "ghp_1234567890abcdefghij1234567890abcdefgh", # GitHub PAT + "github_pat_11ABCDEFG0abcdefghijkl_ABCDEFGHI", # GitHub fine-grained + "xoxb-123456789012-1234567890123-abcdefghij", # Slack bot + "AKIAIOSFODNN7EXAMPLE", # AWS + "pa$$w|rd;with&shell`meta", # shell metacharacters + ] + + @pytest.mark.parametrize("secret", SHAPES) + def test_the_declared_server_is_rendered_not_withheld(self, mod, home, secret): + result, written = _render( + mod, home, + {"mcpServers": {"aistudio": { + "command": "npx", "args": ["-y", "@aistudio/mcp"], + "env": {"GEMINI_API_KEY": "${GEMINI_API_KEY}"}, + }}}, + env=f'GEMINI_API_KEY="{secret}"\n', + ) + assert result["added"] == ["aistudio"], ( + f"a real-shaped credential was withheld: {result['skipped']}" + ) + assert written["mcpServers"]["aistudio"]["env"]["GEMINI_API_KEY"] == secret + + def test_the_default_form_still_resolves(self, mod, home): + """`${VAR:-default}` is a Trinity extension the vendored validator does + not know, so the probe normalises it to `${VAR}` rather than teaching + the vendored copy a new syntax (Invariant #5) or validating a rendered + secret.""" + result, written = _render( + mod, home, + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "p"], + "env": {"P": "${MISSING:-/opt/fallback}"}}}}, + ) + assert result["added"] == ["x"], result["skipped"] + assert written["mcpServers"]["x"]["env"]["P"] == "/opt/fallback" + + +class TestOneBadServerNeverCostsTheGoodOnes: + """`render()` documents 'never raises' and the per-server call was unguarded. + + `_resolves_to_private_ip` catches only `socket.gaierror`, but + `getaddrinfo` raises `UnicodeError` for an over-long hostname label — so a + single bad entry propagated out of the loop and NOTHING was written, + losing every valid sibling. `startup.sh`'s `|| echo` saved the boot, not + the render. + """ + + def test_an_over_long_hostname_label_only_withholds_its_own_entry(self, mod, home): + result, written = _render( + mod, home, + {"mcpServers": { + "good": {"command": "npx", "args": ["-y", "ok"], "env": {}}, + "bad": {"type": "http", "url": f"https://{'a' * 80}.example.com/mcp"}, + }}, + ) + assert result["added"] == ["good"], ( + "a valid sibling was lost to another entry's failure" + ) + assert "bad" in result["skipped"] + assert written is not None and "good" in written["mcpServers"] + + def test_an_unexpected_error_is_reported_not_raised(self, mod, home, monkeypatch): + """The guard is on the CALL, so any future validator exception type is + covered — not just the one hostname case that was found.""" + def boom(*a, **k): + raise RuntimeError("validator exploded") + + monkeypatch.setattr(mod, "render_server", boom) + result, _ = _render( + mod, home, + {"mcpServers": {"x": {"command": "npx", "args": ["-y", "p"], "env": {}}}}, + ) + assert result["status"] == "ok" + assert "validator exploded" in result["skipped"]["x"] + + +def test_the_withheld_reason_names_the_server_not_the_probe(mod, home): + """The named, actionable reason is this feature's core value claim. + + The probe key used to be the literal `"_probe"`, which the validator embeds + verbatim — so the operator read `withheld MCP server 'aistudio': Server + '_probe': ...`. + """ + result, _ = _render( + mod, home, + {"mcpServers": {"aistudio": {"command": "/bin/sh", "args": ["-c", "id"]}}}, + ) + reason = result["skipped"]["aistudio"] + assert "_probe" not in reason, f"the probe key leaked into the reason: {reason}" + assert "aistudio" in reason, f"the reason does not name the server: {reason}"