From 8f498ae566a474b88dd2e856118635cb79636a06 Mon Sep 17 00:00:00 2001 From: ViperJuice Date: Wed, 12 Aug 2026 06:47:42 +0000 Subject: [PATCH 1/8] wip: stale-notice fix + regression tests (pre-RED-check) --- src/pmcp/server.py | 4 +- src/pmcp/tools/handlers.py | 46 ++++++- tests/test_tools.py | 244 +++++++++++++++++++++++++++++++++++++ 3 files changed, 292 insertions(+), 2 deletions(-) diff --git a/src/pmcp/server.py b/src/pmcp/server.py index cfdebfdc..cd423d46 100644 --- a/src/pmcp/server.py +++ b/src/pmcp/server.py @@ -126,6 +126,7 @@ def __init__( self._project_root = project_root self._custom_config_path = custom_config_path self._cache_dir = cache_dir or Path(".mcp-gateway") + self._descriptions_cache_path = get_cache_path(self._cache_dir) self._host = host self._port = port self._auth_token = auth_token @@ -185,6 +186,7 @@ def __init__( project_root=project_root, custom_config_path=custom_config_path, guidance_config=self._guidance_config, + descriptions_cache_path=self._descriptions_cache_path, ) if self._audit_jsonl is not None: self._scoped_advisor_audit = ScopedAdvisorAudit( @@ -652,7 +654,7 @@ async def initialize(self) -> None: logger.info("Initializing MCP Gateway...") # Load pre-built descriptions cache - cache_path = get_cache_path(self._cache_dir) + cache_path = self._descriptions_cache_path self._descriptions_cache = load_descriptions_cache(cache_path) if self._descriptions_cache: diff --git a/src/pmcp/tools/handlers.py b/src/pmcp/tools/handlers.py index 2d4e08db..5ea69143 100644 --- a/src/pmcp/tools/handlers.py +++ b/src/pmcp/tools/handlers.py @@ -73,6 +73,7 @@ fetch_registry_servers, load_registry_cache, ) +from pmcp.manifest.refresher import save_descriptions_cache from pmcp.manifest.version_checker import ( detect_package_type, get_package_version, @@ -974,6 +975,7 @@ def __init__( custom_config_path: Path | None = None, guidance_config: GuidanceConfig | None = None, descriptions_cache: DescriptionsCache | None = None, + descriptions_cache_path: Path | None = None, ) -> None: self._client_manager = client_manager self._policy_manager = policy_manager @@ -981,6 +983,10 @@ def __init__( self._custom_config_path = custom_config_path self._guidance_config = guidance_config self._descriptions_cache = descriptions_cache + # Where the descriptions cache lives on disk (.mcp-gateway/descriptions.yaml + # by default). Needed so a successful gateway.update_server can persist the + # refreshed version, not just patch the in-memory copy -- see update_server(). + self._descriptions_cache_path = descriptions_cache_path self._detected_clis: set[str] | None = None self._detected_cli_infos: dict[str, CLIInfo] = {} self._platform: str | None = None @@ -5015,7 +5021,45 @@ async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: latest_version, _ = await get_package_version( server_config.command, server_config.args, timeout=5.0 ) - self._stale_check_cache.pop(server_name, None) + + # gateway.refresh() only reloads configs and reconnects transports; it + # never touches the descriptions cache, so `desc.version` (an upstream + # snapshot from the last `pmcp refresh`, not "installed version") would + # otherwise stay pinned at its pre-update value forever and keep + # producing a stale "update available" notice from every emission site + # (_get_update_warning, catalog_search's stale_updates, the background + # stale sweep) -- even after this update succeeded. If we have a fresh + # latest_version, write it in as the new baseline and persist it so the + # fix survives a restart (load_descriptions_cache reads this file back). + # A `None` latest_version means the version fetch failed; leave the old + # value rather than write something worse than what's already there. + now = time.time() + if latest_version: + if ( + self._descriptions_cache is not None + and server_name in self._descriptions_cache.servers + ): + self._descriptions_cache.servers[server_name].version = latest_version + if self._descriptions_cache_path is not None: + try: + save_descriptions_cache( + self._descriptions_cache, self._descriptions_cache_path + ) + except Exception as e: + # The package update itself already succeeded; a bookkeeping + # failure here shouldn't flip the reported result to failed. + logger.warning( + f"Failed to persist descriptions cache after updating " + f"'{server_name}': {e}" + ) + # Repopulate (not just pop) with the fresh (current, latest) pair so + # catalog_search's stale_updates -- which reads _stale_check_cache + # directly -- agrees the server is current immediately, instead of + # leaving a hole that a later _get_update_warning/_run_stale_index + # recompute would refill from the (now-stale) descriptions value. + self._stale_check_cache[server_name] = (now, latest_version, latest_version) + else: + self._stale_check_cache.pop(server_name, None) message = f"Updated '{server_name}' ({package_type}:{package_name}) and refreshed gateway connections." if not refresh_result.ok: diff --git a/tests/test_tools.py b/tests/test_tools.py index d80827fd..56572182 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -4836,6 +4836,250 @@ async def test_start_stop_stale_indexer(self): assert gt._stale_index_task is None +class TestUpdateServerVersionRepair: + """A successful gateway.update_server must clear the stale-notice trail. + + gateway.refresh() (called from inside update_server) never touches the + descriptions cache -- only refresh_all() (server.py's own startup/refresh + path, which update_server does not call) does. Without this repair, + desc.version stays pinned at its pre-update value forever, and every + notice emission site (_get_update_warning, catalog_search's + stale_updates, the background stale sweep) keeps recomputing the same + stale "update available" notice from it -- even across a restart, since + load_descriptions_cache reads the same stale value back off disk. + """ + + def _make_manifest_with_playwright(self, monkeypatch: pytest.MonkeyPatch) -> Manifest: + manifest = Manifest( + version="1.0", + cli_alternatives={}, + servers={ + "playwright": ServerConfig( + name="playwright", + description="Browser automation", + keywords=["browser"], + install={}, + command="npx", + args=["-y", "@playwright/mcp"], + requires_api_key=False, + ) + }, + discovery_queue_path=".mcp-gateway/discovery_queue.json", + ) + monkeypatch.setattr("pmcp.tools.handlers.load_manifest", lambda: manifest) + return manifest + + def _make_cache(self, version: str = "0.1.0") -> DescriptionsCache: + return DescriptionsCache( + generated_at="2024-01-01T00:00:00", + gateway_version="1.0.0", + servers={ + "playwright": GeneratedServerDescriptions( + package="@playwright/mcp", + version=version, + generated_at="2024-01-01T00:00:00", + capability_summary="Browser automation", + tools=[], + ) + }, + ) + + def _wire_success( + self, + gt: GatewayTools, + monkeypatch: pytest.MonkeyPatch, + *, + latest_version: str | None, + ) -> None: + """Wire update_server's collaborators for a successful probe + refresh.""" + + async def _fake_probe(command, env=None): + return (True, "ok") + + monkeypatch.setattr(gt, "_run_update_probe_command", _fake_probe) + monkeypatch.setattr( + gt, + "refresh", + lambda input_data: __import__("asyncio").sleep( + 0, result=types.SimpleNamespace(ok=True) + ), + ) + + async def fake_get_package_version(command, args, timeout=5.0): + return (latest_version, "npm") + + monkeypatch.setattr( + "pmcp.tools.handlers.get_package_version", fake_get_package_version + ) + + @pytest.mark.asyncio + async def test_update_server_bumps_descriptions_cache_version( + self, monkeypatch: pytest.MonkeyPatch + ): + """On success, the descriptions-cache version is rewritten to latest.""" + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + gt = GatewayTools( + client_manager=MockClientManager(), # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is True + assert result.latest_version == "0.2.0" + assert cache.servers["playwright"].version == "0.2.0" + + @pytest.mark.asyncio + async def test_update_server_clears_get_update_warning( + self, monkeypatch: pytest.MonkeyPatch + ): + """_get_update_warning must stop reporting the update as available. + + Reproduces the maintainer's report: gateway.update_server("context7") + reported success but the notice kept recurring on every subsequent + call. + """ + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + gt = GatewayTools( + client_manager=MockClientManager(), # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + await gt.update_server({"server_name": "playwright"}) + + warning = await gt._get_update_warning("playwright") + assert warning is None + + @pytest.mark.asyncio + async def test_update_server_clears_catalog_search_stale_updates( + self, monkeypatch: pytest.MonkeyPatch + ): + """catalog_search's stale_updates must also stop naming the server. + + This is the trap: update_server's own _stale_check_cache.pop() would + already make an *immediate* catalog_search go quiet, so that alone + doesn't discriminate the fix. Force the recompute that production + traffic would trigger (a follow-up _get_update_warning, or the + background stale sweep) before checking catalog_search, so a + left-behind stale descriptions value has a chance to leak back in. + """ + tools = [ + ToolInfo( + tool_id="playwright::snapshot", + server_name="playwright", + tool_name="snapshot", + description="Take snapshot", + short_description="Take snapshot", + input_schema={"type": "object", "properties": {}}, + tags=["browser"], + risk_hint=RiskHint.LOW, + ) + ] + client_manager = MockClientManager(tools) + client_manager.set_server_online("playwright") + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + await gt.update_server({"server_name": "playwright"}) + # Force the same recompute _get_update_warning or the background + # sweep would perform on the next call. + await gt._get_update_warning("playwright") + + result = await gt.catalog_search({}) + + assert result.stale_updates is None + + @pytest.mark.asyncio + async def test_update_server_persists_version_across_restart( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ): + """The repaired version must survive a restart, not just live in memory. + + Simulates a restart by discarding the in-process GatewayTools and + re-loading the cache file from disk, the same way + GatewayServer.initialize() does at startup. + """ + from pmcp.manifest.refresher import load_descriptions_cache + + self._make_manifest_with_playwright(monkeypatch) + cache_path = tmp_path / "descriptions.yaml" + cache = self._make_cache(version="0.1.0") + gt = GatewayTools( + client_manager=MockClientManager(), # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + descriptions_cache_path=cache_path, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + result = await gt.update_server({"server_name": "playwright"}) + assert result.ok is True + + reloaded = load_descriptions_cache(cache_path) + assert reloaded is not None + assert reloaded.servers["playwright"].version == "0.2.0" + + @pytest.mark.asyncio + async def test_update_server_failed_probe_does_not_bump_version( + self, monkeypatch: pytest.MonkeyPatch + ): + """A failed update probe must not touch the recorded version.""" + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + gt = GatewayTools( + client_manager=MockClientManager(), # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + + async def _fake_probe_fails(command, env=None): + return (False, "npm ERR! network timeout") + + monkeypatch.setattr(gt, "_run_update_probe_command", _fake_probe_fails) + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is False + assert cache.servers["playwright"].version == "0.1.0" + + @pytest.mark.asyncio + async def test_update_server_none_latest_version_leaves_old_value( + self, monkeypatch: pytest.MonkeyPatch + ): + """A failed version fetch (None) must leave the old version intact. + + Writing "unknown" (or any other sentinel) would be worse than leaving + the previous, at-least-plausible value in place. + """ + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + gt = GatewayTools( + client_manager=MockClientManager(), # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version=None) + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is True + assert result.latest_version is None + assert cache.servers["playwright"].version == "0.1.0" + assert "playwright" not in gt._stale_check_cache + + class TestInvokeErrorPaths: """Tests for gateway.invoke error handling paths.""" From 31b65f087ecbf68afef44b9846c6b3f4094c0678 Mon Sep 17 00:00:00 2001 From: ViperJuice Date: Wed, 12 Aug 2026 06:51:35 +0000 Subject: [PATCH 2/8] chore: ruff format tests/test_tools.py; add CHANGELOG entry --- CHANGELOG.md | 14 ++++++++++++++ tests/test_tools.py | 4 +++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e9be3a56..42332391 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- **`gateway.update_server` no longer leaves a stale "update available" + notice behind after a successful update.** `update_server` calls + `gateway.refresh()` to reconnect, but `refresh()` only reloads configs and + transports -- it never touches the descriptions cache, so the recorded + `version` (an upstream-latest snapshot from the last `refresh`/describe + pass, not the installed version) stayed pinned at its pre-update value. + Every notice path (`gateway.describe`/`invoke`'s update warning, + `catalog_search`'s `stale_updates`, and the background stale sweep) kept + recomputing the identical stale notice from that value -- surviving even a + restart, since the cache is reloaded from disk. On a successful update, + the freshly-probed latest version is now written into the descriptions + cache and persisted to disk so the fix survives a restart too. + ## [2.1.0] - 2026-08-12 ### Added diff --git a/tests/test_tools.py b/tests/test_tools.py index 56572182..428723ef 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -4849,7 +4849,9 @@ class TestUpdateServerVersionRepair: load_descriptions_cache reads the same stale value back off disk. """ - def _make_manifest_with_playwright(self, monkeypatch: pytest.MonkeyPatch) -> Manifest: + def _make_manifest_with_playwright( + self, monkeypatch: pytest.MonkeyPatch + ) -> Manifest: manifest = Manifest( version="1.0", cli_alternatives={}, From 5d5fa9896c069bcd5213015cd95d891fce2df42a Mon Sep 17 00:00:00 2001 From: ViperJuice Date: Wed, 12 Aug 2026 07:25:30 +0000 Subject: [PATCH 3/8] wip: gate stale-notice repair on an actual server restart (board review) --- src/pmcp/tools/handlers.py | 171 ++++++++++++++++++++------- src/pmcp/types.py | 16 +++ tests/test_tools.py | 236 +++++++++++++++++++++++++++++++++++-- 3 files changed, 368 insertions(+), 55 deletions(-) diff --git a/src/pmcp/tools/handlers.py b/src/pmcp/tools/handlers.py index 5ea69143..2cc528a1 100644 --- a/src/pmcp/tools/handlers.py +++ b/src/pmcp/tools/handlers.py @@ -11,6 +11,7 @@ import platform import shutil from collections import deque +from datetime import datetime, timezone from pathlib import Path from collections.abc import Callable from typing import Any, Literal, cast @@ -116,6 +117,7 @@ ListPendingInput, ListPendingOutput, PendingRequestInfo, + PrebuiltToolInfo, ProvisionInput, ProvisionJobStatus, ProvisionOutput, @@ -611,8 +613,11 @@ def get_gateway_tool_definitions() -> list[Tool]: Tool( name="gateway.update_server", description=( - "Update a subordinate MCP server package to latest version and reconnect it. " - "Use this when invoke/describe/provision warn that a newer version is available." + "Update a subordinate MCP server package to latest version and restart it " + "so the new version is actually running. " + "Use this when invoke/describe/provision warn that a newer version is available. " + "Refuses to restart by default when the server has pending requests; " + "set force=true to cancel them." ), input_schema={ "type": "object", @@ -620,7 +625,12 @@ def get_gateway_tool_definitions() -> list[Tool]: "server_name": { "type": "string", "description": "Name of server to update", - } + }, + "force": { + "type": "boolean", + "default": False, + "description": "Cancel this server's pending requests before restarting", + }, }, "required": ["server_name"], }, @@ -4948,7 +4958,7 @@ async def submit_feedback(self, input_data: dict[str, Any]) -> SubmitFeedbackOut ) async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: - """gateway.update_server - Update a subordinate MCP package and reconnect.""" + """gateway.update_server - Update a subordinate MCP package and restart it.""" parsed = UpdateServerInput.model_validate(input_data) server_name = parsed.server_name @@ -5022,56 +5032,129 @@ async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: server_config.command, server_config.args, timeout=5.0 ) - # gateway.refresh() only reloads configs and reconnects transports; it - # never touches the descriptions cache, so `desc.version` (an upstream - # snapshot from the last `pmcp refresh`, not "installed version") would - # otherwise stay pinned at its pre-update value forever and keep - # producing a stale "update available" notice from every emission site - # (_get_update_warning, catalog_search's stale_updates, the background - # stale sweep) -- even after this update succeeded. If we have a fresh - # latest_version, write it in as the new baseline and persist it so the - # fix survives a restart (load_descriptions_cache reads this file back). - # A `None` latest_version means the version fetch failed; leave the old - # value rather than write something worse than what's already there. + # gateway.refresh() is a diff-based reconcile (_refresh_config_unchanged): + # it deliberately leaves a server whose command/args are unchanged + # connected and running, so a version-only update like `npx pkg@latest` + # never changes argv and refresh() alone never respawns the process -- + # the OLD package keeps serving requests despite the probe having + # installed the new one. Explicitly restart the target server, reusing + # the same resolve+disconnect+connect machinery gateway.restart_server + # already exercises, so the freshly-fetched package is what's actually + # running. + restart_result = await self.restart_server( + {"server_name": server_name, "force": parsed.force} + ) + + # `desc.version` (an upstream snapshot from the last `pmcp + # refresh`/describe pass, not "installed version") is what every stale + # "update available" notice emission site reads (_get_update_warning, + # catalog_search's stale_updates, the background stale sweep). Only + # bump/persist it -- and only clear the memoized stale-check entry -- + # once the server has actually been restarted onto the new package. + # If the restart failed or was refused, the gateway is still serving + # the OLD version, so the notice is still telling the truth and must + # be left alone. now = time.time() - if latest_version: - if ( - self._descriptions_cache is not None - and server_name in self._descriptions_cache.servers - ): - self._descriptions_cache.servers[server_name].version = latest_version - if self._descriptions_cache_path is not None: - try: - save_descriptions_cache( - self._descriptions_cache, self._descriptions_cache_path - ) - except Exception as e: - # The package update itself already succeeded; a bookkeeping - # failure here shouldn't flip the reported result to failed. - logger.warning( - f"Failed to persist descriptions cache after updating " - f"'{server_name}': {e}" + if restart_result.ok: + if latest_version: + if ( + self._descriptions_cache is not None + and server_name in self._descriptions_cache.servers + ): + desc_entry = self._descriptions_cache.servers[server_name] + desc_entry.version = latest_version + # The restart just gave us a live, freshly-connected tool + # list for this server -- regenerate `tools`/`generated_at` + # from it too, not just `version`. Otherwise the entry + # would claim tools were "generated" for a version they + # weren't (GeneratedServerDescriptions.version is + # documented as "Package version when generated"), and a + # package update that changed tool signatures/descriptions + # or added/removed tools would keep serving the pre-update + # tool list to offline catalog_search. This regenerates + # from the connection `restart_server` already made -- + # no extra stdio round-trip, unlike refresher.refresh_server. + # `capability_summary` is deliberately left as-is: it only + # feeds GatewayServer.initialize()'s startup MCP-instructions + # text (computed once, not read live by catalog_search or + # invoke), so leaving it one refresh cycle behind is a much + # smaller, non-functional gap than a stale `tools` list was. + live_tools = [ + t + for t in self._client_manager.get_all_tools() + if t.server_name == server_name + ] + desc_entry.tools = [ + PrebuiltToolInfo( + name=t.tool_name, + description=t.description, + short_description=t.short_description, + tags=t.tags, + risk_hint=t.risk_hint.value, ) - # Repopulate (not just pop) with the fresh (current, latest) pair so - # catalog_search's stale_updates -- which reads _stale_check_cache - # directly -- agrees the server is current immediately, instead of - # leaving a hole that a later _get_update_warning/_run_stale_index - # recompute would refill from the (now-stale) descriptions value. - self._stale_check_cache[server_name] = (now, latest_version, latest_version) - else: - self._stale_check_cache.pop(server_name, None) + for t in live_tools + ] + desc_entry.generated_at = datetime.now(timezone.utc).isoformat() + if self._descriptions_cache_path is not None: + try: + save_descriptions_cache( + self._descriptions_cache, self._descriptions_cache_path + ) + except Exception as e: + # The package update + restart already succeeded; a + # bookkeeping failure here shouldn't flip the + # reported result to failed. + logger.warning( + f"Failed to persist descriptions cache after updating " + f"'{server_name}': {e}" + ) + # Repopulate (not just pop) with the fresh (current, latest) + # pair so catalog_search's stale_updates -- which reads + # _stale_check_cache directly -- agrees the server is current + # immediately, instead of leaving a hole that a later + # _get_update_warning/_run_stale_index recompute would refill + # from the (now-stale) descriptions value. + self._stale_check_cache[server_name] = ( + now, + latest_version, + latest_version, + ) + else: + self._stale_check_cache.pop(server_name, None) - message = f"Updated '{server_name}' ({package_type}:{package_name}) and refreshed gateway connections." - if not refresh_result.ok: - message += " Some servers failed to reconnect; inspect gateway.health." + message = ( + f"Updated and restarted '{server_name}' ({package_type}:{package_name}); " + "the new version is now active." + ) + if not refresh_result.ok: + message += ( + " Some other servers failed to reconnect during the gateway " + "refresh; inspect gateway.health." + ) + else: + restart_errors = ( + "; ".join(restart_result.errors) + if restart_result.errors + else restart_result.message + ) + message = ( + f"Fetched the update for '{server_name}' ({package_type}:{package_name}), " + f"but could not restart the server to activate it: {restart_errors}. " + "The gateway is still serving the previous version. Retry " + f"gateway.update_server(server_name='{server_name}') or " + f"gateway.restart_server(server_name='{server_name}', force=true)." + ) return UpdateServerOutput( - ok=True, + ok=restart_result.ok, server=server_name, package_type=package_type, package_name=package_name, refreshed=refresh_result.ok, + restarted=restart_result.ok, latest_version=latest_version, + cancelled_request_count=restart_result.cancelled_request_count, + cancelled_task_count=restart_result.cancelled_task_count, message=message, ) diff --git a/src/pmcp/types.py b/src/pmcp/types.py index 4bd22c0c..523085a7 100644 --- a/src/pmcp/types.py +++ b/src/pmcp/types.py @@ -1236,6 +1236,13 @@ class UpdateServerInput(BaseModel): """Input for gateway.update_server.""" server_name: str = Field(min_length=1, description="Server to update") + force: bool = Field( + default=False, + description=( + "Restart the server even if it has pending requests or active MCP " + "tasks, cancelling them. Mirrors gateway.restart_server's force flag." + ), + ) class UpdateServerOutput(BaseModel): @@ -1246,7 +1253,16 @@ class UpdateServerOutput(BaseModel): package_type: Literal["npm", "pypi", "cargo", "docker", "unknown"] package_name: str | None = None refreshed: bool = False + # Whether the server was actually restarted onto the new package. Distinct + # from `refreshed`: gateway.refresh()'s diff-based reconcile deliberately + # leaves a server whose command/args are unchanged connected and running + # (see _refresh_config_unchanged), so a version-only update never + # respawns the process through refresh() alone -- only an explicit + # restart does. + restarted: bool = False latest_version: str | None = None + cancelled_request_count: int = 0 + cancelled_task_count: int = 0 message: str diff --git a/tests/test_tools.py b/tests/test_tools.py index 428723ef..f25e9c7f 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -4837,16 +4837,31 @@ async def test_start_stop_stale_indexer(self): class TestUpdateServerVersionRepair: - """A successful gateway.update_server must clear the stale-notice trail. - - gateway.refresh() (called from inside update_server) never touches the - descriptions cache -- only refresh_all() (server.py's own startup/refresh - path, which update_server does not call) does. Without this repair, - desc.version stays pinned at its pre-update value forever, and every - notice emission site (_get_update_warning, catalog_search's - stale_updates, the background stale sweep) keeps recomputing the same - stale "update available" notice from it -- even across a restart, since - load_descriptions_cache reads the same stale value back off disk. + """A successful gateway.update_server must clear the stale-notice trail + -- but only once the update is actually *active*, not merely fetched. + + gateway.refresh() (also called from inside update_server, for broader + reconciliation) never touches the descriptions cache -- only + refresh_all() (server.py's own startup/refresh path, which update_server + does not call) does. That alone motivates writing the freshly-probed + version into the descriptions cache on success. + + But refresh() is *also* diff-based (_refresh_config_unchanged): it + deliberately leaves a server whose resolved command/args are unchanged + connected and running, so it never respawns the process. A version-only + update (`npx pkg@latest`, `uvx --refresh pkg`) never changes argv, so + refresh() alone never restarts the target server -- the OLD package + keeps serving requests no matter how many times refresh() runs. Board + review caught that an earlier version of this fix wrote the new version + into the cache (and silenced the notice) without ever actually + restarting the server, which trades a noisy-but-honest notice for a + silently wrong served version -- worse. update_server now explicitly + restarts the server (reusing gateway.restart_server's own resolve + + disconnect + connect machinery) and only bumps/persists the descriptions + cache -- including regenerating `tools`/`generated_at` from the tool + list that restart just gave it live -- when that restart actually + succeeded. If the restart fails or is refused, the notice is left + exactly alone: it is still telling the truth. """ def _make_manifest_with_playwright( @@ -4869,6 +4884,11 @@ def _make_manifest_with_playwright( discovery_queue_path=".mcp-gateway/discovery_queue.json", ) monkeypatch.setattr("pmcp.tools.handlers.load_manifest", lambda: manifest) + # _resolve_lifecycle_config (used by the real gateway.restart_server + # call update_server now makes) checks configured .mcp.json entries + # before the manifest -- return none, so "playwright" resolves via + # the manifest fixture above instead of hitting the real filesystem. + monkeypatch.setattr("pmcp.tools.handlers.load_configs", lambda **_: []) return manifest def _make_cache(self, version: str = "0.1.0") -> DescriptionsCache: @@ -4893,7 +4913,15 @@ def _wire_success( *, latest_version: str | None, ) -> None: - """Wire update_server's collaborators for a successful probe + refresh.""" + """Wire update_server's probe + version-fetch + broad refresh(). + + Deliberately does NOT stub gateway.restart_server or the + ClientManager it drives -- that is the exact machinery under test + here (board review: an earlier version of this fix never actually + restarted the server). `refresh()` is a separate, broader + reconciliation pass unrelated to this defect and is stubbed as + before. + """ async def _fake_probe(command, env=None): return (True, "ok") @@ -4914,6 +4942,72 @@ async def fake_get_package_version(command, args, timeout=5.0): "pmcp.tools.handlers.get_package_version", fake_get_package_version ) + @pytest.mark.asyncio + async def test_refresh_alone_does_not_restart_config_unchanged_online_server( + self, monkeypatch: pytest.MonkeyPatch + ): + """The invariant that made the original defect invisible. + + A version-only update never changes a server's resolved + command/args, so refresh()'s diff-based reconcile (which exists to + avoid needlessly respawning unrelated unchanged servers) leaves it + connected exactly as-is -- it does not restart it. update_server + cannot rely on calling refresh() to activate an update; it must + restart the server explicitly. + """ + client_manager = MockClientManager() + config = ResolvedServerConfig( + name="playwright", + source="project", + config=LocalMcpServerConfig(command="npx", args=["-y", "@playwright/mcp"]), + ) + client_manager.add_connected_server(config) + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + ) + patch_refresh_config_sources(monkeypatch, [config]) + monkeypatch.setattr( + "pmcp.tools.handlers.load_enabled_auto_start", lambda **_: {"playwright"} + ) + monkeypatch.setattr(gt, "_load_provisioned_registry", lambda: {}) + + result = await gt.refresh({"reason": "test"}) + + assert result.ok is True + assert "disconnect_server:playwright:True" not in client_manager.events + assert "connect_server:playwright" not in client_manager.events + assert client_manager.is_server_online("playwright") is True + + @pytest.mark.asyncio + async def test_update_server_explicitly_restarts_the_server( + self, monkeypatch: pytest.MonkeyPatch + ): + """update_server must actually restart the server, not just refresh(). + + Proves the board-review fix: with refresh() fully stubbed out (see + _wire_success), the only way disconnect/connect events can appear on + the client manager is through update_server's own explicit restart + call. + """ + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + client_manager = MockClientManager() + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is True + assert result.restarted is True + assert "disconnect_server:playwright:False" in client_manager.events + assert "connect_server:playwright" in client_manager.events + assert client_manager.is_server_online("playwright") is True + @pytest.mark.asyncio async def test_update_server_bumps_descriptions_cache_version( self, monkeypatch: pytest.MonkeyPatch @@ -4931,6 +5025,7 @@ async def test_update_server_bumps_descriptions_cache_version( result = await gt.update_server({"server_name": "playwright"}) assert result.ok is True + assert result.restarted is True assert result.latest_version == "0.2.0" assert cache.servers["playwright"].version == "0.2.0" @@ -5077,10 +5172,129 @@ async def test_update_server_none_latest_version_leaves_old_value( result = await gt.update_server({"server_name": "playwright"}) assert result.ok is True + assert result.restarted is True assert result.latest_version is None assert cache.servers["playwright"].version == "0.1.0" assert "playwright" not in gt._stale_check_cache + @pytest.mark.asyncio + async def test_update_server_restart_failure_leaves_notice_intact( + self, monkeypatch: pytest.MonkeyPatch + ): + """A refused/failed restart must NOT bump the version or silence the notice. + + The package may have been fetched successfully, but if the running + server was never restarted onto it, the gateway is still serving the + OLD version -- the "update available" notice is still correct and + must be left alone. `ok` must reflect that the update was not fully + activated. + """ + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + client_manager = MockClientManager() + client_manager.set_server_online("playwright") + client_manager.add_pending_request("playwright") + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + # Default force=False: MockClientManager.disconnect_server refuses a + # restart while a request is pending, exactly like real ClientManager. + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is False + assert result.restarted is False + assert "could not restart" in result.message.lower() + assert cache.servers["playwright"].version == "0.1.0" + assert "playwright" not in gt._stale_check_cache + + warning = await gt._get_update_warning("playwright") + assert warning is not None + assert "0.1.0" in warning and "0.2.0" in warning + + @pytest.mark.asyncio + async def test_update_server_force_cancels_pending_and_activates( + self, monkeypatch: pytest.MonkeyPatch + ): + """force=true cancels pending work so the restart can proceed. + + Surfaces the cancellation count on the output, mirroring + gateway.restart_server's own cancelled_request_count. + """ + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + client_manager = MockClientManager() + client_manager.set_server_online("playwright") + request = client_manager.add_pending_request("playwright") + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + result = await gt.update_server({"server_name": "playwright", "force": True}) + + assert result.ok is True + assert result.restarted is True + assert result.cancelled_request_count == 1 + assert request.future.cancelled() + assert cache.servers["playwright"].version == "0.2.0" + + @pytest.mark.asyncio + async def test_update_server_regenerates_tools_from_live_connection( + self, monkeypatch: pytest.MonkeyPatch + ): + """Restart gives update_server a live tool list -- use it. + + GeneratedServerDescriptions.version is documented as "Package version + when generated". Bumping only `version` while leaving `tools` at the + pre-update snapshot would make the record lie about its own + provenance, and would keep serving the pre-update tool list to + offline catalog_search even after a package update that changed + tools. Regenerate `tools`/`generated_at` together with `version`, + from the connection the restart just made -- no extra stdio + round-trip. `capability_summary` is deliberately left as-is (it only + feeds the startup MCP-instructions text, not live catalog_search). + """ + self._make_manifest_with_playwright(monkeypatch) + cache = self._make_cache(version="0.1.0") + old_generated_at = cache.servers["playwright"].generated_at + old_summary = cache.servers["playwright"].capability_summary + live_tools = [ + ToolInfo( + tool_id="playwright::new_tool", + server_name="playwright", + tool_name="new_tool", + description="A tool that only exists in the new version", + short_description="New in 0.2.0", + input_schema={"type": "object", "properties": {}}, + tags=["new"], + risk_hint=RiskHint.HIGH, + ) + ] + client_manager = MockClientManager(live_tools) + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + self._wire_success(gt, monkeypatch, latest_version="0.2.0") + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is True + entry = cache.servers["playwright"] + assert [t.name for t in entry.tools] == ["new_tool"] + assert entry.tools[0].tags == ["new"] + assert entry.tools[0].risk_hint == "high" + assert entry.generated_at != old_generated_at + # Deliberately unchanged -- see docstring. + assert entry.capability_summary == old_summary + class TestInvokeErrorPaths: """Tests for gateway.invoke error handling paths.""" From 24c3c4ac01ae36c3235cda254b2bbace8fbedd3f Mon Sep 17 00:00:00 2001 From: ViperJuice Date: Wed, 12 Aug 2026 07:27:20 +0000 Subject: [PATCH 4/8] docs: update CHANGELOG for restart-gated update_server fix --- CHANGELOG.md | 39 +++++++++++++++++++++++++++------------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42332391..8954de2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,18 +8,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed -- **`gateway.update_server` no longer leaves a stale "update available" - notice behind after a successful update.** `update_server` calls - `gateway.refresh()` to reconnect, but `refresh()` only reloads configs and - transports -- it never touches the descriptions cache, so the recorded - `version` (an upstream-latest snapshot from the last `refresh`/describe - pass, not the installed version) stayed pinned at its pre-update value. - Every notice path (`gateway.describe`/`invoke`'s update warning, - `catalog_search`'s `stale_updates`, and the background stale sweep) kept - recomputing the identical stale notice from that value -- surviving even a - restart, since the cache is reloaded from disk. On a successful update, - the freshly-probed latest version is now written into the descriptions - cache and persisted to disk so the fix survives a restart too. +- **`gateway.update_server` now actually restarts the server, and no longer + leaves a stale "update available" notice behind after a successful + update.** Two compounding bugs: + - `update_server` called `gateway.refresh()` to activate the update, but + `refresh()` is a diff-based reconcile that deliberately leaves a server + whose resolved command/args are unchanged connected and running. A + version-only update (`npx pkg@latest`, `uvx --refresh pkg`) never + changes argv, so `refresh()` alone never respawned the process -- the + gateway kept serving the OLD package despite `update_server` reporting + success. `update_server` now explicitly restarts the target server + (reusing `gateway.restart_server`'s own resolve/disconnect/connect + machinery) and gates every downstream effect on that restart actually + succeeding; a refused or failed restart is now reported as `ok: false` + with a message explaining the update was fetched but not activated, + instead of silently claiming success. + - Separately, the recorded `version` (an upstream-latest snapshot from the + last `refresh`/describe pass, not the installed version) stayed pinned + at its pre-update value forever, because nothing in the update path ever + rewrote it. Every notice path (`gateway.describe`/`invoke`'s update + warning, `catalog_search`'s `stale_updates`, and the background stale + sweep) kept recomputing the identical stale notice from that value -- + surviving even a restart, since the cache is reloaded from disk. Once + the server restart succeeds, the freshly-probed version -- along with + the tool list and generation timestamp, read live from the just-restarted + connection -- is now written into the descriptions cache and persisted + to disk, so both the notice and the offline tool listing stay accurate + across restarts. ## [2.1.0] - 2026-08-12 From 3679dc9ac7f93d27d8ca643a34ebe5c63a686b44 Mon Sep 17 00:00:00 2001 From: ViperJuice Date: Wed, 12 Aug 2026 07:56:39 +0000 Subject: [PATCH 5/8] fix: unify update_server's probe/restart config resolution; refuse pinned servers (board review round 2) --- src/pmcp/manifest/version_checker.py | 58 +++++++++--- src/pmcp/tools/handlers.py | 137 ++++++++++++++++++++++++--- tests/test_tools.py | 134 ++++++++++++++++++++++++++ 3 files changed, 304 insertions(+), 25 deletions(-) diff --git a/src/pmcp/manifest/version_checker.py b/src/pmcp/manifest/version_checker.py index 9c34adf8..a67eaa88 100644 --- a/src/pmcp/manifest/version_checker.py +++ b/src/pmcp/manifest/version_checker.py @@ -32,6 +32,50 @@ def _strip_npm_tag(package: str) -> str: return name if tag_sep and name else package +def _npm_tag(package: str) -> str | None: + """The npm dist-tag/version suffix ``_strip_npm_tag`` would discard, or + ``None`` if *package* has no such suffix. + + Mirrors ``_strip_npm_tag``'s own scope-aware ``rpartition`` exactly (same + guard conditions), so the two functions can never disagree about whether + a suffix is present or where it starts. Used by gateway.update_server's + pin detection. + """ + if package.startswith("@"): + scope, sep, remainder = package.partition("/") + if not sep: + return None + _name, tag_sep, tag = remainder.rpartition("@") + return tag if tag_sep and _name else None + + _name, tag_sep, tag = package.rpartition("@") + return tag if tag_sep and _name else None + + +def _npm_package_arg(args: list[str]) -> str | None: + """Return the raw npm/npx package token from *args*, or ``None``. + + "Raw" means *before* ``_strip_npm_tag`` removes any ``@tag``/``@version`` + suffix -- factored out of ``detect_package_type``'s npm branch so a + caller that needs the untouched token (gateway.update_server's pin + detection) can get it from the exact same scan ``detect_package_type`` + itself uses, instead of re-implementing the scan a second time and + risking it picking a different argument as "the package". + """ + for arg in args: + if arg == "-y": + continue + # Skip flags + if arg.startswith("-"): + continue + # Found package name (might have @version or @dist-tag suffix) + pkg = _strip_npm_tag(arg) + # Handle scoped packages like @playwright/mcp + if pkg.startswith("@") or not pkg.startswith("-"): + return arg + return None + + async def get_npm_version(package_name: str, timeout: float = 10.0) -> str | None: """ Get the latest version of an npm package. @@ -231,17 +275,9 @@ def detect_package_type( """ if command in ("npx", "npm"): # Find npm package in args (usually after -y flag) - for i, arg in enumerate(args): - if arg == "-y": - continue - # Skip flags - if arg.startswith("-"): - continue - # Found package name (might have @version or @dist-tag suffix) - pkg = _strip_npm_tag(arg) - # Handle scoped packages like @playwright/mcp - if pkg.startswith("@") or not pkg.startswith("-"): - return ("npm", pkg) + raw = _npm_package_arg(args) + if raw is not None: + return ("npm", _strip_npm_tag(raw)) elif command == "uvx": # First non-flag argument is the package diff --git a/src/pmcp/tools/handlers.py b/src/pmcp/tools/handlers.py index 2cc528a1..b0916271 100644 --- a/src/pmcp/tools/handlers.py +++ b/src/pmcp/tools/handlers.py @@ -48,7 +48,7 @@ summarize_startup_resolution, ) from pmcp.errors import ErrorCode, GatewayException, make_error -from pmcp.env_store import set_env_value +from pmcp.env_store import sanitized_subprocess_env, set_env_value from pmcp.validation import env_var_allowed, is_valid_package_name from pmcp.identity import filter_self_references from pmcp.manifest.code_patterns_loader import get_code_hint @@ -56,7 +56,6 @@ from pmcp.templates.code_snippets_loader import get_code_snippet from pmcp.manifest.installer import ( MissingApiKeyError, - build_install_child_env, get_job_manager, InstallError, ) @@ -76,6 +75,8 @@ ) from pmcp.manifest.refresher import save_descriptions_cache from pmcp.manifest.version_checker import ( + _npm_package_arg, + _npm_tag, detect_package_type, get_package_version, is_version_newer, @@ -265,6 +266,58 @@ def _effective_env(env: dict[str, str] | None) -> dict[str, str]: return False +def _detect_effective_version_pin( + package_type: Literal["npm", "pypi", "cargo", "docker", "unknown"], + command: str, + args: list[str], +) -> str | None: + """Return the pinned version/tag *args* explicitly locks to, or ``None``. + + Used by gateway.update_server: a server pinned to a concrete version + cannot be moved to ``@latest`` by this tool while the pin stands (the + README documents pinning a server's version in ``.mcp.json`` as the + supported configuration channel, e.g. for ``index-it-mcp``). + + npm: reuses ``_npm_package_arg``/``_strip_npm_tag`` (the exact scan + ``detect_package_type`` itself uses) rather than re-scanning + independently, so this can never disagree with detect_package_type about + which arg is "the package". ``@latest`` is not a pin -- it's what + gateway.update_server would install anyway. + + pypi/uvx: ``detect_package_type`` does not strip anything for uvx, so an + inline ``pkg==1.2.3`` token already makes the later PyPI lookup fail + (returning ``latest_version=None``, which the existing "don't write on a + failed fetch" gate already protects against) -- this check exists only + to turn that into a clear, specific refusal message instead of an opaque + "could not fetch latest version" one. + """ + if package_type == "npm": + raw = _npm_package_arg(args) + if raw is None: + return None + tag = _npm_tag(raw) + return None if not tag or tag == "latest" else tag + if package_type == "pypi" and command == "uvx": + for arg in args: + if arg.startswith("-"): + continue + if "==" in arg: + _, _, version = arg.partition("==") + return version or None + return None + + +# Human-readable label for a ResolvedServerConfig.source, used in messages +# that need to point an operator at the file a pin (or other override) came +# from. +_CONFIG_SOURCE_LABELS: dict[str, str] = { + "project": "the project .mcp.json", + "user": "the user .mcp.json", + "custom": "the custom MCP config file", + "manifest": "the manifest entry", +} + + # Risk level ordering for filtering RISK_ORDER = {"low": 1, "medium": 2, "high": 3, "unknown": 4} TRACE_CONTEXT_KEYS = ("traceparent", "tracestate", "baggage") @@ -4962,18 +5015,47 @@ async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: parsed = UpdateServerInput.model_validate(input_data) server_name = parsed.server_name - server_config = self._get_server_config_for_update(server_name) - if not server_config: + # Resolve the server's EFFECTIVE config through the exact same + # function -- and therefore the exact same .mcp.json-over-manifest + # precedence -- gateway.restart_server itself uses below. Board + # review: this tool previously resolved the probe target through + # _get_server_config_for_update, which only ever consults the + # manifest/discovered servers and never .mcp.json. For a server + # configured in both places (README documents pinning a server's + # version via .mcp.json as the supported override channel), that + # meant probing one command while restarting a different one -- + # structurally capable of probing/installing upstream @latest while + # restarting onto a completely different, e.g. pinned, config. + # Resolving once through restart_server's own resolver makes that + # divergence impossible: both calls are the same lookup against the + # same inputs, so they can't disagree. + prior_status = self._status_value(server_name) + resolved_config, resolve_failure = self._resolve_lifecycle_config( + server_name, action="restart", prior_status=prior_status + ) + if resolve_failure is not None: + return UpdateServerOutput( + ok=False, + server=server_name, + package_type="unknown", + message=resolve_failure.message, + ) + if resolved_config is None: return UpdateServerOutput( ok=False, server=server_name, package_type="unknown", - message=f"Server '{server_name}' not found in manifest or discovered servers.", + message=f"Server '{server_name}' could not be resolved.", ) - package_type, package_name = detect_package_type( - server_config.command, server_config.args - ) + if isinstance(resolved_config.config, LocalMcpServerConfig): + command = resolved_config.config.command + args = resolved_config.config.args + else: + # Remote servers have no local package for this tool to update. + command, args = "", [] + + package_type, package_name = detect_package_type(command, args) if package_type == "unknown" or not package_name: return UpdateServerOutput( ok=False, @@ -4985,6 +5067,24 @@ async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: ), ) + pinned_to = _detect_effective_version_pin(package_type, command, args) + if pinned_to is not None: + source_desc = _CONFIG_SOURCE_LABELS.get( + resolved_config.source, f"the {resolved_config.source} config" + ) + return UpdateServerOutput( + ok=False, + server=server_name, + package_type=package_type, + package_name=package_name, + message=( + f"'{server_name}' is pinned to '{pinned_to}' in {source_desc} " + f"({command} {' '.join(args)}). gateway.update_server will not " + "move a pinned server to the latest version -- edit or remove " + "the pin in that config to allow updates." + ), + ) + if package_type == "npm": update_cmd = ["npx", "-y", f"{package_name}@latest", "--help"] elif package_type == "cargo": @@ -4996,10 +5096,21 @@ async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: try: # Sanitized env: the probe executes the server's own package code, so - # it gets only this server's resolved credential, never other servers'. - ok, output = await self._run_update_probe_command( - update_cmd, env=build_install_child_env(server_config) + # it gets only this server's resolved credential, never other + # servers'. resolved_config.config.env already carries this + # server's fully-resolved credential -- for a manifest server, + # _manifest_server_to_config injects it the same way + # build_install_child_env used to (same credential_lookup_keys + # scan); for a .mcp.json server it's whatever that entry declares. + # Using it here means the probe's env can never diverge from the + # restart's env either, for the same reason command/args can't. + probe_env = sanitized_subprocess_env( + resolved_config.config.env + if isinstance(resolved_config.config, LocalMcpServerConfig) + else None, + self._project_root, ) + ok, output = await self._run_update_probe_command(update_cmd, env=probe_env) except TimeoutError: return UpdateServerOutput( ok=False, @@ -5028,9 +5139,7 @@ async def update_server(self, input_data: dict[str, Any]) -> UpdateServerOutput: ) refresh_result = await self.refresh({"reason": f"update_server:{server_name}"}) - latest_version, _ = await get_package_version( - server_config.command, server_config.args, timeout=5.0 - ) + latest_version, _ = await get_package_version(command, args, timeout=5.0) # gateway.refresh() is a diff-based reconcile (_refresh_config_unchanged): # it deliberately leaves a server whose command/args are unchanged diff --git a/tests/test_tools.py b/tests/test_tools.py index f25e9c7f..6463094d 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -5295,6 +5295,140 @@ async def test_update_server_regenerates_tools_from_live_connection( # Deliberately unchanged -- see docstring. assert entry.capability_summary == old_summary + @pytest.mark.asyncio + async def test_update_server_uses_configured_override_not_manifest( + self, monkeypatch: pytest.MonkeyPatch + ): + """A .mcp.json override must drive the probe, not the manifest. + + Board review (round 2): update_server previously resolved its probe + target via _get_server_config_for_update (manifest/discovered only) + while gateway.restart_server resolves via _resolve_lifecycle_config + (.mcp.json overrides the manifest). For a server configured in both + places with a *different*, unpinned package, that meant probing and + version-checking the manifest's package while restarting a + completely different one -- and if both happened to succeed, writing + the manifest package's "latest" into the cache for a server that was + never actually running it. Unifying resolution around + _resolve_lifecycle_config must make the .mcp.json override win end + to end: the probe, the version check, AND the restarted process must + all be the *same* configured package. + """ + self._make_manifest_with_playwright(monkeypatch) + configured_override = ResolvedServerConfig( + name="playwright", + source="project", + config=LocalMcpServerConfig( + command="npx", args=["-y", "different-playwright-fork"] + ), + ) + monkeypatch.setattr( + "pmcp.tools.handlers.load_configs", lambda **_: [configured_override] + ) + cache = self._make_cache(version="0.1.0") + client_manager = MockClientManager() + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + + async def _fake_probe(command, env=None): + return (True, "ok") + + monkeypatch.setattr(gt, "_run_update_probe_command", _fake_probe) + monkeypatch.setattr( + gt, + "refresh", + lambda input_data: __import__("asyncio").sleep( + 0, result=types.SimpleNamespace(ok=True) + ), + ) + + async def fake_get_package_version(command, args, timeout=5.0): + # The manifest's bare "@playwright/mcp" would resolve to a + # DIFFERENT (wrong) answer than the .mcp.json override -- if + # this is ever called with the manifest's args, the config + # resolution has diverged from what actually gets restarted. + if "different-playwright-fork" in args: + return ("9.9.9", "npm") + return ("0.2.0", "npm") # manifest's package -- must not be used + + monkeypatch.setattr( + "pmcp.tools.handlers.get_package_version", fake_get_package_version + ) + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is True + assert result.latest_version == "9.9.9" + assert cache.servers["playwright"].version == "9.9.9" + # The server that was actually restarted ran the configured override. + assert client_manager.connected_configs[-1].config.args == [ + "-y", + "different-playwright-fork", + ] + + @pytest.mark.asyncio + async def test_update_server_refuses_pinned_configured_override( + self, monkeypatch: pytest.MonkeyPatch + ): + """A version-pinned .mcp.json override must refuse, not silently lie. + + The exact board-review failure scenario: manifest has an unpinned + `npx -y @playwright/mcp`, .mcp.json pins `@playwright/mcp@1.2.3` + (README documents this pinning pattern for index-it-mcp as the + supported override channel). Restarting the pinned server succeeds + (it's a perfectly valid process to run) -- the bug was recording + upstream's unrelated "latest" as if it were now running. The probe + must never even be attempted: there is nothing @latest can do while + the pin stands. + """ + self._make_manifest_with_playwright(monkeypatch) + pinned_override = ResolvedServerConfig( + name="playwright", + source="project", + config=LocalMcpServerConfig( + command="npx", args=["-y", "@playwright/mcp@1.2.3"] + ), + ) + monkeypatch.setattr( + "pmcp.tools.handlers.load_configs", lambda **_: [pinned_override] + ) + cache = self._make_cache(version="0.1.0") + client_manager = MockClientManager() + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + + probe_calls: list[Any] = [] + + async def _fake_probe(command, env=None): + probe_calls.append(command) + return (True, "ok") + + monkeypatch.setattr(gt, "_run_update_probe_command", _fake_probe) + + async def fake_get_package_version(command, args, timeout=5.0): + return ("9.9.9", "npm") # upstream's real latest -- must be ignored + + monkeypatch.setattr( + "pmcp.tools.handlers.get_package_version", fake_get_package_version + ) + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is False + assert "pinned" in result.message.lower() + assert "1.2.3" in result.message + assert "project" in result.message.lower() + assert probe_calls == [] # never even attempted + assert "disconnect_server:playwright:False" not in client_manager.events + assert cache.servers["playwright"].version == "0.1.0" + assert "playwright" not in gt._stale_check_cache + class TestInvokeErrorPaths: """Tests for gateway.invoke error handling paths.""" From fab30330e43b58cf49f4238205aba63a4628ea1d Mon Sep 17 00:00:00 2001 From: ViperJuice Date: Wed, 12 Aug 2026 07:57:39 +0000 Subject: [PATCH 6/8] docs: update CHANGELOG for config-resolution unification + pin refusal --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8954de2f..72fab8a6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -35,6 +35,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 connection -- is now written into the descriptions cache and persisted to disk, so both the notice and the offline tool listing stay accurate across restarts. + - A third bug in the same area: `update_server` resolved its probe target + (the command it version-checks and updates) via a manifest/discovered-only + lookup, while the restart above resolves via the same precedence + `gateway.restart_server` uses, where a `.mcp.json` entry overrides the + manifest. For a server configured in both places -- README documents + pinning a server's exact version via `.mcp.json` as the supported + override channel -- this meant probing and version-checking one command + while restarting a different one. Concretely: manifest has an unpinned + `npx -y context7`, `.mcp.json` pins `npx -y context7@1.2.3`; the probe + detects/installs upstream's real latest, the restart activates the + *pinned* 1.2.3 process, and the (now-fixed) bookkeeping above would + record the probed "latest" as current -- silently misreporting the + pinned server's version. Both steps now resolve through the identical + function, so they can never disagree, and a server whose effective + config pins a concrete version is refused outright (`ok: false`, + explaining the pin and which config file it came from) rather than + probed and silently mis-recorded. ## [2.1.0] - 2026-08-12 From 2c1b440f6845c7ff98927cf21503c3bbfa8f231f Mon Sep 17 00:00:00 2001 From: ViperJuice Date: Wed, 12 Aug 2026 14:24:30 +0000 Subject: [PATCH 7/8] fix: extend update_server pin refusal to docker tags and cargo --version Board review round 3 finding 2: detect_package_type strips the :tag off a docker image reference, so "docker run acme/server:1.2.3" would pull acme/server:latest, restart the still-pinned 1.2.3 config, and record the new digest as active -- the same silent misreporting the npm pin gate already prevented. Factors the docker image scan out of detect_package_type (_docker_image_arg) so pin detection and package detection cannot disagree, adds _docker_image_tag (final path segment only, so registry:5000/img is not misread as a tag of 5000), and covers cargo's separate --version pin. --- src/pmcp/manifest/version_checker.py | 104 +++++++++++++++++---------- src/pmcp/tools/handlers.py | 26 +++++++ tests/test_tools.py | 83 +++++++++++++++++++++ 3 files changed, 174 insertions(+), 39 deletions(-) diff --git a/src/pmcp/manifest/version_checker.py b/src/pmcp/manifest/version_checker.py index a67eaa88..062d3107 100644 --- a/src/pmcp/manifest/version_checker.py +++ b/src/pmcp/manifest/version_checker.py @@ -312,51 +312,77 @@ def detect_package_type( i += 1 elif command == "docker": - # docker run [options] image[:tag] [cmd...] - _value_flags = { - "-e", - "--env", - "-v", - "--volume", - "-p", - "--publish", - "--name", - "--network", - "-u", - "--user", - "--entrypoint", - "-w", - "--workdir", - "--label", - "-l", - "--memory", - "-m", - "--cpus", - "--add-host", - "--dns", - "--hostname", - "-h", - } - skip_next = False - for arg in args: - if skip_next: - skip_next = False - continue - if arg in ("run", "exec", "start", "create", "pull", "push"): - continue - if arg in _value_flags: - skip_next = True - continue - if arg.startswith("-"): - continue - # First positional arg after subcommand is the image name; strip tag - image = arg.split(":")[0] + raw = _docker_image_arg(args) + if raw is not None: + # Strip the tag: docker run [options] image[:tag] [cmd...] + image = raw.split(":")[0] if image: return ("docker", image) return ("unknown", None) +def _docker_image_arg(args: list[str]) -> str | None: + """Return the raw ``docker`` image token from *args*, or ``None``. + + "Raw" means with any ``:tag`` suffix still attached -- factored out of + ``detect_package_type``'s docker branch for the same reason + ``_npm_package_arg`` was: gateway.update_server's pin detection needs the + untouched token, and re-implementing this scan separately would risk the + two disagreeing about which argument is "the image". + """ + _value_flags = { + "-e", + "--env", + "-v", + "--volume", + "-p", + "--publish", + "--name", + "--network", + "-u", + "--user", + "--entrypoint", + "-w", + "--workdir", + "--label", + "-l", + "--memory", + "-m", + "--cpus", + "--add-host", + "--dns", + "--hostname", + "-h", + } + skip_next = False + for arg in args: + if skip_next: + skip_next = False + continue + if arg in ("run", "exec", "start", "create", "pull", "push"): + continue + if arg in _value_flags: + skip_next = True + continue + if arg.startswith("-"): + continue + # First positional arg after the subcommand is the image reference. + return arg + return None + + +def _docker_image_tag(image_ref: str) -> str | None: + """Return the ``:tag`` on a docker image reference, or ``None``. + + Only the final path segment can carry a tag, so a registry host with a + port (``registry:5000/img:1.2.3``) does not read as a tag of ``5000``. + """ + last_segment = image_ref.rsplit("/", 1)[-1] + name, sep, tag = last_segment.partition(":") + return tag if sep and name and tag else None + + async def get_package_version( command: str, args: list[str], timeout: float = 10.0 ) -> tuple[str | None, Literal["npm", "pypi", "cargo", "docker", "unknown"]]: diff --git a/src/pmcp/tools/handlers.py b/src/pmcp/tools/handlers.py index b0916271..8858cba4 100644 --- a/src/pmcp/tools/handlers.py +++ b/src/pmcp/tools/handlers.py @@ -75,6 +75,8 @@ ) from pmcp.manifest.refresher import save_descriptions_cache from pmcp.manifest.version_checker import ( + _docker_image_arg, + _docker_image_tag, _npm_package_arg, _npm_tag, detect_package_type, @@ -290,6 +292,17 @@ def _detect_effective_version_pin( failed fetch" gate already protects against) -- this check exists only to turn that into a clear, specific refusal message instead of an opaque "could not fetch latest version" one. + + docker: ``detect_package_type`` strips the ``:tag`` off the image + reference, so without this branch ``docker run acme/server:1.2.3`` would + pull ``acme/server:latest``, restart the still-pinned ``1.2.3`` config, + and then record the latest digest as active -- the same silent + misreporting the npm branch above exists to prevent (board review round + 3). Uses ``_docker_image_arg``/``_docker_image_tag`` so a registry host + with a port (``registry:5000/img``) is not misread as a tag. + + cargo: ``cargo install`` pins with a separate ``--version X`` flag rather + than an inline suffix, so it needs its own scan. """ if package_type == "npm": raw = _npm_package_arg(args) @@ -304,6 +317,19 @@ def _detect_effective_version_pin( if "==" in arg: _, _, version = arg.partition("==") return version or None + if package_type == "docker": + raw_image = _docker_image_arg(args) + if raw_image is None: + return None + tag = _docker_image_tag(raw_image) + return None if not tag or tag == "latest" else tag + if package_type == "cargo": + for index, arg in enumerate(args): + if arg.startswith("--version="): + _, _, version = arg.partition("=") + return version or None + if arg == "--version" and index + 1 < len(args): + return args[index + 1] or None return None diff --git a/tests/test_tools.py b/tests/test_tools.py index 6463094d..2a905de9 100644 --- a/tests/test_tools.py +++ b/tests/test_tools.py @@ -5429,6 +5429,89 @@ async def fake_get_package_version(command, args, timeout=5.0): assert cache.servers["playwright"].version == "0.1.0" assert "playwright" not in gt._stale_check_cache + @pytest.mark.asyncio + async def test_update_server_refuses_pinned_docker_tag( + self, monkeypatch: pytest.MonkeyPatch + ): + """A ``:tag``-pinned docker image must refuse, exactly like an npm pin. + + Board review round 3: ``detect_package_type`` strips the tag off a + docker image reference (``image = raw.split(":")[0]``), so without a + docker branch in ``_detect_effective_version_pin`` this tool would + ``docker pull acme/server:latest``, restart the still-pinned + ``1.2.3`` config, and then record the freshly-pulled digest as the + running version -- the same silent misreporting the npm pin gate + exists to prevent, just via a different package manager. + """ + self._make_manifest_with_playwright(monkeypatch) + pinned_override = ResolvedServerConfig( + name="playwright", + source="project", + config=LocalMcpServerConfig( + command="docker", args=["run", "--rm", "acme/server:1.2.3"] + ), + ) + monkeypatch.setattr( + "pmcp.tools.handlers.load_configs", lambda **_: [pinned_override] + ) + cache = self._make_cache(version="0.1.0") + client_manager = MockClientManager() + gt = GatewayTools( + client_manager=client_manager, # type: ignore + policy_manager=PolicyManager(), + descriptions_cache=cache, + ) + + probe_calls: list[Any] = [] + + async def _fake_probe(command, env=None): + probe_calls.append(command) + return (True, "ok") + + monkeypatch.setattr(gt, "_run_update_probe_command", _fake_probe) + + async def fake_get_package_version(command, args, timeout=5.0): + return ("sha256:beefbeef", "docker") # must be ignored + + monkeypatch.setattr( + "pmcp.tools.handlers.get_package_version", fake_get_package_version + ) + + result = await gt.update_server({"server_name": "playwright"}) + + assert result.ok is False + assert "pinned" in result.message.lower() + assert "1.2.3" in result.message + assert probe_calls == [] # never even attempted + assert cache.servers["playwright"].version == "0.1.0" + assert "playwright" not in gt._stale_check_cache + + def test_detect_effective_version_pin_matrix(self): + """Pin detection per package manager, including the not-a-pin cases.""" + from pmcp.tools.handlers import _detect_effective_version_pin as detect + + # npm: explicit version pins, @latest is not a pin, bare is not a pin. + assert detect("npm", "npx", ["-y", "@playwright/mcp@1.2.3"]) == "1.2.3" + assert detect("npm", "npx", ["-y", "@playwright/mcp@latest"]) is None + assert detect("npm", "npx", ["-y", "@playwright/mcp"]) is None + + # docker: only the final path segment can carry a tag, so a registry + # host with a port must not read as a pin of "5000". + assert detect("docker", "docker", ["run", "acme/server:1.2.3"]) == "1.2.3" + assert detect("docker", "docker", ["run", "acme/server:latest"]) is None + assert detect("docker", "docker", ["run", "acme/server"]) is None + assert detect("docker", "docker", ["run", "registry:5000/img"]) is None + assert detect("docker", "docker", ["run", "registry:5000/img:2.0"]) == "2.0" + + # cargo: pins via a separate --version flag, both spellings. + assert detect("cargo", "cargo", ["install", "srv", "--version", "1.0"]) == "1.0" + assert detect("cargo", "cargo", ["install", "srv", "--version=1.0"]) == "1.0" + assert detect("cargo", "cargo", ["install", "srv"]) is None + + # pypi/uvx inline == pin. + assert detect("pypi", "uvx", ["srv==1.2.3"]) == "1.2.3" + assert detect("pypi", "uvx", ["srv"]) is None + class TestInvokeErrorPaths: """Tests for gateway.invoke error handling paths.""" From f2f229ac6ac991da5110bd2f9160b2d6559e952e Mon Sep 17 00:00:00 2001 From: ViperJuice Date: Wed, 12 Aug 2026 14:30:47 +0000 Subject: [PATCH 8/8] docs: note docker/cargo pin refusal in CHANGELOG --- CHANGELOG.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72fab8a6..ecd06763 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,7 +51,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 function, so they can never disagree, and a server whose effective config pins a concrete version is refused outright (`ok: false`, explaining the pin and which config file it came from) rather than - probed and silently mis-recorded. + probed and silently mis-recorded. Pin detection covers every package + manager the tool can update, not just npm: a `:tag`-pinned docker image + and a `cargo install --version` pin are refused the same way. Docker + needed its own handling because package detection strips the tag off the + image reference, so `docker run acme/server:1.2.3` would otherwise pull + `acme/server:latest`, restart the still-pinned config, and record the new + digest as active. ## [2.1.0] - 2026-08-12