diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ee53d79..ba1b7d06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added +- **Version pinning, piece 1: pins only.** A manifest entry's `version:`, or an + overlay's `server_version: {: }`, holds an npx-launched server at one + exact version. pmcp writes the pin into the server's `args` and every platform's + `install` argv, keeping the package name, all or nothing. A pin is ignored with a + warning, and the entry is left unchanged, when it is not one exact version, the package + slot is not a plain registry spec, the launcher is not a bare `npx`, or the entry's own + env could redirect npm. Refusal warnings never include the pin value or an argv, and + name a server only if pmcp ships it. `gateway.update_server` does not move a pinned + server, and `pmcp update` reports it as `[PINNED]`. See + [Consiliency/pmcp#294](https://github.com/Consiliency/pmcp/issues/294); the plan merged + in [Consiliency/pmcp#317](https://github.com/Consiliency/pmcp/pull/317). - **`pmcp guidance --feedback-submission on|off`, and the `enable_feedback_submission` key it writes.** This is the only setting that lets PMCP post a feedback issue to GitHub on your behalf, and it is **off by default**. diff --git a/README.md b/README.md index 94542a3c..da428e6d 100644 --- a/README.md +++ b/README.md @@ -1118,6 +1118,40 @@ crashing the gateway — the shipped manifest always still loads. > run when provisioned — treat an overlay file with the same trust as your own > `.mcp.json`. Policy still applies (denied servers stay denied). +### Pinning a client version + +To hold an npx-launched server at one version, add to `~/.pmcp/manifest.yaml` (or an +approved project overlay): + +```yaml +server_version: + firecrawl: "3.25.5" +``` + +or `version: "3.25.5"` on a whole `servers:` entry. pmcp writes the pin into the server's +args and every install argv, keeping the package name. It must be one exact version: a +range, a dist-tag such as `latest`, build metadata or a package spec is ignored with a +warning (a pin from an earlier source, if any, stands). `server_version: {name: null}` +sets nothing; to remove a pin, delete its line. + +Only npx servers whose package slot is a plain registry name (`pkg`, `pkg@1.2.3`, +`pkg@tag`) can be pinned this way, and only if the entry's own env cannot redirect npm: +locale and terminal keys, npm's logging/timing settings and the keys pmcp ships for that +server are fine; anything else (such as `npm_config_registry` or `NODE_OPTIONS`) makes pmcp +ignore the pin. Only a bare `npx` launcher is pinned (not a path such as `./npx` or +`node_modules/.bin/npx`; on Windows, `npx.cmd`/`npx.exe` too). The launcher of every +`install` argv is judged by the rules of the host pmcp runs on, whatever platform key it sits +under, so a `windows:` argv using `npx.cmd` refuses the pin on Linux or macOS. For uvx, pip, +cargo or docker, or any other case, pin the version in the server's own `command`/`args` in +`.mcp.json` or `.pmcp.json`. + +The pin fixes the npm spec; npm resolves it in your own environment (your `.npmrc`, npm's +cache, and a `node_modules` in the directory pmcp runs in; a `.npmrc` there can also set +the npm keys allowed next to a pin). `gateway.update_server` does not move a pinned server. +`pmcp update` shows it as `[PINNED]` with the version you wrote, except when the entry +also sets an npm setting such as `npm_config_loglevel`: then it prints `[FAILED] … Could not +determine a registry package`, and still moves nothing. + ### Adding Custom Servers For MCP servers not in the manifest, add them to `~/.mcp.json`: diff --git a/src/pmcp/cli.py b/src/pmcp/cli.py index 74ced0db..bbaee51d 100644 --- a/src/pmcp/cli.py +++ b/src/pmcp/cli.py @@ -1005,15 +1005,26 @@ async def _call(tool_name: str, arguments: dict[str, object]) -> dict[str, objec return for item in results: - ok = bool(item.get("ok")) - status = "OK" if ok else "FAILED" - server = item.get("server", "unknown") - message = item.get("message", "") - print(f"[{status}] {server}: {message}") + print(_format_update_result(item)) finally: await client_manager.disconnect_all() +def _format_update_result(item: dict[str, object]) -> str: + """One `pmcp update` line: main's ``[OK]``/``[FAILED]``, or ``[PINNED]``. + + A server whose manifest pin pmcp materialised was deliberately not moved, + so it is ``[PINNED]`` with that validated exact version, not ``[FAILED]`` + (Consiliency/pmcp#294 piece 1). Every other result prints as before. + """ + server = item.get("server", "unknown") + pinned = item.get("pinned_version") + if pinned: + return f"[PINNED] {server}: pinned at {pinned}" + status = "OK" if bool(item.get("ok")) else "FAILED" + return f"[{status}] {server}: {item.get('message', '')}" + + def _get_gateway_url() -> str: """Return the PMCP gateway MCP endpoint URL.""" return os.environ.get( diff --git a/src/pmcp/manifest/loader.py b/src/pmcp/manifest/loader.py index 9837e826..2a12d9d4 100644 --- a/src/pmcp/manifest/loader.py +++ b/src/pmcp/manifest/loader.py @@ -2,6 +2,7 @@ from __future__ import annotations +import functools import logging import os import re @@ -9,14 +10,138 @@ from collections.abc import Mapping from dataclasses import dataclass, field, replace from pathlib import Path +from collections.abc import Iterable from typing import Any, Literal, cast import yaml from pmcp.project_consent import log_refusal, read_and_gate +from pmcp.validation import ( + NPM_FILE_TYPE_RE, + is_valid_package_version, + parse_package_spec, +) logger = logging.getLogger(__name__) + +@functools.lru_cache(maxsize=1) +def _shipped_manifest_entries() -> dict[str, dict[str, Any]]: + """pmcp's OWN shipped ``manifest.yaml``, read directly. + + Never through ``load_manifest``, which applies overlays: no user, project + or ``$PMCP_MANIFEST_PATH`` overlay can add a name or a declared key here. + The file ships with pmcp and does not change under a running process. + """ + path = Path(__file__).parent / "manifest.yaml" + try: + servers = (yaml.safe_load(path.read_bytes()) or {}).get("servers") or {} + except (OSError, yaml.YAMLError, AttributeError): + return {} + return { + name: entry + for name, entry in servers.items() + if isinstance(name, str) and isinstance(entry, dict) + } + + +def _server_label(name: object) -> str: + """How a version-pin log line names a server (Consiliency/pmcp#294 piece 1). + + A name pmcp's SHIPPED manifest defines is shown; any other name is an + overlay's own key, which could be anything an operator pasted (a token), + so it is never shown. No grammar decides this: only the shipped list does. + """ + if isinstance(name, str) and name in _shipped_manifest_entries(): + return f"server '{name}'" + return "an overlay server (name not shown)" + + +def _shipped_declared_keys(name: str) -> frozenset[str]: + """The env keys pmcp's SHIPPED manifest declares for *name*: its + ``env_var`` and its ``api_key_optional_when`` relaxers. An overlay's + declarations exempt nothing.""" + entry = _shipped_manifest_entries().get(name) or {} + keys: set[str] = set() + if isinstance(entry.get("env_var"), str): + keys.add(entry["env_var"]) + relaxers = entry.get("api_key_optional_when") or [] + if isinstance(relaxers, list): + keys.update(k for k in relaxers if isinstance(k, str)) + return frozenset(keys) + + +# Env keys an entry may put in the child's environment without changing what +# npm fetches or runs (an ALLOWLIST: every other key -- `npm_config_package`, +# `npm_config_registry`, `NODE_OPTIONS`, `NODE_PATH`, `PATH`, `HOME`, +# `PREFIX`, `XDG_*`, `nvm_*`, a proxy, a CA file, an unknown name -- may). +# Locale, terminal and colour keys change how output looks, never what runs. +_NPM_INERT_ENV_KEYS = frozenset( + {"LANG", "LANGUAGE", "TERM", "TZ", "NO_COLOR", "FORCE_COLOR"} +) +_NPM_INERT_ENV_PREFIXES = ("LC_",) +# npm config keys (as npm reads them from `npm_config_*`, any case) that change +# output, logging, network timing or the install prompt, never which package +# or version runs. `cache` is NOT here: npx runs a cached package on its +# package.json's word (Consiliency/pmcp#295 board, round 5). +_NPM_CONFIG_KEYS_THAT_CANNOT_REDIRECT = frozenset( + { + "loglevel", + "color", + "progress", + "timing", + "unicode", + "logs-dir", + "logs-max", + "fund", + "audit", + "update-notifier", + "yes", + "fetch-retries", + "fetch-retry-factor", + "fetch-retry-maxtimeout", + "fetch-retry-mintimeout", + "fetch-timeout", + "maxsockets", + } +) + + +def _npm_config_key(env_key: str) -> str: + """The config key npm reads from an ``npm_config_*`` variable: npm's own + ``loadEnv`` rule (``@npmcli/config``, npm 10 and 11) -- strip the prefix + case-insensitively; unless the rest starts with ``//``, replace every ``_`` + except a leading one with ``-`` and lowercase.""" + key = env_key[len("npm_config_") :] + if key.startswith("//"): + return key + return (key[:1] + key[1:].replace("_", "-")).lower() + + +def npm_env_may_redirect(server_name: str, keys: Iterable[str]) -> bool: + """Could an entry-supplied env key change what npm runs for *server_name*? + + True unless EVERY key is proven inert: declared by pmcp's shipped manifest + for this server, a locale/terminal key, or an npm logging/timing config + key. Values are not consulted: an empty value of a non-inert key still + changes npm's reading (an empty ``PATH`` searches the cwd). + """ + declared = _shipped_declared_keys(server_name) + for key in keys: + key = str(key) + if key in declared: + continue + upper = key.upper() + if upper in _NPM_INERT_ENV_KEYS or upper.startswith(_NPM_INERT_ENV_PREFIXES): + continue + if key.lower().startswith("npm_config_") and ( + _npm_config_key(key) in _NPM_CONFIG_KEYS_THAT_CANNOT_REDIRECT + ): + continue + return True + return False + + Platform = Literal["mac", "wsl", "linux", "windows"] ServerTransport = Literal["local", "remote", "sse", "http", "streamable-http"] @@ -88,6 +213,12 @@ class ServerConfig: status: str | None = None source: str | None = None replacement: str | None = None + # The exact client version this entry runs (Consiliency/pmcp#294). Set by + # `version:` on an entry or by an overlay's `server_version:` patch, and + # materialised by `load_manifest` into the npx package slot of `args` and + # of every `install` argv. ``None`` means unpinned -- including a pin that + # was refused, so "version is set" always means "the argv is pinned". + version: str | None = None def credential_storage_key(server: Any) -> str | None: @@ -553,6 +684,248 @@ def _parse_api_key_optional_when( return parsed +def _parse_version_pin(name: str, raw: Any, field_label: str) -> str | None: + """Parse a client version pin, fail-soft and fail-closed. + + One exact SemVer version (``is_valid_package_version``, the grammar the + provision gate already requires of an argv pin) or nothing. Refused, with + a warning: a range (``^3.25.5``, ``3.x``) and a dist-tag (``latest``) -- + both re-resolve at every ``npx -y`` spawn, which is the drift a pin exists + to stop -- a ``v`` prefix, a YAML number, and anything carrying a package + name, whitespace or a flag. The value never names a package: the name is + always taken from the entry's own argv (see `_materialize_version_pin`). + """ + if raw is None: + return None + # SemVer build metadata (the `+...` segment) is refused too: npm ignores it + # when resolving, so `3.25.5+x` would run 3.25.5 while every report echoed + # a label that names nothing (Consiliency/pmcp#295 board, N2). + if isinstance(raw, str) and is_valid_package_version(raw) and "+" not in raw: + return raw + logger.warning( + f"Ignoring a '{field_label}' pin for {_server_label(name)}: a version " + 'pin must be one exact version such as "3.25.5" -- not a range, a ' + 'dist-tag such as "latest", build metadata (+...), a name npm reads as ' + "a local tarball (.tgz/.tar/.tar.gz), or a package spec. This pin is " + "ignored (the value is not logged); a pin from an earlier source, if " + "any, stands" + ) + return None + + +# npm-package-arg's classification, restated as the ALLOWLIST a pin may +# rewrite (Consiliency/pmcp#295 board rounds 1-2). npa decides a spec's class +# in a fixed order, and only its final branch, `fromRegistry`, fetches `name` +# from the registry; every earlier branch (URL, git, alias, file, directory, +# hosted git) names something else. The plan's class table maps each branch to +# the clause below that refuses it. +# +# npa `isFileType`, as `validation.NPM_FILE_TYPE_RE` defines it (reused, never +# restated here): a selector -- or an UNSCOPED bare name -- matching it is a +# tarball FILE to npm. npa checks it BEFORE the registry branch, so before +# "is this a version" too: `1.0.0-x.tgz` is valid SemVer and still a file, +# which is why the selector is tested before `is_valid_package_version` +# (itself tarball-aware) is consulted (round 3, B1'). +_NPM_FILE_TYPE_RE = NPM_FILE_TYPE_RE +# validate-npm-package-name's exclusionList, compared case-insensitively as it +# does: npa refuses these as names (round 3, N-c). +_NPM_EXCLUDED_NAMES = frozenset({"node_modules", "favicon.ico"}) +# A dist-tag: npa's registry branch accepts any encodeURIComponent-safe word +# that is neither a version nor a range; this is the letter-led subset of it +# (fullmatch, so no trailing newline). +_TAG_WORD_RE = re.compile(r"[A-Za-z][A-Za-z0-9._-]*") +# A letter-led word npm may read as a VERSION or RANGE, not a tag: semver's +# loose grammar allows ANY run of leading `v`/`=` (`[v=\s]*`), then a partial +# version whose parts are numbers or x/X/* wildcards, then a loose prerelease +# with or without a hyphen (`x`, `X.x`, `v1`, `vv1.2.3`, `vvX`, `v1.X.xbeta`). +# Refused like every range: rewriting it would be harmless to package identity +# (still `fromRegistry`), but a range is not the version-or-tag class a pin +# replaces. Deliberately broad -- any tail -- so a real tag it also matches +# (`xyz`) only loses its pin, with a warning (round 3, N-a). +_PARTIAL_VERSION_WORD_RE = re.compile( + r"[vV=]*(?:[0-9]+|[xX*])(?:\.(?:[0-9]+|[xX*])){0,2}.*" +) + + +def split_plain_registry_spec(arg: str) -> tuple[str, str | None] | None: + """``(name, selector)`` if npm would fetch *arg* from the registry as + ``name`` at an exact version or a dist-tag; else ``None``. + + Accepted: ``name`` (npa: registry range ``*``), ``name@`` + (npa: ``version``) and ``name@`` (npa: ``tag``). Refused, by + class: anything ``parse_package_spec`` rejects (URLs, aliases, git, paths, + flags: their name half is not a package name, or they have no name); an + unscoped name ending in a tarball suffix (npa: ``file``); a selector that + is not an exact version or a tag word (``:`` / ``/`` / ``~`` / ``.``-led + forms, i.e. alias, git, URL, file, directory, and every range); a tag word + ending in a tarball suffix (npa: ``file``); and a tag word that npm reads as + a version or range (``x``, ``v1``, ``vvX``); a SemVer-shaped selector that + ends in a tarball suffix (npa: ``file``, checked first); and the names + npm excludes (``node_modules``, ``favicon.ico``). Pure grammar: it never asks the + resolver, so it works while npm package identity is disabled. + """ + try: + name, selector = parse_package_spec(arg) + except (ValueError, TypeError, AttributeError): + return None + if not name.startswith("@") and _NPM_FILE_TYPE_RE.search(name): + return None + if name.lower() in _NPM_EXCLUDED_NAMES: + return None + if selector is None: + return name, None + # npa's order: `isFileType` on the selector BEFORE any registry reading, so + # a SemVer-shaped tarball (`3.25.5-corp.tgz`) is a file, not a version. + if _NPM_FILE_TYPE_RE.search(selector): + return None + if is_valid_package_version(selector): + return name, selector + if _TAG_WORD_RE.fullmatch(selector) and not _PARTIAL_VERSION_WORD_RE.fullmatch( + selector + ): + return name, selector + return None + + +def _pin_npx_args(args: list[str], version: str) -> tuple[list[str], str] | None: + """*args* with the npx package slot pinned to *version*, and the name. + + The slot is the provision gate's (`provision_gate._package_slot`): the + first argument that is not an allowlisted leading flag. It must be a plain + registry spec (`split_plain_registry_spec`); its NAME is kept and only its + version suffix is replaced, so a pin can select a version of the package + the entry already runs and never a different one. ``None`` otherwise: + ``myalias@npm:firecrawl-mcp@3.25.5`` -> ``myalias@3.25.5`` would be the + registry package ``myalias`` (codex P1), and + ``firecrawl-mcp@corp-mcp.TGZ`` -> ``firecrawl-mcp@3.25.5`` would turn a + local tarball into a public-registry fetch (round-2 B1). + """ + # Local import: provision_gate is a consumer of this module's ServerConfig. + from pmcp.provision_gate import _NPX_LEADING_FLAGS + + for index, arg in enumerate(args): + if arg in _NPX_LEADING_FLAGS: + continue + plain = split_plain_registry_spec(arg) + if plain is None: + return None + name, _selector = plain + return [*args[:index], f"{name}@{version}", *args[index + 1 :]], name + return None + + +def _on_windows() -> bool: + return os.name == "nt" + + +def _is_bare_npx(command: object) -> bool: + """Is *command* npx spelled BARE -- the launcher the PATH resolves? + + ``npx`` (and, on Windows only, ``npx.cmd`` / ``npx.exe``, any case). A + path (``/tmp/x/npx``, ``./npx``, ``node_modules/.bin/npx``, + ``/proc/self/cwd/npx``, ``~/bin/npx.cmd``) names some other program that + is merely called npx, so a pin written into its argv would not hold + (Consiliency/pmcp#294 piece 1, round 2). The basename is never evidence. + """ + if not isinstance(command, str): + return False + if _on_windows(): + return command.lower() in {"npx", "npx.cmd", "npx.exe"} + return command == "npx" + + +def _materialize_version_pin(server: ServerConfig) -> ServerConfig: + """Write ``server.version`` into every argv that spawns the server. + + Both ``args`` (what ``client/manager.py`` spawns) and every ``install`` + argv (what ``start_install`` runs): pinning one and not the other approves + X and runs latest (`provision_gate._config_runs_exactly`). All or nothing: + if any argv cannot be pinned to the same package, the pin is dropped with + a warning and the entry is returned unpinned with ``version=None``. + + The argv pin holds only if nothing the ENTRY puts in the child's + environment can make npm run something else: an ``npm_config_package`` + turns the pinned spec into a shell command, an ``npm_config_registry`` + fetches it from another registry (measured on npm 10 and 11). So any + ``extra_env`` key -- which also carries the overlays' ``server_env`` + patches -- that ``npm_env_may_redirect`` cannot prove inert refuses the + pin, as does an ``env_var`` credential key the shipped manifest does not + declare. The host's own environment and npm configuration are trusted. + """ + version = server.version + if version is None: + return server + + def refuse(reason: str) -> ServerConfig: + # Fixed text: the reason CATEGORY and the server name only -- never + # the pin, an argv, a command or a package (piece 1). + logger.warning( + f"Ignoring the version pin for {_server_label(server.name)}: " + f"{reason}; the server stays unpinned" + ) + return replace(server, version=None) + + if server.url: + return refuse("it is a remote server, so there is no local client to pin") + if not _is_bare_npx(server.command): + return refuse( + "'version'/'server_version' pins servers launched as bare `npx` " + "only (not uvx/pip/cargo/docker, and not a path to a program named " + "npx); pin any other server with explicit command and args in " + ".mcp.json or .pmcp.json instead" + ) + env_keys = [*server.extra_env, *([server.env_var] if server.env_var else [])] + if npm_env_may_redirect(server.name, env_keys): + return refuse( + "the entry's env may redirect npm (only keys pmcp's shipped manifest " + "declares for it, locale and terminal keys, and npm logging/timing " + "keys are allowed next to a pin)" + ) + pinned = _pin_npx_args(list(server.args), version) + if pinned is None: + return refuse( + "its args name no plain registry package (name or name@version/tag) " + "to pin -- an alias, URL, git, file or range spec could change which " + "package runs" + ) + args, package = pinned + install: dict[Platform, list[str]] = {} + for platform, argv in server.install.items(): + if not argv: + install[platform] = argv + continue + if not _is_bare_npx(argv[0]): + return refuse("an install command is not bare `npx`") + pinned_install = _pin_npx_args(list(argv[1:]), version) + if pinned_install is None or pinned_install[1] != package: + return refuse( + "an install command does not run the plain registry package " + "its args run" + ) + install[platform] = [argv[0], *pinned_install[0]] + return replace(server, args=args, install=install) + + +def _materialize_version_pin_soft(server: ServerConfig) -> ServerConfig: + """`_materialize_version_pin`, contained to one entry. + + Overlay entries are only shape-checked where a field is parsed, so an argv + can still carry a non-string (``args: ["-y", 123]``) or a non-string + ``command``. HEAD loads such an entry untouched; a pin on it must cost that + entry its pin, never the whole manifest (Consiliency/pmcp#295 board, + codex P3). + """ + try: + return _materialize_version_pin(server) + except Exception as exc: + logger.warning( + f"Ignoring the version pin for {_server_label(server.name)}: " + f"its command/args/install could not be read " + f"({type(exc).__name__}); the server stays unpinned" + ) + return replace(server, version=None) + + def _parse_server_config(name: str, data: dict[str, Any]) -> ServerConfig: """Parse a server config from raw YAML data.""" install_data = data.get("install", {}) @@ -613,6 +986,7 @@ def _parse_server_config(name: str, data: dict[str, Any]) -> ServerConfig: status=data.get("status"), source=data.get("source"), replacement=data.get("replacement"), + version=_parse_version_pin(name, data.get("version"), "version"), ) @@ -722,7 +1096,10 @@ def _overlay_manifest_paths() -> list[tuple[str, Path]]: _OverlayDocument = tuple[ - dict[str, ServerConfig], dict[str, CLIAlternative], dict[str, dict[str, str]] + dict[str, ServerConfig], + dict[str, CLIAlternative], + dict[str, dict[str, str]], + dict[str, str], ] @@ -739,7 +1116,7 @@ def _load_overlay_file(path: Path) -> _OverlayDocument: content = path.read_bytes() except OSError as exc: logger.warning(f"Skipping unreadable manifest overlay {path}: {exc}") - return {}, {}, {} + return {}, {}, {}, {} return _parse_overlay_document(path, content) @@ -747,7 +1124,7 @@ def _load_overlay_file(path: Path) -> _OverlayDocument: def _parse_overlay_document(path: Path, content: bytes) -> _OverlayDocument: """Parse overlay bytes, fail-soft. ``path`` is for messages only. - Returns ``(servers, cli_alternatives, server_env)``. A YAML error or a + Returns ``(servers, cli_alternatives, server_env, server_version)``. A YAML error or a non-mapping top-level document logs a warning naming the file and returns empty dicts. Each entry is parsed in its own try/except so one malformed entry is skipped without dropping siblings. @@ -759,18 +1136,22 @@ def _parse_overlay_document(path: Path, content: bytes) -> _OverlayDocument: operator can point a shipped server at a self-hosted endpoint without restating its command, args, and install block. It deliberately cannot create a server: ``servers:`` remains whole-entry replace. + + ``server_version`` is the same kind of patch for ``version``: it pins an + existing server's client without restating its install matrix, and it + cannot create a server either (Consiliency/pmcp#294). """ try: data = yaml.safe_load(content) except yaml.YAMLError as exc: logger.warning(f"Skipping unreadable manifest overlay {path}: {exc}") - return {}, {}, {} + return {}, {}, {}, {} if not isinstance(data, dict): logger.warning( f"Skipping manifest overlay {path}: top-level document is not a mapping" ) - return {}, {}, {} + return {}, {}, {}, {} servers: dict[str, ServerConfig] = {} raw_servers = data.get("servers", {}) @@ -814,7 +1195,22 @@ def _parse_overlay_document(path: Path, content: bytes) -> _OverlayDocument: elif raw_server_env: logger.warning(f"Skipping 'server_env' in overlay {path}: not a mapping") - return servers, cli_alternatives, server_env + server_version: dict[str, str] = {} + raw_server_version = data.get("server_version", {}) + if isinstance(raw_server_version, dict): + for name, raw_version in raw_server_version.items(): + if not isinstance(name, str) or not name: + logger.warning( + f"Skipping non-string 'server_version' key in overlay {path}" + ) + continue + version = _parse_version_pin(name, raw_version, "server_version") + if version is not None: + server_version[name] = version + elif raw_server_version: + logger.warning(f"Skipping 'server_version' in overlay {path}: not a mapping") + + return servers, cli_alternatives, server_env, server_version def load_manifest(manifest_path: Path | None = None) -> Manifest: @@ -868,19 +1264,31 @@ def load_manifest(manifest_path: Path | None = None) -> Manifest: continue # Parse the bytes the gate judged. Re-opening `overlay_path` # here would apply content nobody approved. - overlay_servers, overlay_clis, overlay_server_env = ( - _parse_overlay_document(overlay_path, content) - ) + ( + overlay_servers, + overlay_clis, + overlay_server_env, + overlay_server_version, + ) = _parse_overlay_document(overlay_path, content) else: - overlay_servers, overlay_clis, overlay_server_env = _load_overlay_file( - overlay_path - ) - if overlay_servers or overlay_clis or overlay_server_env: + ( + overlay_servers, + overlay_clis, + overlay_server_env, + overlay_server_version, + ) = _load_overlay_file(overlay_path) + if ( + overlay_servers + or overlay_clis + or overlay_server_env + or overlay_server_version + ): logger.info( f"Applying manifest overlay ({label}) from {overlay_path}: " f"{len(overlay_servers)} servers, " f"{len(overlay_clis)} CLI alternatives, " - f"{len(overlay_server_env)} server_env patches" + f"{len(overlay_server_env)} server_env patches, " + f"{len(overlay_server_version)} server_version pins" ) for name in overlay_servers: if name in servers: @@ -906,6 +1314,26 @@ def load_manifest(manifest_path: Path | None = None) -> Manifest: existing, extra_env={**existing.extra_env, **patch} ) + # Same rules as server_env: after this source's replaces, and never + # for a server the manifest does not already define. + for name, version in overlay_server_version.items(): + existing = servers.get(name) + if existing is None: + logger.warning( + f"Manifest overlay ({label}) from {overlay_path} has a " + f"'server_version' pin for a server it does not " + f"define ({_server_label(name)}): skipped" + ) + continue + servers[name] = replace(existing, version=version) + + # Materialise every pin once, after all overlays: a later source's + # whole-entry replace or server_version patch must be what gets written + # into argv, not an earlier one's. + servers = { + name: _materialize_version_pin_soft(entry) for name, entry in servers.items() + } + manifest = Manifest( version=data.get("version", "1.0"), cli_alternatives=cli_alternatives, diff --git a/src/pmcp/tools/handlers.py b/src/pmcp/tools/handlers.py index 45a956c0..09e9f346 100644 --- a/src/pmcp/tools/handlers.py +++ b/src/pmcp/tools/handlers.py @@ -79,7 +79,7 @@ get_job_manager, InstallError, ) -from pmcp.manifest.loader import load_manifest +from pmcp.manifest.loader import load_manifest, npm_env_may_redirect from pmcp.manifest.package_identity import PackageIdentity, resolve_package_identity from pmcp.manifest.matcher import ( _keyword_match_score, @@ -346,6 +346,38 @@ def _effective_env(env: dict[str, str] | None) -> dict[str, str]: return False +def _materialised_pin( + server_name: str, resolved: ResolvedServerConfig, pinned_to: str +) -> str | None: + """The manifest pin pmcp materialised, if it is what this config runs. + + Consiliency/pmcp#294 piece 1: ``[PINNED]`` only for a ``version:`` / + ``server_version:`` pin that ``load_manifest`` validated (one exact + version) and wrote into ``args`` and every ``install`` argv -- and only + when the config being updated runs exactly that argv (a manifest-sourced + config, or one that inherits the manifest's command and args) and main's + own pin reader names the same version -- and only when that config sets + no cwd (a project's ``.npmrc`` or ``node_modules`` there decides what npm + runs) and no env key ``npm_env_may_redirect`` cannot prove inert (the + same rule that gates materialisation, applied to the config's own env). + Everything else keeps main's reporting unchanged. + """ + config = resolved.config + if not isinstance(config, LocalMcpServerConfig): + return None + if config.cwd or npm_env_may_redirect(server_name, (config.env or {}).keys()): + return None + manifest_server = load_manifest().get_server(server_name) + if manifest_server is None or manifest_server.version is None: + return None + if [config.command, *config.args] != [ + manifest_server.command, + *manifest_server.args, + ]: + return None + return manifest_server.version if pinned_to == manifest_server.version else None + + def _detect_effective_version_pin( package_type: Literal["npm", "pypi", "cargo", "docker", "unknown"], command: str, @@ -4999,6 +5031,9 @@ async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: server=server_name, package_type=package_type, package_name=package_name, + pinned_version=_materialised_pin( + server_name, resolved_config, pinned_to + ), message=( f"'{server_name}' is pinned to '{pinned_to}' in {source_desc} " f"({command} {' '.join(args)}). gateway.update_server will not " diff --git a/src/pmcp/types.py b/src/pmcp/types.py index 95b5a536..7dc8e7c3 100644 --- a/src/pmcp/types.py +++ b/src/pmcp/types.py @@ -1528,6 +1528,11 @@ class UpdateServerOutput(BaseModel): cancelled_request_count: int = 0 cancelled_task_count: int = 0 message: str + # Set only when the server's manifest entry carries a version pin that + # pmcp itself materialised into the argv that runs (Consiliency/pmcp#294 + # piece 1): the validated exact version, so `pmcp update` can say + # [PINNED] instead of [FAILED]. None for every other refusal. + pinned_version: str | None = None class AuthConnectInput(GatewayArguments): diff --git a/tests/test_version_pin.py b/tests/test_version_pin.py new file mode 100644 index 00000000..e12c20ae --- /dev/null +++ b/tests/test_version_pin.py @@ -0,0 +1,1057 @@ +"""Version pinning, piece 1 -- pins only (Consiliency/pmcp#294, proposal 1). + +A manifest entry may carry ``version:``, and an overlay may set it on an +existing (e.g. shipped) entry with ``server_version:`` -- without restating the +install matrix. ``load_manifest`` materialises the pin into the npx package +slot of ``args`` AND every ``install`` argv, keeping the package NAME from the +entry, all or nothing; a refused pin leaves the entry unchanged and logs fixed +text. ``gateway.update_server`` / ``pmcp update`` report such a server as +``[PINNED]``; every other result is reported exactly as on main. +""" + +from __future__ import annotations + +import logging +from pathlib import Path +from typing import Any, cast + +import pytest +import yaml + +from pmcp.config.loader import _merge_manifest_defaults +from pmcp.manifest.loader import ( + Manifest, + load_manifest, + split_plain_registry_spec, +) +from pmcp.policy.policy import PolicyManager +from pmcp.provision_gate import _config_runs_exactly +from pmcp.tools import handlers as handlers_module +from pmcp.tools.handlers import GatewayTools +from pmcp.types import LocalMcpServerConfig, ResolvedServerConfig + +PLATFORMS = ("mac", "linux", "wsl", "windows") + + +@pytest.fixture(autouse=True) +def _isolate_overlays(monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + """No ambient overlay: HOME is already isolated by conftest; add a clean cwd.""" + monkeypatch.delenv("PMCP_MANIFEST_PATH", raising=False) + work = tmp_path / "work" + work.mkdir() + monkeypatch.chdir(work) + + +def _user_overlay(text: str) -> None: + path = Path.home() / ".pmcp" / "manifest.yaml" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + +def _warnings(caplog: pytest.LogCaptureFixture) -> list[str]: + return [r.getMessage() for r in caplog.records if r.levelno >= logging.WARNING] + + +# --------------------------------------------------------------------------- +# Materialisation and refusal +# --------------------------------------------------------------------------- + + +def test_server_version_pins_the_shipped_firecrawl_entry_everywhere_it_spawns() -> None: + """One overlay line pins args AND every install argv; nothing else changes.""" + shipped = load_manifest().servers["firecrawl"] + assert shipped.version is None + _user_overlay('server_version:\n firecrawl: "3.25.5"\n') + + pinned = load_manifest().servers["firecrawl"] + + assert pinned.version == "3.25.5" + assert pinned.args == ["-y", "firecrawl-mcp@3.25.5"] + assert pinned.install == { + p: ["npx", "-y", "firecrawl-mcp@3.25.5"] for p in PLATFORMS + } + # The provision gate's own definition of "runs exactly this spec". + assert _config_runs_exactly(pinned, "firecrawl-mcp@3.25.5") + for field_name in ("command", "env_var", "api_key_optional_when", "extra_env"): + assert getattr(pinned, field_name) == getattr(shipped, field_name) + + +def test_version_replaces_an_existing_tag_on_a_scoped_package() -> None: + """``@playwright/mcp@latest`` becomes ``@playwright/mcp@1.2.3``, scope kept.""" + _user_overlay('server_version:\n playwright: "1.2.3"\n') + + pinned = load_manifest().servers["playwright"] + + assert pinned.args == ["-y", "@playwright/mcp@1.2.3"] + assert _config_runs_exactly(pinned, "@playwright/mcp@1.2.3") + + +def test_version_key_on_a_whole_servers_entry_is_materialised() -> None: + _user_overlay( + """ +servers: + pinned-custom: + description: "custom" + keywords: [c] + install: + linux: ["npx", "-y", "custom-mcp"] + command: "npx" + args: ["-y", "custom-mcp", "--port", "3000"] + version: "2.0.0-rc.1" +""" + ) + + entry = load_manifest().servers["pinned-custom"] + + assert entry.args == ["-y", "custom-mcp@2.0.0-rc.1", "--port", "3000"] + assert entry.install == {"linux": ["npx", "-y", "custom-mcp@2.0.0-rc.1"]} + + +@pytest.mark.parametrize( + "raw", + [ + '"^3.25.5"', # range: re-resolves at every spawn + '"~3.25.5"', + '"3.x"', + '"*"', + '">=3.25.0"', + '"latest"', # dist-tag: the registry moves it + '"next"', + '"v3.25.5"', # not SemVer + "3.25", # YAML float + '"3.25"', + '"3.25.5 --registry=http://evil.test"', + '"evil-pkg@1.0.0"', # an attempt to name a different package + '"npm:evil-pkg@1.0.0"', + '"../../tmp/x"', + '""', + "true", + '"3.25.5+evil"', # build metadata: npm ignores it, reports would echo it + # SemVer-shaped tarballs: npa checks isFileType BEFORE reading a version, + # so each of these makes npx run a LOCAL FILE (round 3, B1'). + '"3.25.5-evil.tgz"', + '"1.0.0-x.TAR"', + '"1.0.0-a.tar.gz"', + '"1.2.3-X.Tar.Gz"', + # npm 10's isFileType (npm-package-arg 12.x) has an unescaped dot in + # `tar.gz`, so `tar-gz` is a tarball too; main's rule follows it. + '"1.0.0-x.tar-gz"', + # A core part above 2**53-1 is a dist-TAG to npm, not a version. + '"9007199254740992.0.0"', + ], +) +def test_server_version_refuses_anything_but_one_exact_version( + raw: str, caplog: pytest.LogCaptureFixture +) -> None: + shipped = load_manifest().servers["firecrawl"] + _user_overlay(f"server_version:\n firecrawl: {raw}\n") + + with caplog.at_level(logging.WARNING): + entry = load_manifest().servers["firecrawl"] + + assert entry.version is None + assert entry.args == shipped.args + assert entry.install == shipped.install + assert any("firecrawl" in m and "server_version" in m for m in _warnings(caplog)) + + +def test_server_version_cannot_create_a_server( + caplog: pytest.LogCaptureFixture, +) -> None: + _user_overlay('server_version:\n no-such-server: "1.0.0"\n') + + with caplog.at_level(logging.WARNING): + manifest = load_manifest() + + assert "no-such-server" not in manifest.servers + # Round 1: an overlay's own key is never shown (only shipped names are). + assert any("does not define (an overlay server" in m for m in _warnings(caplog)) + assert not any("no-such-server" in m for m in _warnings(caplog)) + + +def test_unapproved_project_server_version_contributes_nothing( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + overlay = tmp_path / "proj" / ".pmcp" / "manifest.yaml" + overlay.parent.mkdir(parents=True) + overlay.write_text('server_version:\n firecrawl: "3.25.5"\n') + monkeypatch.chdir(tmp_path / "proj") + + entry = load_manifest().servers["firecrawl"] + + assert entry.version is None + assert entry.args == ["-y", "firecrawl-mcp"] + + +def test_approved_project_server_version_applies( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, approve_project_file: Any +) -> None: + overlay = tmp_path / "proj" / ".pmcp" / "manifest.yaml" + overlay.parent.mkdir(parents=True) + overlay.write_text('server_version:\n firecrawl: "3.25.5"\n') + approve_project_file(overlay) + monkeypatch.chdir(tmp_path / "proj") + + assert load_manifest().servers["firecrawl"].args == ["-y", "firecrawl-mcp@3.25.5"] + + +def test_version_on_a_uvx_server_is_refused_with_the_escape_hatch( + caplog: pytest.LogCaptureFixture, +) -> None: + shipped = load_manifest().servers["fetch"] + assert shipped.command == "uvx" + _user_overlay('server_version:\n fetch: "1.0.0"\n') + + with caplog.at_level(logging.WARNING): + entry = load_manifest().servers["fetch"] + + assert entry.version is None + assert entry.args == shipped.args + assert any("npx" in m and ".mcp.json" in m for m in _warnings(caplog)) + + +def test_version_is_refused_when_an_install_argv_names_another_package( + caplog: pytest.LogCaptureFixture, +) -> None: + """Pinning args but not install would approve X and run latest.""" + _user_overlay( + """ +servers: + split-brain: + description: "args and install disagree" + keywords: [s] + install: + linux: ["npx", "-y", "other-mcp"] + command: "npx" + args: ["-y", "split-mcp"] +server_version: + split-brain: "1.0.0" +""" + ) + + with caplog.at_level(logging.WARNING): + entry = load_manifest().servers["split-brain"] + + assert entry.version is None + assert entry.args == ["-y", "split-mcp"] + assert entry.install == {"linux": ["npx", "-y", "other-mcp"]} + + +# One case per npm-package-arg class that is NOT a registry version/tag +# (the plan's class table). Round 2 (B1) found the tarball class missing: every +# earlier file/URL case carried a `:` prefix, which the old regex excluded +# anyway, so no test exercised a letter-led tarball. +_NON_PLAIN_SLOTS = { + "alias": "myalias@npm:firecrawl-mcp@3.25.5", + "url": "t@https://evil.test/t.tgz", + "git-url": "t@git+https://evil.test/t.git", + "hosted-shortcut": "t@github:evil/x", + "hosted-path": "t@evil/x", + "git-ssh": "t@git@github.com:evil/x", + "file-prefix": "t@file:../evil", + "relative-dir": "t@../evil", + "home-dir": "t@~/evil", + "absolute-dir": "t@/abs/evil", + "tarball-tgz": "t@corp.tgz", + "tarball-TGZ-mixed": "firecrawl-mcp@corp-mcp.TGZ", + "tarball-tar": "t@x.tar", + "tarball-tar.gz": "t@x.tar.gz", + "tarball-Tar.Gz": "@s/p@X.Tar.Gz", + "bare-tarball-tgz": "corp.tgz", + "bare-tarball-TAR": "x.TAR", + "tarball-name-with-version": "corp.tgz@3.25.5", + "range-caret": "t@^3.25.0", + "range-gte": "t@>=1.0.0", + "range-x": "t@x", + "range-X": "t@X", + "range-v-partial": "t@v1.2.x", + "tag-trailing-newline": "t@latest\n", + "not-uri-safe-tag": "t@tag!", + # Round 3 (B1'): a strict-SemVer selector with a tarball tail is a FILE to + # npa, which checks isFileType before it reads a version. A fixed, + # node-free sample of the generated corpus's classes (Verification step 10). + "semver-tarball-prerelease": "firecrawl-mcp@3.25.5-corp.tgz", + "semver-tarball-TAR": "t@1.0.0-x.TAR", + "semver-tarball-build": "t@3.25.5+b.tar.gz", + "semver-tarball-scoped": "@s/p@1.2.3-X.Tar.Gz", + "semver-tarball-npm10-tar-gz": "t@1.0.0-x.tar-gz", + "tarball-npm10-tar-gz": "t@corp.tar-gz", + "oversized-core-is-a-tag": "t@9007199254740992.0.0", + # Round 3 (N-a): loose SemVer allows any run of leading v's. + "range-vv": "t@vv1", + "range-vvX": "t@vvX", + "range-v-xbeta": "t@v1.X.xbeta", + "version-vv": "t@vv1.2.3", + # Round 3 (N-c): validate-npm-package-name's exclusion list, any case. + "excluded-node_modules": "node_modules", + "excluded-Node_Modules-versioned": "Node_Modules@1.0.0", + "excluded-favicon": "favicon.ico@latest", +} + + +def _odd_slot_overlay(slot: str, version: str = "3.25.5") -> None: + _user_overlay( + yaml.safe_dump( + { + "servers": { + "odd-slot": { + "description": "odd slot", + "keywords": ["o"], + "install": {"linux": ["npx", "-y", slot]}, + "command": "npx", + "args": ["-y", slot], + "version": version, + } + } + } + ) + ) + + +@pytest.mark.parametrize( + "slot", list(_NON_PLAIN_SLOTS.values()), ids=list(_NON_PLAIN_SLOTS) +) +def test_version_refuses_a_slot_that_is_not_a_plain_registry_spec( + slot: str, caplog: pytest.LogCaptureFixture +) -> None: + """Only ``name`` or ``name@`` may have its version replaced.""" + _odd_slot_overlay(slot) + + with caplog.at_level(logging.WARNING): + entry = load_manifest().servers["odd-slot"] + + assert split_plain_registry_spec(slot) is None + assert entry.version is None + assert entry.args == ["-y", slot] + assert entry.install == {"linux": ["npx", "-y", slot]} + assert any("plain registry" in m for m in _warnings(caplog)) + + +@pytest.mark.parametrize( + ("slot", "pinned"), + [ + ("t", "t@3.25.5"), # npa: registry range `*` + ("t@1.0.0", "t@3.25.5"), # npa: version + ("t@1.0.0-rc.1", "t@3.25.5"), + ("t@next", "t@3.25.5"), # npa: tag + ("t@beta-2.tgzx", "t@3.25.5"), # tag: `.tgzx` is not a tarball suffix + ("@s/p", "@s/p@3.25.5"), + ("@s/p.tgz", "@s/p.tgz@3.25.5"), # a SCOPED name is never a file to npa + ("@s/p@latest", "@s/p@3.25.5"), + ], +) +def test_version_pins_every_plain_registry_class(slot: str, pinned: str) -> None: + _odd_slot_overlay(slot) + + entry = load_manifest().servers["odd-slot"] + + assert entry.args == ["-y", pinned] + assert entry.version == "3.25.5" + + +def test_version_replaces_a_dist_tag_slot() -> None: + """A dist-tag is part of the plain-registry grammar and is replaced.""" + _user_overlay( + """ +servers: + tagged: + description: "tagged" + keywords: [t] + install: + linux: ["npx", "-y", "tagged-mcp@next"] + command: "npx" + args: ["-y", "tagged-mcp@next"] + version: "1.0.0" +""" + ) + + assert load_manifest().servers["tagged"].args == ["-y", "tagged-mcp@1.0.0"] + + +@pytest.mark.parametrize( + "shape", + [ + 'command: "npx"\n args: ["-y", 123]', + 'command: "npx"\n args: ["-y", "ok-mcp"]\n install:\n linux: ["npx", "-y", 123]', + 'command: 123\n args: ["-y", "ok-mcp"]', + ], +) +def test_a_pin_on_a_malformed_entry_costs_only_that_entry( + shape: str, caplog: pytest.LogCaptureFixture +) -> None: + """HEAD loads every entry of this overlay; the pin must not change that.""" + shipped_count = len(load_manifest().servers) + _user_overlay( + f""" +servers: + malformed: + description: "non-string argv" + keywords: [m] + {shape} + version: "1.2.3" +""" + ) + + with caplog.at_level(logging.WARNING): + manifest = load_manifest() # must not raise + + assert len(manifest.servers) == shipped_count + 1 + assert manifest.servers["malformed"].version is None + assert manifest.servers["firecrawl"].args == ["-y", "firecrawl-mcp"] + assert any("an overlay server (name not shown)" in m for m in _warnings(caplog)) + assert not any("malformed" in m for m in _warnings(caplog)) + + +def test_a_config_entry_without_a_command_inherits_the_pin() -> None: + _user_overlay('server_version:\n firecrawl: "3.25.5"\n') + manifest_servers = load_manifest().servers + + merged = _merge_manifest_defaults( + "firecrawl", LocalMcpServerConfig(command="", args=[]), manifest_servers + ) + + assert merged is not None + assert merged.args == ["-y", "firecrawl-mcp@3.25.5"] + + +def test_explicit_config_args_win_over_the_manifest_pin() -> None: + """``.pmcp.json`` with its own command/args is the operator's override.""" + _user_overlay('server_version:\n firecrawl: "3.25.5"\n') + manifest_servers = load_manifest().servers + + merged = _merge_manifest_defaults( + "firecrawl", + LocalMcpServerConfig(command="npx", args=["-y", "firecrawl-mcp@3.20.0"]), + manifest_servers, + ) + + assert merged is not None + assert merged.args == ["-y", "firecrawl-mcp@3.20.0"] + + +def test_a_remote_server_pin_is_refused(caplog: pytest.LogCaptureFixture) -> None: + remote = next(n for n, s in load_manifest().servers.items() if s.url) + _user_overlay(f'server_version:\n {remote}: "1.0.0"\n') + + with caplog.at_level(logging.WARNING): + entry = load_manifest().servers[remote] + + assert entry.version is None + assert any("remote server" in m for m in _warnings(caplog)) + + +def test_a_later_overlay_source_wins( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, approve_project_file: Any +) -> None: + """User overlay first, then the approved project overlay: the later pin + is the one written into argv (materialised once, after every source).""" + _user_overlay('server_version:\n firecrawl: "3.25.5"\n') + overlay = tmp_path / "proj" / ".pmcp" / "manifest.yaml" + overlay.parent.mkdir(parents=True) + overlay.write_text('server_version:\n firecrawl: "3.26.0"\n') + approve_project_file(overlay) + monkeypatch.chdir(tmp_path / "proj") + + entry = load_manifest().servers["firecrawl"] + + assert entry.version == "3.26.0" + assert entry.args == ["-y", "firecrawl-mcp@3.26.0"] + + +def test_a_later_whole_entry_replace_drops_an_earlier_pin( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, approve_project_file: Any +) -> None: + _user_overlay('server_version:\n firecrawl: "3.25.5"\n') + overlay = tmp_path / "proj" / ".pmcp" / "manifest.yaml" + overlay.parent.mkdir(parents=True) + overlay.write_text( + yaml.safe_dump( + { + "servers": { + "firecrawl": { + "description": "replaced", + "keywords": ["f"], + "install": {"linux": ["npx", "-y", "firecrawl-mcp"]}, + "command": "npx", + "args": ["-y", "firecrawl-mcp"], + } + } + } + ) + ) + approve_project_file(overlay) + monkeypatch.chdir(tmp_path / "proj") + + entry = load_manifest().servers["firecrawl"] + + assert entry.version is None + assert entry.args == ["-y", "firecrawl-mcp"] + + +# --------------------------------------------------------------------------- +# update_server / pmcp update: [PINNED] only for a materialised pin +# --------------------------------------------------------------------------- + + +class _ClientManager: + connected: dict[str, ResolvedServerConfig] = {} + + def get_all_tools(self) -> list[Any]: + return [] + + def get_server_status(self, name: str) -> None: + return None + + def get_all_server_statuses(self) -> list[Any]: + return [] + + def get_registry_meta(self) -> tuple[str, float]: + return ("test-rev", 0.0) + + def get_connected_configs(self) -> dict[str, ResolvedServerConfig]: + return {} + + +def _gateway( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + manifest: Manifest, + configured: list[ResolvedServerConfig] | None = None, +) -> GatewayTools: + monkeypatch.setattr(handlers_module, "load_manifest", lambda: manifest) + monkeypatch.setattr(handlers_module, "load_configs", lambda **_: configured or []) + policy_path = tmp_path / "gateway-policy.yaml" + policy_path.write_text("servers: {}\n") + gateway = GatewayTools( + client_manager=cast(Any, _ClientManager()), + policy_manager=PolicyManager(policy_path=policy_path), + ) + cast(Any, gateway)._platform = "linux" + + async def probe(command: list[str], env: Any = None) -> tuple[bool, str]: + raise AssertionError("a pinned server is never probed") + + monkeypatch.setattr(gateway, "_run_update_probe_command", probe) + return gateway + + +@pytest.mark.asyncio +async def test_update_server_reports_a_materialised_pin_as_pinned( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + from pmcp.cli import _format_update_result + + monkeypatch.setenv("FIRECRAWL_API_KEY", "k-test") + _user_overlay('server_version:\n firecrawl: "3.25.5"\n') + gateway = _gateway(monkeypatch, tmp_path, load_manifest()) + + result = await gateway.update_server({"server_name": "firecrawl"}) + + assert result.ok is False + assert result.pinned_version == "3.25.5" + assert "is pinned to '3.25.5'" in result.message # main's text, unchanged + assert _format_update_result(result.model_dump()) == ( + "[PINNED] firecrawl: pinned at 3.25.5" + ) + + +@pytest.mark.parametrize( + "configured_args", + [ + ["-y", "firecrawl-mcp@3.20.0"], # a configured pin of its own + ["-y", "firecrawl-mcp@3.25.5", "--extra"], # not the materialised argv + ], +) +@pytest.mark.asyncio +async def test_update_server_keeps_mains_report_for_any_other_pin( + configured_args: list[str], monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + """Only a pin pmcp materialised, running exactly as materialised, is + [PINNED]; anything else is reported exactly as main reports it.""" + from pmcp.cli import _format_update_result + + monkeypatch.setenv("FIRECRAWL_API_KEY", "k-test") + _user_overlay('server_version:\n firecrawl: "3.25.5"\n') + configured = ResolvedServerConfig( + name="firecrawl", + source="project", + config=LocalMcpServerConfig(command="npx", args=configured_args), + ) + gateway = _gateway(monkeypatch, tmp_path, load_manifest(), [configured]) + + result = await gateway.update_server({"server_name": "firecrawl"}) + + assert result.ok is False + assert result.pinned_version is None + assert "is pinned to" in result.message # main's refusal, unchanged + assert _format_update_result(result.model_dump()).startswith("[FAILED] firecrawl: ") + + +def test_pmcp_update_prints_mains_lines_for_everything_else() -> None: + from pmcp.cli import _format_update_result + + assert _format_update_result({"server": "a", "ok": True, "message": "m"}) == ( + "[OK] a: m" + ) + assert _format_update_result({"server": "b", "ok": False, "message": "n"}) == ( + "[FAILED] b: n" + ) + + +# --------------------------------------------------------------------------- +# No refused pin value, and no credential-shaped value, is ever logged or +# reported (the pin logs are fixed text: reason category + server name) +# --------------------------------------------------------------------------- + +_SENTINELS = ("fc-0a1b2c3d4e5f6a7b8c9d", "fc_live_0a1b2c3d4e5f6a7b") + + +@pytest.mark.parametrize("sentinel", _SENTINELS, ids=["alnum-dash", "underscore"]) +@pytest.mark.asyncio +async def test_no_refused_pin_or_credential_reaches_a_log_or_update_output( + sentinel: str, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + caplog: pytest.LogCaptureFixture, +) -> None: + """Every refusal path of `_parse_version_pin` and `_materialize_version_pin` + (value grammar, non-npx, remote, non-plain slot, install mismatch, + unreadable argv, unknown server). Two credential-shaped sentinels: + + * ``value`` only in refused pin VALUES and an overlay KEY: it never enters + an argv, so it must appear in no log line of any logger, and in no + update_server field or `pmcp update` line; + * ``argv`` only in the entries' own argv: the pin logs are fixed text, so + it must appear in no line the loader logs and in no `[PINNED]` output. + (main's own messages that echo an entry's argv are unchanged by piece 1.) + """ + from pmcp.cli import _format_update_result + + value, argv_s = sentinel, "zz" + sentinel[::-1] + monkeypatch.setenv("FIRECRAWL_API_KEY", "k-test") + remote = next(n for n, s in load_manifest().servers.items() if s.url) + custom = { + "description": "c", + "keywords": ["c"], + "command": "npx", + "args": ["-y", "custom-mcp", "--api-key", argv_s], + "install": {"linux": ["npx", "-y", "custom-mcp", "--api-key", argv_s]}, + "version": value, + } + overlay = { + "servers": { + "c-value": custom, + "c-slot": {**custom, "args": ["-y", f"{argv_s}.tgz"], "version": "1.0.0"}, + "c-install": { + **custom, + "install": {"linux": ["npx", "-y", f"other-{argv_s}"]}, + "version": "1.0.0", + }, + "c-uvx": {**custom, "command": "uvx", "version": "1.0.0"}, + "c-bad": {**custom, "args": ["-y", 5, argv_s], "version": "1.0.0"}, + "c-ok": {**custom, "version": "1.0.0"}, + }, + "server_version": { + "firecrawl": f"3.25.5+{value}", + "playwright": f"^1.0.0 --api-key {value}", + remote: "1.0.0", + f"https://user:{value}@example.test/x": "1.0.0", + "fetch": value, + }, + } + _user_overlay(yaml.safe_dump(overlay)) + caplog.set_level(logging.DEBUG) + + manifest = load_manifest() + loader_lines = [ + r.getMessage() for r in caplog.records if r.name == "pmcp.manifest.loader" + ] + gateway = _gateway(monkeypatch, tmp_path, manifest) + monkeypatch.setattr(gateway, "_run_update_probe_command", _fake_probe) + outputs = [] + for name in ("firecrawl", "playwright", "fetch", "c-value", "c-slot", "c-ok"): + result = await gateway.update_server({"server_name": name}) + outputs.append((result, _format_update_result(result.model_dump()))) + + refused = ( + "firecrawl", + "playwright", + "fetch", + "c-value", + "c-slot", + "c-install", + "c-uvx", + "c-bad", + remote, + ) + assert all(manifest.servers[n].version is None for n in refused) + assert manifest.servers["c-ok"].version == "1.0.0" + assert ( + len([m for m in loader_lines if "Ignoring" in m or "does not define" in m]) + >= 10 + ) + assert value not in caplog.text + assert all( + value not in r.model_dump_json() and value not in line for r, line in outputs + ) + assert all(argv_s not in m for m in loader_lines) + pinned_lines = [line for r, line in outputs if r.pinned_version] + assert pinned_lines == ["[PINNED] c-ok: pinned at 1.0.0"] + + +async def _fake_probe(command: list[str], env: Any = None) -> tuple[bool, str]: + return (False, "probe output") + + +# --------------------------------------------------------------------------- +# Round 1 (board on 602064d): the entry's env and cwd; names shown only if +# shipped; a later refused pin; `server_version: null` +# --------------------------------------------------------------------------- + +_REDIRECTING_KEYS = { + "npm_config_package": "evil@1.0.0", # npx runs the pinned spec as a command + "NPM_CONFIG_PACKAGE": "evil@1.0.0", # npm reads the prefix in any case + "Npm_Config_Registry": "http://registry.test/", + "npm_config_registry": "", # an empty value still changes npm's reading + "npm_config_cache": "/srv/cache", # npx trusts a cached package.json + "npm_config_//registry.npmjs.org/:_authToken": "tok-zz91", # not on the allowlist + "NODE_OPTIONS": "--require=/srv/x.js", + "NODE_PATH": "/srv/lib", + "PATH": "/srv/bin", + "HOME": "/srv/home", + "PREFIX": "/srv", + "XDG_CONFIG_HOME": "/srv/cfg", + "nvm_dir": "/srv/nvm", + "HTTPS_PROXY": "http://proxy.test:3128", + "SOME_UNKNOWN_KEY": "unknown-zz92", +} + + +@pytest.mark.parametrize("via", ["server_env", "extra_env"]) +@pytest.mark.parametrize("key", list(_REDIRECTING_KEYS)) +def test_a_pin_is_refused_when_the_entry_env_may_redirect_npm( + key: str, via: str, caplog: pytest.LogCaptureFixture +) -> None: + """Round 1 codex B1: with the entry's `npm_config_package`, real npm 10 and + 11 ran the materialised `npx -y pkg@1.2.3` as a shell command from another + package's environment (plan appendix). A pin piece 1 writes must not run a + different package, so any entry env key not proven inert refuses the pin; + the entry is unchanged.""" + shipped = load_manifest().servers["firecrawl"] + value = _REDIRECTING_KEYS[key] + overlay: dict[str, Any] + if via == "server_env": + overlay = {"server_env": {"firecrawl": {key: value}}} + else: + entry = { + "description": "f", + "keywords": ["f"], + "command": "npx", + "args": list(shipped.args), + "install": dict(shipped.install), + "extra_env": {key: value}, + } + overlay = {"servers": {"firecrawl": entry}} + overlay["server_version"] = {"firecrawl": "3.25.5"} + _user_overlay(yaml.safe_dump(overlay)) + + with caplog.at_level(logging.WARNING): + pinned = load_manifest().servers["firecrawl"] + + assert pinned.version is None + assert pinned.args == shipped.args + assert any("may redirect npm" in m for m in _warnings(caplog)) + assert not any(value and value in m for m in _warnings(caplog)) + + +@pytest.mark.parametrize( + "env", + [ + {"LANG": "C.UTF-8", "LC_ALL": "C", "TERM": "xterm", "NO_COLOR": "1"}, + {"npm_config_loglevel": "silent", "NPM_CONFIG_FUND": "false"}, + {"FIRECRAWL_API_URL": "http://self-hosted.test:3002"}, # shipped-declared + ], + ids=["locale", "npm-logging", "shipped-declared"], +) +def test_a_pin_holds_next_to_keys_proven_inert(env: dict[str, str]) -> None: + overlay = { + "server_env": {"firecrawl": env}, + "server_version": {"firecrawl": "3.25.5"}, + } + _user_overlay(yaml.safe_dump(overlay)) + + entry = load_manifest().servers["firecrawl"] + + assert entry.version == "3.25.5" + assert entry.args == ["-y", "firecrawl-mcp@3.25.5"] + + +def test_an_overlay_credential_key_is_not_declared_by_the_shipped_manifest( + caplog: pytest.LogCaptureFixture, +) -> None: + """An overlay's `env_var` is injected into the child, and only pmcp's + shipped manifest can declare a key inert, so an overlay-only server with a + credential cannot be pinned this way (pin it in `.mcp.json` args).""" + entry = { + "description": "c", + "keywords": ["c"], + "command": "npx", + "args": ["-y", "custom-mcp"], + "install": {"linux": ["npx", "-y", "custom-mcp"]}, + "env_var": "CUSTOM_API_KEY", + "version": "1.0.0", + } + _user_overlay(yaml.safe_dump({"servers": {"custom-cred": entry}})) + + with caplog.at_level(logging.WARNING): + loaded = load_manifest().servers["custom-cred"] + + assert loaded.version is None + assert any("may redirect npm" in m for m in _warnings(caplog)) + + +@pytest.mark.parametrize( + ("cwd", "env", "pinned"), + [ + (None, {}, True), + (None, {"LANG": "C.UTF-8"}, True), + ("/srv/project", {}, False), # a project .npmrc / node_modules there + (None, {"NODE_OPTIONS": "--require=/srv/x.js"}, False), + (None, {"npm_config_package": "evil@1.0.0"}, False), + (None, {"SOME_UNKNOWN_KEY": "u"}, False), + (None, {"HTTPS_PROXY": "http://proxy.test:3128"}, False), + (None, {"PATH": "/srv/bin"}, False), + (None, {"NODE_PATH": "/srv/lib"}, False), + (None, {"npm_config_cache": "/srv/cache"}, False), + ], + ids=[ + "inherits", + "inert-env", + "entry-cwd", + "node-options", + "npm-package", + "unknown-key", + "proxy", + "path", + "node-path", + "npm-cache", + ], +) +@pytest.mark.asyncio +async def test_pinned_is_withheld_when_the_config_env_or_cwd_may_redirect_npm( + cwd: str | None, + env: dict[str, str], + pinned: bool, + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + """A config that runs the materialised argv (e.g. a `.pmcp.json` entry with + no command) brings its OWN env and cwd; `[PINNED]` applies only if they + cannot redirect npm (the same rule as materialisation).""" + monkeypatch.setenv("FIRECRAWL_API_KEY", "k-test") + _user_overlay('server_version:\n firecrawl: "3.25.5"\n') + manifest = load_manifest() + configured = ResolvedServerConfig( + name="firecrawl", + source="project", + config=LocalMcpServerConfig( + command="npx", + args=list(manifest.servers["firecrawl"].args), + env=env, + cwd=cwd, + ), + ) + gateway = _gateway(monkeypatch, tmp_path, manifest, [configured]) + + result = await gateway.update_server({"server_name": "firecrawl"}) + + assert result.ok is False + assert (result.pinned_version == "3.25.5") is pinned + + +_NAME_SENTINELS = ("fc-0a1b2c3d4e5f6a7b8c9d", "fc_live_0a1b2c3d4e5f6a7b") + + +@pytest.mark.parametrize("name", _NAME_SENTINELS, ids=["alnum-dash", "underscore"]) +def test_no_overlay_server_name_is_ever_logged( + name: str, caplog: pytest.LogCaptureFixture +) -> None: + """Round 1 codex B2 / claude N1: a bare credential-shaped KEY -- as a + `server_version` key for no server, and as a `servers:` key with a refused + value, a refused slot and an unreadable argv -- never reaches a log line. + Only names in pmcp's shipped manifest are shown.""" + from pmcp.manifest.loader import _server_label + + base = { + "description": "c", + "keywords": ["c"], + "command": "npx", + "install": {"linux": ["npx", "-y", "custom-mcp"]}, + } + overlay = { + "servers": { + name: {**base, "args": ["-y", "custom-mcp"], "version": "latest"}, + name + "-slot": {**base, "args": ["-y", "x.tgz"], "version": "1.0.0"}, + name + "-bad": {**base, "args": ["-y", 5], "version": "1.0.0"}, + }, + "server_version": {name + "-nope": "1.0.0"}, + } + _user_overlay(yaml.safe_dump(overlay)) + caplog.set_level(logging.DEBUG) + + load_manifest() + + lines = [r.getMessage() for r in caplog.records if r.name == "pmcp.manifest.loader"] + assert sum("an overlay server (name not shown)" in m for m in lines) >= 4 + assert not any(name in m for m in lines) + assert _server_label("firecrawl") == "server 'firecrawl'" + + +def test_a_later_refused_pin_leaves_an_earlier_pin_standing( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + approve_project_file: Any, + caplog: pytest.LogCaptureFixture, +) -> None: + """Round 1 claude N2: the user overlay pins 1.0.0, then an approved project + overlay says `latest` (refused). The earlier pin stands, and the warning + says so, not "the server stays unpinned".""" + _user_overlay('server_version:\n firecrawl: "1.0.0"\n') + overlay = tmp_path / "proj" / ".pmcp" / "manifest.yaml" + overlay.parent.mkdir(parents=True) + overlay.write_text("server_version:\n firecrawl: latest\n") + approve_project_file(overlay) + monkeypatch.chdir(tmp_path / "proj") + + with caplog.at_level(logging.WARNING): + entry = load_manifest().servers["firecrawl"] + + assert entry.version == "1.0.0" + assert entry.args == ["-y", "firecrawl-mcp@1.0.0"] + refusal = [m for m in _warnings(caplog) if "server_version" in m] + assert refusal and all("stays unpinned" not in m for m in refusal) + assert any("an earlier source, if any, stands" in m for m in refusal) + + +def test_server_version_null_sets_nothing( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + approve_project_file: Any, + caplog: pytest.LogCaptureFixture, +) -> None: + """`server_version: {name: null}` means "no pin from this source": it + neither pins nor unpins, and logs nothing. Only a whole-entry replace (or + removing the earlier line) takes a pin away.""" + _user_overlay('server_version:\n firecrawl: "1.0.0"\n') + overlay = tmp_path / "proj" / ".pmcp" / "manifest.yaml" + overlay.parent.mkdir(parents=True) + overlay.write_text("server_version:\n firecrawl: null\n") + approve_project_file(overlay) + monkeypatch.chdir(tmp_path / "proj") + + with caplog.at_level(logging.WARNING): + entry = load_manifest().servers["firecrawl"] + + assert entry.version == "1.0.0" + assert not any("server_version" in m for m in _warnings(caplog)) + + +# --------------------------------------------------------------------------- +# Round 2 (piece 1 rev 2.1): only a BARE npx launcher is pinned +# --------------------------------------------------------------------------- + +_NOT_BARE_NPX = [ + "/tmp/x/npx", + "./npx", + "node_modules/.bin/npx", + "/proc/self/cwd/npx", + "~/bin/npx.cmd", +] + + +@pytest.mark.parametrize("where", ["command", "install"]) +@pytest.mark.parametrize("spelling", _NOT_BARE_NPX) +def test_a_pin_is_refused_for_a_launcher_that_is_not_bare_npx( + spelling: str, where: str, caplog: pytest.LogCaptureFixture +) -> None: + """Round 2 (claude N1): the loader decided "npx" by basename, so a program + at some path merely NAMED npx got its argv pinned; provisioning would run + that program. Only the bare spelling the PATH resolves is pinned.""" + command = spelling if where == "command" else "npx" + install = spelling if where == "install" else "npx" + entry = { + "description": "c", + "keywords": ["c"], + "command": command, + "args": ["-y", "custom-mcp"], + "install": {"linux": [install, "-y", "custom-mcp"]}, + "version": "1.0.0", + } + _user_overlay(yaml.safe_dump({"servers": {"custom-path": entry}})) + + with caplog.at_level(logging.WARNING): + loaded = load_manifest().servers["custom-path"] + + assert loaded.version is None + assert loaded.args == ["-y", "custom-mcp"] + assert any("bare `npx`" in m for m in _warnings(caplog)) + assert not any(spelling in m for m in _warnings(caplog)) + + +@pytest.mark.parametrize( + ("windows", "command", "pinned"), + [ + (False, "npx", True), + (False, "npx.cmd", False), + (False, "NPX", False), + (True, "npx.cmd", True), + (True, "NPX.EXE", True), + (True, "~/bin/npx.cmd", False), + (True, "C:\\tools\\npx.cmd", False), + ], +) +def test_the_bare_npx_spellings_per_platform( + windows: bool, command: str, pinned: bool, monkeypatch: pytest.MonkeyPatch +) -> None: + from pmcp.manifest import loader as loader_module + + monkeypatch.setattr(loader_module, "_on_windows", lambda: windows) + entry = { + "description": "c", + "keywords": ["c"], + "command": command, + "args": ["-y", "custom-mcp"], + "install": {"linux": [command, "-y", "custom-mcp"]}, + "version": "1.0.0", + } + _user_overlay(yaml.safe_dump({"servers": {"custom-win": entry}})) + + assert (load_manifest().servers["custom-win"].version == "1.0.0") is pinned + + +def test_a_failure_while_materialising_costs_only_that_entry( + monkeypatch: pytest.MonkeyPatch, caplog: pytest.LogCaptureFixture +) -> None: + """Containment, tested directly: since round 2 the launcher check reads + non-string commands without raising, so the malformed-entry tests no + longer reach the per-entry guard. Any exception while writing one pin + must cost that entry its pin, never the manifest or another pin.""" + from pmcp.manifest import loader as loader_module + + real = loader_module._pin_npx_args + + def flaky(args: list[str], version: str) -> Any: + if "firecrawl-mcp" in args: + raise RuntimeError("boom") + return real(args, version) + + monkeypatch.setattr(loader_module, "_pin_npx_args", flaky) + _user_overlay('server_version:\n firecrawl: "3.25.5"\n playwright: "1.2.3"\n') + + with caplog.at_level(logging.WARNING): + manifest = load_manifest() # must not raise + + assert manifest.servers["firecrawl"].version is None + assert manifest.servers["firecrawl"].args == ["-y", "firecrawl-mcp"] + assert manifest.servers["playwright"].args == ["-y", "@playwright/mcp@1.2.3"] + assert any("could not be read (RuntimeError)" in m for m in _warnings(caplog))