From 97ef06ac5c98d269252094f7b69f5703ac64b4ca Mon Sep 17 00:00:00 2001 From: Beon de Nood Date: Wed, 11 Mar 2026 15:27:44 -0400 Subject: [PATCH 1/2] fix: improve binary install experience - Add visible stderr output during binary download (P0) - Fix CORE_MIN_VERSION 2.3.0 -> 2.4.0 to match SDK (P0) - Fix hardcoded version string in health check error message (P0) - Align cache directory to ~/.capiscio/bin/ (shared with SDK) (P1) - Remove unused platformdirs dependency (P1) - Add retry with exponential backoff for download (3 attempts) (P1) --- capiscio_mcp/_core/health.py | 4 +- capiscio_mcp/_core/lifecycle.py | 85 +++++++++++++++++++++------------ capiscio_mcp/_core/version.py | 5 +- pyproject.toml | 1 - 4 files changed, 59 insertions(+), 36 deletions(-) diff --git a/capiscio_mcp/_core/health.py b/capiscio_mcp/_core/health.py index 1d36fbb..5c86adb 100644 --- a/capiscio_mcp/_core/health.py +++ b/capiscio_mcp/_core/health.py @@ -8,7 +8,7 @@ import logging from typing import TYPE_CHECKING -from capiscio_mcp._core.version import is_core_compatible, PROTO_VERSION +from capiscio_mcp._core.version import is_core_compatible, PROTO_VERSION, CORE_MIN_VERSION, CORE_MAX_VERSION from capiscio_mcp.errors import CoreConnectionError, CoreVersionError if TYPE_CHECKING: @@ -54,7 +54,7 @@ async def check_version_compatibility( if not is_core_compatible(response.core_version): raise CoreVersionError( f"capiscio-core version {response.core_version} is not compatible. " - f"This SDK requires core version >= 2.5.0 and < 3.0.0" + f"This SDK requires core version >= {CORE_MIN_VERSION} and < {CORE_MAX_VERSION}" ) # Check proto version diff --git a/capiscio_mcp/_core/lifecycle.py b/capiscio_mcp/_core/lifecycle.py index f2b3583..de43cc1 100644 --- a/capiscio_mcp/_core/lifecycle.py +++ b/capiscio_mcp/_core/lifecycle.py @@ -18,8 +18,10 @@ from pathlib import Path from typing import Optional, Tuple +import sys +import time + import requests -from platformdirs import user_cache_dir from capiscio_mcp._core.version import ( CORE_MIN_VERSION, @@ -72,8 +74,11 @@ def get_platform_info() -> Tuple[str, str]: def get_cache_dir() -> Path: - """Get the directory where binaries are cached.""" - cache_dir = Path(user_cache_dir("capiscio-mcp", "capiscio")) / "bin" + """Get the directory where binaries are cached. + + Uses ~/.capiscio/bin/ to share cache with capiscio-sdk-python. + """ + cache_dir = Path.home() / ".capiscio" / "bin" cache_dir.mkdir(parents=True, exist_ok=True) return cache_dir @@ -118,36 +123,54 @@ def download_binary(version: Optional[str] = None) -> Path: os_name, arch_name = get_platform_info() url = get_download_url(version, os_name, arch_name) + sys.stderr.write( + f"capiscio-core v{version} not found. " + f"Downloading for {os_name}/{arch_name}...\n" + ) + sys.stderr.flush() logger.info(f"Downloading capiscio-core v{version} for {os_name}/{arch_name}...") - try: - response = requests.get(url, stream=True, timeout=60) - response.raise_for_status() - - # Ensure directory exists - target_path.parent.mkdir(parents=True, exist_ok=True) - - # Write binary - with open(target_path, "wb") as f: - for chunk in response.iter_content(chunk_size=8192): - f.write(chunk) - - # Make executable (Unix) - if os_name != "windows": - st = os.stat(target_path) - os.chmod(target_path, st.st_mode | stat.S_IEXEC) - - logger.info(f"Successfully installed capiscio-core v{version}") - return target_path - - except requests.exceptions.RequestException as e: - if target_path.exists(): - target_path.unlink() - raise CoreConnectionError(f"Failed to download binary from {url}: {e}") from e - except Exception as e: - if target_path.exists(): - target_path.unlink() - raise CoreConnectionError(f"Failed to install binary: {e}") from e + # Ensure directory exists + target_path.parent.mkdir(parents=True, exist_ok=True) + + max_attempts = 3 + for attempt in range(1, max_attempts + 1): + try: + response = requests.get(url, stream=True, timeout=60) + response.raise_for_status() + + # Write binary + with open(target_path, "wb") as f: + for chunk in response.iter_content(chunk_size=8192): + f.write(chunk) + + # Make executable (Unix) + if os_name != "windows": + st = os.stat(target_path) + os.chmod(target_path, st.st_mode | stat.S_IEXEC) + + sys.stderr.write(f"Installed capiscio-core v{version} at {target_path}\n") + sys.stderr.flush() + logger.info(f"Successfully installed capiscio-core v{version}") + return target_path + + except Exception as e: + if target_path.exists(): + target_path.unlink() + if attempt < max_attempts: + delay = 2 ** (attempt - 1) + logger.warning( + f"Download attempt {attempt}/{max_attempts} failed: {e}. " + f"Retrying in {delay}s..." + ) + time.sleep(delay) + else: + raise CoreConnectionError( + f"Failed to download binary from {url} " + f"after {max_attempts} attempts: {e}" + ) from e + # unreachable, but keeps type checker happy + raise CoreConnectionError("Download failed") async def ensure_binary(version: Optional[str] = None) -> Path: diff --git a/capiscio_mcp/_core/version.py b/capiscio_mcp/_core/version.py index f0a0d4f..99072cc 100644 --- a/capiscio_mcp/_core/version.py +++ b/capiscio_mcp/_core/version.py @@ -16,8 +16,9 @@ MCP_VERSION = "0.1.0" # Compatible capiscio-core versions (internal constraint) -# Note: MCP integration was added in 2.3.1 -CORE_MIN_VERSION = "2.3.0" +# Note: MCP integration was added in 2.3.1, but 2.4.0 is required +# for full compatibility with the current SDK. +CORE_MIN_VERSION = "2.4.0" CORE_MAX_VERSION = "3.0.0" # exclusive # Proto schema version for wire compatibility diff --git a/pyproject.toml b/pyproject.toml index 785c854..28547ac 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,7 +31,6 @@ dependencies = [ "grpcio-tools>=1.60.0", "protobuf>=4.25.0", "requests>=2.31.0", - "platformdirs>=4.0.0", "cryptography>=42.0.0", "base58>=2.1.0", ] From 1a4ed077a3889094cba17d4bfea4fc9788484af0 Mon Sep 17 00:00:00 2001 From: Beon de Nood Date: Wed, 11 Mar 2026 16:19:24 -0400 Subject: [PATCH 2/2] fix: address copilot review comments on PR #8 - Replace sys.stderr.write with logger.info for download messages - Use requests.get as context manager to prevent connection leaks - Only retry on transient errors (5xx/timeout/connection); fail fast on 4xx - Wrap long import line in health.py for Black compliance - Remove unused sys import - Update 404 test mock for context manager + fast-fail behavior --- capiscio_mcp/_core/health.py | 7 ++++- capiscio_mcp/_core/lifecycle.py | 47 ++++++++++++++++++++++----------- tests/test_core_lifecycle.py | 8 ++++-- 3 files changed, 43 insertions(+), 19 deletions(-) diff --git a/capiscio_mcp/_core/health.py b/capiscio_mcp/_core/health.py index 5c86adb..74f4e6b 100644 --- a/capiscio_mcp/_core/health.py +++ b/capiscio_mcp/_core/health.py @@ -8,7 +8,12 @@ import logging from typing import TYPE_CHECKING -from capiscio_mcp._core.version import is_core_compatible, PROTO_VERSION, CORE_MIN_VERSION, CORE_MAX_VERSION +from capiscio_mcp._core.version import ( + is_core_compatible, + PROTO_VERSION, + CORE_MIN_VERSION, + CORE_MAX_VERSION, +) from capiscio_mcp.errors import CoreConnectionError, CoreVersionError if TYPE_CHECKING: diff --git a/capiscio_mcp/_core/lifecycle.py b/capiscio_mcp/_core/lifecycle.py index de43cc1..ff72b06 100644 --- a/capiscio_mcp/_core/lifecycle.py +++ b/capiscio_mcp/_core/lifecycle.py @@ -18,7 +18,6 @@ from pathlib import Path from typing import Optional, Tuple -import sys import time import requests @@ -123,12 +122,10 @@ def download_binary(version: Optional[str] = None) -> Path: os_name, arch_name = get_platform_info() url = get_download_url(version, os_name, arch_name) - sys.stderr.write( + logger.info( f"capiscio-core v{version} not found. " - f"Downloading for {os_name}/{arch_name}...\n" + f"Downloading for {os_name}/{arch_name}..." ) - sys.stderr.flush() - logger.info(f"Downloading capiscio-core v{version} for {os_name}/{arch_name}...") # Ensure directory exists target_path.parent.mkdir(parents=True, exist_ok=True) @@ -136,25 +133,43 @@ def download_binary(version: Optional[str] = None) -> Path: max_attempts = 3 for attempt in range(1, max_attempts + 1): try: - response = requests.get(url, stream=True, timeout=60) - response.raise_for_status() - - # Write binary - with open(target_path, "wb") as f: - for chunk in response.iter_content(chunk_size=8192): - f.write(chunk) + with requests.get(url, stream=True, timeout=60) as response: + response.raise_for_status() + + # Write binary + with open(target_path, "wb") as f: + for chunk in response.iter_content(chunk_size=8192): + f.write(chunk) # Make executable (Unix) if os_name != "windows": st = os.stat(target_path) os.chmod(target_path, st.st_mode | stat.S_IEXEC) - sys.stderr.write(f"Installed capiscio-core v{version} at {target_path}\n") - sys.stderr.flush() - logger.info(f"Successfully installed capiscio-core v{version}") + logger.info(f"Installed capiscio-core v{version} at {target_path}") return target_path - except Exception as e: + except requests.exceptions.HTTPError as e: + if target_path.exists(): + target_path.unlink() + # Fail fast on client errors (4xx) — not transient + if e.response is not None and 400 <= e.response.status_code < 500: + raise CoreConnectionError( + f"Failed to download binary from {url}: {e}" + ) from e + if attempt < max_attempts: + delay = 2 ** (attempt - 1) + logger.warning( + f"Download attempt {attempt}/{max_attempts} failed: {e}. " + f"Retrying in {delay}s..." + ) + time.sleep(delay) + else: + raise CoreConnectionError( + f"Failed to download binary from {url} " + f"after {max_attempts} attempts: {e}" + ) from e + except (requests.exceptions.ConnectionError, requests.exceptions.Timeout, OSError) as e: if target_path.exists(): target_path.unlink() if attempt < max_attempts: diff --git a/tests/test_core_lifecycle.py b/tests/test_core_lifecycle.py index b369c69..aa6d1fa 100644 --- a/tests/test_core_lifecycle.py +++ b/tests/test_core_lifecycle.py @@ -181,7 +181,7 @@ def test_download_success(self, mock_path, mock_get): @patch("requests.get") @patch("capiscio_mcp._core.lifecycle.get_binary_path") def test_download_404_error(self, mock_path, mock_get): - """404 response should raise error.""" + """404 response should raise error immediately (no retry).""" import requests with tempfile.TemporaryDirectory() as tmpdir: @@ -190,7 +190,11 @@ def test_download_404_error(self, mock_path, mock_get): mock_response = MagicMock() mock_response.status_code = 404 - mock_response.raise_for_status.side_effect = requests.exceptions.HTTPError("404") + http_err = requests.exceptions.HTTPError("404") + http_err.response = mock_response + mock_response.raise_for_status.side_effect = http_err + mock_response.__enter__ = MagicMock(return_value=mock_response) + mock_response.__exit__ = MagicMock(return_value=False) mock_get.return_value = mock_response from capiscio_mcp.errors import CoreConnectionError