From a71198a508d20b312cd434125a784c8b2ddfe5f7 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 09:49:52 -0600 Subject: [PATCH 01/10] extensions module, entry point stubs --- .../openhands/sdk/extensions/__init__.py | 0 .../openhands/sdk/extensions/fetch.py | 76 +++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 openhands-sdk/openhands/sdk/extensions/__init__.py create mode 100644 openhands-sdk/openhands/sdk/extensions/fetch.py diff --git a/openhands-sdk/openhands/sdk/extensions/__init__.py b/openhands-sdk/openhands/sdk/extensions/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/openhands-sdk/openhands/sdk/extensions/fetch.py b/openhands-sdk/openhands/sdk/extensions/fetch.py new file mode 100644 index 0000000000..8209cf5f96 --- /dev/null +++ b/openhands-sdk/openhands/sdk/extensions/fetch.py @@ -0,0 +1,76 @@ +"""Fetching utilities for extensions.""" + +from __future__ import annotations + +from pathlib import Path + +from openhands.sdk.git.cached_repo import GitHelper, try_cached_clone_or_update +from openhands.sdk.git.utils import extract_repo_name, is_git_url, normalize_git_url +from openhands.sdk.logger import get_logger + + +logger = get_logger(__name__) + + +class ExtensionFetchError(Exception): + """Raised when fetching an extension fails.""" + + +def fetch( + source: str, + cache_dir: Path | None = None, + ref: str | None = None, + update: bool = True, + repo_path: str | None = None, + git_helper: GitHelper | None = None, +) -> Path: + """Fetch an extension from a source and return the local path. + + Args: + source: Extension source -- git URL, GitHub shorthand, or local path. + cache_dir: Directory for caching. + ref: Optional branch, tag, or commit to checkout. + update: If true and cache exists, update it. + repo_path: Subdirectory path within the repository. + git_helper: GitHelper instance (for testing). + + Returns: + Path to the local extension directory. + """ + path, _ = fetch_with_resolution( + source=source, + cache_dir=cache_dir, + ref=ref, + update=update, + repo_path=repo_path, + git_helper=git_helper, + ) + return path + + +def fetch_with_resolution( + source: str, + cache_dir: Path | None = None, + ref: str | None = None, + update: bool = True, + repo_path: str | None = None, + git_helper: GitHelper | None = None, +) -> tuple[Path, str | None]: + """Fetch an extension and return both the path and resolved commit SHA. + + Args: + source: Extension source (git URL, GitHub shorthand, or local path). + cache_dir: Directory for caching. + ref: Optional branch, tag, or commit to checkout. + update: If True and cache exists, update it. + repo_path: Subdirectory path within the repository. + git_helper: GitHelper instance (for testing). + + Returns: + Tuple of (path, resolved_ref) where resolved_ref is the commit SHA for git + sources and None for local paths. + + Raises: + ExtensionFetchError: If fetching the extension fails. + """ + raise NotImplementedError() From 4858e59ff16fb9b4ebe5f3b963a711d78577b408 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 10:27:06 -0600 Subject: [PATCH 02/10] full fetch logic --- .../openhands/sdk/extensions/fetch.py | 203 +++++++++++++++++- 1 file changed, 200 insertions(+), 3 deletions(-) diff --git a/openhands-sdk/openhands/sdk/extensions/fetch.py b/openhands-sdk/openhands/sdk/extensions/fetch.py index 8209cf5f96..22e0716730 100644 --- a/openhands-sdk/openhands/sdk/extensions/fetch.py +++ b/openhands-sdk/openhands/sdk/extensions/fetch.py @@ -2,6 +2,8 @@ from __future__ import annotations +import hashlib +from enum import Enum from pathlib import Path from openhands.sdk.git.cached_repo import GitHelper, try_cached_clone_or_update @@ -16,9 +18,114 @@ class ExtensionFetchError(Exception): """Raised when fetching an extension fails.""" +class SourceType(str, Enum): + LOCAL = "local" + GIT = "git" + GITHUB = "github" + + +def parse_extension_source(source: str) -> tuple[SourceType, str]: + """Parse extension source into (SourceType, url). + + Args: + source: Plugin source string. Can be: + - "github:owner/repo" - GitHub repository shorthand + - "https://github.com/owner/repo.git" - Full git URL + - "git@github.com:owner/repo.git" - SSH git URL + - "/local/path" - Local path + + Returns: + Tuple of (source_type, normalized_url) where source_type is one of: + - "github": GitHub repository + - "git": Any git URL + - "local": Local filesystem path + + Examples: + >>> parse_plugin_source("github:owner/repo") + ("github", "https://github.com/owner/repo.git") + >>> parse_plugin_source("https://gitlab.com/org/repo.git") + ("git", "https://gitlab.com/org/repo.git") + >>> parse_plugin_source("/local/path") + ("local", "/local/path") + """ + source = source.strip() + + # GitHub shorthand: github:owner/repo + if source.startswith("github:"): + repo_path = source[7:] # Remove "github:" prefix + # Validate format + if "/" not in repo_path or repo_path.count("/") > 1: + raise ExtensionFetchError( + f"Invalid GitHub shorthand format: {source}. " + f"Expected format: github:owner/repo" + ) + url = f"https://github.com/{repo_path}.git" + return (SourceType.GITHUB, url) + + # Git URLs: detect by protocol/scheme rather than enumerating providers + # This handles GitHub, GitLab, Bitbucket, Codeberg, self-hosted instances, etc. + if is_git_url(source): + url = normalize_git_url(source) + return (SourceType.GIT, url) + + # Local path: starts with /, ~, . or contains / without a URL scheme + if source.startswith(("/", "~", ".")): + return (SourceType.LOCAL, source) + + if "/" in source and "://" not in source: + # Relative path like "plugins/my-plugin" + return (SourceType.LOCAL, source) + + raise ExtensionFetchError( + f"Unable to parse extension source: {source}. " + f"Expected formats: 'github:owner/repo', git URL, or local path" + ) + + +def _resolve_local_source(url: str) -> Path: + """Resolve a local extension source to a path. + + Args: + url: Local path string (may contain ~ for home directory). + + Returns: + Resolved absolute path to the extension directory. + + Raises: + ExtensionFetchError: If path doesn't exist. + """ + local_path = Path(url).expanduser().resolve() + if not local_path.exists(): + raise ExtensionFetchError(f"Local extension path does not exist: {local_path}") + return local_path + + +def _apply_subpath(base_path: Path, subpath: str | None, context: str) -> Path: + """Apply a subpath to a base path, validating it exists. + + Args: + base_path: The root path. + subpath: Optional subdirectory path (may have leading/trailing slashes). + context: Description for error messages (e.g., "plugin repository"). + + Returns: + The final path (base_path if no subpath, otherwise base_path/subpath). + + Raises: + ExtensionFetchError: If subpath doesn't exist. + """ + if not subpath: + return base_path + + final_path = base_path / subpath.strip("/") + if not final_path.exists(): + raise ExtensionFetchError(f"Subdirectory '{subpath}' not found in {context}") + return final_path + + def fetch( source: str, - cache_dir: Path | None = None, + cache_dir: Path, ref: str | None = None, update: bool = True, repo_path: str | None = None, @@ -50,7 +157,7 @@ def fetch( def fetch_with_resolution( source: str, - cache_dir: Path | None = None, + cache_dir: Path, ref: str | None = None, update: bool = True, repo_path: str | None = None, @@ -73,4 +180,94 @@ def fetch_with_resolution( Raises: ExtensionFetchError: If fetching the extension fails. """ - raise NotImplementedError() + source_type, url = parse_extension_source(source) + + if source_type == SourceType.LOCAL: + if repo_path is not None: + raise ExtensionFetchError( + f"repo_path is not supported for local extension sources. " + f"Specify the full path directly instead of " + f"source='{source}' + repo_path='{repo_path}'" + ) + return _resolve_local_source(url), None + + git = git_helper if git_helper is not None else GitHelper() + + plugin_path, resolved_ref = _fetch_remote_source_with_resolution( + url, cache_dir, ref, update, repo_path, git, source + ) + return plugin_path, resolved_ref + + +def get_cache_path(source: str, cache_dir: Path) -> Path: + """Get the cache path for a plugin source. + + Creates a deterministic path based on a hash of the source URL. + + Args: + source: The plugin source (URL or path). + cache_dir: Base cache directory. + + Returns: + Path where the plugin should be cached. + """ + # Create a hash of the source for the directory name + source_hash = hashlib.sha256(source.encode()).hexdigest()[:16] + + # Extract repo name for human-readable cache directory name + readable_name = extract_repo_name(source) + + cache_name = f"{readable_name}-{source_hash}" + return cache_dir / cache_name + + +def _fetch_remote_source_with_resolution( + url: str, + cache_dir: Path, + ref: str | None, + update: bool, + subpath: str | None, + git_helper: GitHelper, + source: str, +) -> tuple[Path, str]: + """Fetch a remote extension source and return path + resolved commit SHA. + + Args: + url: Git URL to fetch. + cache_dir: Base directory for caching. + ref: Optional branch, tag, or commit to checkout. + update: Whether to update existing cache. + subpath: Optional subdirectory within the repository. + git_helper: GitHelper instance for git operations. + source: Original source string (for error messages). + + Returns: + Tuple of (path, resolved_ref) where resolved_ref is the commit SHA. + + Raises: + ExtensionFetchError: If fetching fails or subpath is invalid. + """ + repo_cache_path = get_cache_path(url, cache_dir) + cache_dir.mkdir(parents=True, exist_ok=True) + + result = try_cached_clone_or_update( + url=url, + repo_path=repo_cache_path, + ref=ref, + update=update, + git_helper=git_helper, + ) + + if result is None: + raise ExtensionFetchError(f"Failed to fetch extension from {source}") + + # Get the actual commit SHA that was checked out + try: + resolved_ref = git_helper.get_head_commit(repo_cache_path) + except Exception as e: + logger.warning(f"Could not get commit SHA for {source}: {e}") + # Fall back to the requested ref if we can't get the SHA + resolved_ref = ref or "HEAD" + + final_path = _apply_subpath(repo_cache_path, subpath, "extension repository") + return final_path, resolved_ref From dae2ddc9f02c44069d3f14554bcbd569843a9d33 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 10:31:36 -0600 Subject: [PATCH 03/10] Port fetch tests to tests/sdk/extensions/test_fetch.py Cover parse_extension_source, get_cache_path, fetch, fetch_with_resolution, SourceType enum, and repo_path handling for the new shared extensions module. Co-authored-by: openhands --- tests/sdk/extensions/__init__.py | 0 tests/sdk/extensions/test_fetch.py | 496 +++++++++++++++++++++++++++++ 2 files changed, 496 insertions(+) create mode 100644 tests/sdk/extensions/__init__.py create mode 100644 tests/sdk/extensions/test_fetch.py diff --git a/tests/sdk/extensions/__init__.py b/tests/sdk/extensions/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/sdk/extensions/test_fetch.py b/tests/sdk/extensions/test_fetch.py new file mode 100644 index 0000000000..dcf2b41994 --- /dev/null +++ b/tests/sdk/extensions/test_fetch.py @@ -0,0 +1,496 @@ +"""Tests for extensions fetch utilities.""" + +from pathlib import Path +from unittest.mock import create_autospec + +import pytest + +from openhands.sdk.extensions.fetch import ( + ExtensionFetchError, + SourceType, + fetch, + fetch_with_resolution, + get_cache_path, + parse_extension_source, +) +from openhands.sdk.git.cached_repo import GitHelper +from openhands.sdk.git.exceptions import GitCommandError + + +# -- parse_extension_source --------------------------------------------------- + + +def test_parse_github_shorthand(): + source_type, url = parse_extension_source("github:owner/repo") + assert source_type == SourceType.GITHUB + assert url == "https://github.com/owner/repo.git" + + +def test_parse_github_shorthand_with_whitespace(): + source_type, url = parse_extension_source(" github:owner/repo ") + assert source_type == SourceType.GITHUB + assert url == "https://github.com/owner/repo.git" + + +def test_parse_github_shorthand_invalid_format(): + with pytest.raises(ExtensionFetchError, match="Invalid GitHub shorthand"): + parse_extension_source("github:invalid") + + with pytest.raises(ExtensionFetchError, match="Invalid GitHub shorthand"): + parse_extension_source("github:too/many/parts") + + +def test_parse_https_git_url(): + source_type, url = parse_extension_source("https://github.com/owner/repo.git") + assert source_type == SourceType.GIT + assert url == "https://github.com/owner/repo.git" + + +def test_parse_https_github_url_without_git_suffix(): + source_type, url = parse_extension_source("https://github.com/owner/repo") + assert source_type == SourceType.GIT + assert url == "https://github.com/owner/repo.git" + + +def test_parse_https_github_url_with_trailing_slash(): + source_type, url = parse_extension_source("https://github.com/owner/repo/") + assert source_type == SourceType.GIT + assert url == "https://github.com/owner/repo.git" + + +def test_parse_https_gitlab_url(): + source_type, url = parse_extension_source("https://gitlab.com/org/repo") + assert source_type == SourceType.GIT + assert url == "https://gitlab.com/org/repo.git" + + +def test_parse_https_bitbucket_url(): + source_type, url = parse_extension_source("https://bitbucket.org/org/repo") + assert source_type == SourceType.GIT + assert url == "https://bitbucket.org/org/repo.git" + + +def test_parse_ssh_git_url(): + source_type, url = parse_extension_source("git@github.com:owner/repo.git") + assert source_type == SourceType.GIT + assert url == "git@github.com:owner/repo.git" + + +def test_parse_git_protocol_url(): + source_type, url = parse_extension_source("git://github.com/owner/repo.git") + assert source_type == SourceType.GIT + assert url == "git://github.com/owner/repo.git" + + +def test_parse_absolute_local_path(): + source_type, url = parse_extension_source("/path/to/extension") + assert source_type == SourceType.LOCAL + assert url == "/path/to/extension" + + +def test_parse_home_relative_path(): + source_type, url = parse_extension_source("~/extensions/my-ext") + assert source_type == SourceType.LOCAL + assert url == "~/extensions/my-ext" + + +def test_parse_dot_relative_path(): + source_type, url = parse_extension_source("./extensions/my-ext") + assert source_type == SourceType.LOCAL + assert url == "./extensions/my-ext" + + +def test_parse_invalid_source(): + with pytest.raises(ExtensionFetchError, match="Unable to parse extension source"): + parse_extension_source("invalid-source-format") + + +def test_parse_self_hosted_git_urls(): + source_type, url = parse_extension_source("https://codeberg.org/user/repo") + assert source_type == SourceType.GIT + assert url == "https://codeberg.org/user/repo.git" + + source_type, url = parse_extension_source("https://git.mycompany.com/org/repo") + assert source_type == SourceType.GIT + assert url == "https://git.mycompany.com/org/repo.git" + + +def test_parse_http_url(): + source_type, url = parse_extension_source("http://internal-git.local/repo") + assert source_type == SourceType.GIT + assert url == "http://internal-git.local/repo.git" + + +def test_parse_ssh_with_custom_user(): + ssh_url = "deploy@git.example.com:project/repo.git" + source_type, url = parse_extension_source(ssh_url) + assert source_type == SourceType.GIT + assert url == ssh_url + + +def test_parse_relative_path_with_slash(): + source_type, url = parse_extension_source("extensions/my-ext") + assert source_type == SourceType.LOCAL + assert url == "extensions/my-ext" + + +def test_parse_nested_relative_path(): + source_type, url = parse_extension_source("path/to/my/extension") + assert source_type == SourceType.LOCAL + assert url == "path/to/my/extension" + + +# -- SourceType enum ---------------------------------------------------------- + + +def test_source_type_values(): + assert SourceType.LOCAL == "local" + assert SourceType.GIT == "git" + assert SourceType.GITHUB == "github" + + +# -- get_cache_path ------------------------------------------------------------ + + +def test_cache_path_deterministic(tmp_path: Path): + source = "https://github.com/owner/repo.git" + path1 = get_cache_path(source, tmp_path) + path2 = get_cache_path(source, tmp_path) + assert path1 == path2 + + +def test_cache_path_different_sources(tmp_path: Path): + path1 = get_cache_path("https://github.com/owner/repo1.git", tmp_path) + path2 = get_cache_path("https://github.com/owner/repo2.git", tmp_path) + assert path1 != path2 + + +def test_cache_path_includes_readable_name(tmp_path: Path): + source = "https://github.com/owner/my-extension.git" + path = get_cache_path(source, tmp_path) + assert "my-extension" in path.name + + +# -- fetch (local sources) ---------------------------------------------------- + + +def test_fetch_local_path(tmp_path: Path): + ext_dir = tmp_path / "my-ext" + ext_dir.mkdir() + + result = fetch(str(ext_dir), cache_dir=tmp_path) + assert result == ext_dir.resolve() + + +def test_fetch_local_path_nonexistent(tmp_path: Path): + with pytest.raises(ExtensionFetchError, match="does not exist"): + fetch(str(tmp_path / "nonexistent"), cache_dir=tmp_path) + + +# -- fetch (remote sources) --------------------------------------------------- + + +def test_fetch_github_shorthand_clones(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + + mock_git.clone.side_effect = clone_side_effect + + result = fetch( + "github:owner/repo", + cache_dir=tmp_path, + git_helper=mock_git, + ) + + assert result.exists() + mock_git.clone.assert_called_once() + call_args = mock_git.clone.call_args + assert call_args[0][0] == "https://github.com/owner/repo.git" + + +def test_fetch_with_ref(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + + mock_git.clone.side_effect = clone_side_effect + + fetch( + "github:owner/repo", + cache_dir=tmp_path, + ref="v1.0.0", + git_helper=mock_git, + ) + + mock_git.clone.assert_called_once() + call_kwargs = mock_git.clone.call_args[1] + assert call_kwargs["branch"] == "v1.0.0" + + +def test_fetch_updates_existing_cache(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + mock_git.get_current_branch.return_value = "main" + + cache_path = get_cache_path("https://github.com/owner/repo.git", tmp_path) + cache_path.mkdir(parents=True) + (cache_path / ".git").mkdir() + + result = fetch( + "github:owner/repo", + cache_dir=tmp_path, + update=True, + git_helper=mock_git, + ) + + assert result == cache_path + mock_git.fetch.assert_called() + mock_git.clone.assert_not_called() + + +def test_fetch_no_update_uses_cache(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + cache_path = get_cache_path("https://github.com/owner/repo.git", tmp_path) + cache_path.mkdir(parents=True) + (cache_path / ".git").mkdir() + + result = fetch( + "github:owner/repo", + cache_dir=tmp_path, + update=False, + git_helper=mock_git, + ) + + assert result == cache_path + mock_git.clone.assert_not_called() + mock_git.fetch.assert_not_called() + + +def test_fetch_no_update_with_ref_checks_out(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + cache_path = get_cache_path("https://github.com/owner/repo.git", tmp_path) + cache_path.mkdir(parents=True) + (cache_path / ".git").mkdir() + + fetch( + "github:owner/repo", + cache_dir=tmp_path, + update=False, + ref="v1.0.0", + git_helper=mock_git, + ) + + mock_git.checkout.assert_called_once_with(cache_path, "v1.0.0") + + +def test_fetch_git_error_raises_extension_fetch_error(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + mock_git.clone.side_effect = GitCommandError( + "fatal: repository not found", + command=["git", "clone"], + exit_code=128, + ) + + with pytest.raises(ExtensionFetchError, match="Failed to fetch extension"): + fetch( + "github:owner/nonexistent", + cache_dir=tmp_path, + git_helper=mock_git, + ) + + +def test_fetch_generic_error_raises_extension_fetch_error(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + mock_git.clone.side_effect = RuntimeError("Unexpected error") + + with pytest.raises(ExtensionFetchError, match="Failed to fetch extension"): + fetch( + "github:owner/repo", + cache_dir=tmp_path, + git_helper=mock_git, + ) + + +# -- fetch_with_resolution ---------------------------------------------------- + + +def test_fetch_with_resolution_local_returns_none_ref(tmp_path: Path): + ext_dir = tmp_path / "my-ext" + ext_dir.mkdir() + + path, resolved_ref = fetch_with_resolution(str(ext_dir), cache_dir=tmp_path) + assert path == ext_dir.resolve() + assert resolved_ref is None + + +def test_fetch_with_resolution_remote_returns_sha(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + + mock_git.clone.side_effect = clone_side_effect + mock_git.get_head_commit.return_value = "abc123deadbeef" + + path, resolved_ref = fetch_with_resolution( + "github:owner/repo", + cache_dir=tmp_path, + git_helper=mock_git, + ) + + assert path.exists() + assert resolved_ref == "abc123deadbeef" + + +def test_fetch_with_resolution_falls_back_on_sha_error(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + + mock_git.clone.side_effect = clone_side_effect + mock_git.get_head_commit.side_effect = RuntimeError("not a git repo") + + path, resolved_ref = fetch_with_resolution( + "github:owner/repo", + cache_dir=tmp_path, + ref="v2.0", + git_helper=mock_git, + ) + + assert path.exists() + assert resolved_ref == "v2.0" + + +def test_fetch_with_resolution_falls_back_to_head(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + + mock_git.clone.side_effect = clone_side_effect + mock_git.get_head_commit.side_effect = RuntimeError("not a git repo") + + path, resolved_ref = fetch_with_resolution( + "github:owner/repo", + cache_dir=tmp_path, + git_helper=mock_git, + ) + + assert path.exists() + assert resolved_ref == "HEAD" + + +# -- repo_path parameter ------------------------------------------------------ + + +def test_fetch_local_with_repo_path_raises_error(tmp_path: Path): + ext_dir = tmp_path / "monorepo" + ext_dir.mkdir() + (ext_dir / "extensions" / "my-ext").mkdir(parents=True) + + with pytest.raises( + ExtensionFetchError, + match="repo_path is not supported for local", + ): + fetch( + str(ext_dir), + cache_dir=tmp_path, + repo_path="extensions/my-ext", + ) + + +def test_fetch_github_with_repo_path(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + subdir = dest / "extensions" / "sub-ext" + subdir.mkdir(parents=True) + + mock_git.clone.side_effect = clone_side_effect + + result = fetch( + "github:owner/monorepo", + cache_dir=tmp_path, + repo_path="extensions/sub-ext", + git_helper=mock_git, + ) + + assert result.exists() + assert result.name == "sub-ext" + assert "extensions" in str(result) + + +def test_fetch_github_with_nonexistent_repo_path(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + + mock_git.clone.side_effect = clone_side_effect + + with pytest.raises(ExtensionFetchError, match="Subdirectory.*not found"): + fetch( + "github:owner/repo", + cache_dir=tmp_path, + repo_path="nonexistent", + git_helper=mock_git, + ) + + +def test_fetch_with_repo_path_and_ref(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + subdir = dest / "extensions" / "my-ext" + subdir.mkdir(parents=True) + + mock_git.clone.side_effect = clone_side_effect + + result = fetch( + "github:owner/monorepo", + cache_dir=tmp_path, + ref="v1.0.0", + repo_path="extensions/my-ext", + git_helper=mock_git, + ) + + assert result.exists() + mock_git.clone.assert_called_once() + call_kwargs = mock_git.clone.call_args[1] + assert call_kwargs["branch"] == "v1.0.0" + + +def test_fetch_no_repo_path_returns_root(tmp_path: Path): + mock_git = create_autospec(GitHelper, instance=True) + + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() + (dest / "extensions").mkdir() + + mock_git.clone.side_effect = clone_side_effect + + result = fetch( + "github:owner/repo", + cache_dir=tmp_path, + repo_path=None, + git_helper=mock_git, + ) + + assert result.exists() + assert (result / ".git").exists() From 5187d874af5d28cf0e12bb34665860ea9a35d657 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 10:32:44 -0600 Subject: [PATCH 04/10] minor --- openhands-sdk/openhands/sdk/extensions/fetch.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/extensions/fetch.py b/openhands-sdk/openhands/sdk/extensions/fetch.py index 22e0716730..8efb272c8e 100644 --- a/openhands-sdk/openhands/sdk/extensions/fetch.py +++ b/openhands-sdk/openhands/sdk/extensions/fetch.py @@ -200,7 +200,7 @@ def fetch_with_resolution( def get_cache_path(source: str, cache_dir: Path) -> Path: - """Get the cache path for a plugin source. + """Get the cache path for an extension source. Creates a deterministic path based on a hash of the source URL. From d1b3b6b5ab7216ea7f5bd0de661eda8ce193ff42 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 10:35:50 -0600 Subject: [PATCH 05/10] Add SourceType docstring and fix stale plugin refs in extensions/fetch.py - Add class docstring to SourceType enum describing each variant. - Update parse_extension_source docstring: fix arg description, return types, and example function names. - Fix _apply_subpath docstring example from 'plugin repository' to 'extension repository'. - Fix get_cache_path docstring references from plugin to extension. - Rename plugin_path variable to ext_path in fetch_with_resolution. Co-authored-by: openhands --- .../openhands/sdk/extensions/fetch.py | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/openhands-sdk/openhands/sdk/extensions/fetch.py b/openhands-sdk/openhands/sdk/extensions/fetch.py index 8efb272c8e..c885eb2751 100644 --- a/openhands-sdk/openhands/sdk/extensions/fetch.py +++ b/openhands-sdk/openhands/sdk/extensions/fetch.py @@ -19,6 +19,13 @@ class ExtensionFetchError(Exception): class SourceType(str, Enum): + """Classification of an extension source. + + LOCAL -- a filesystem path (absolute, home-relative, or dot-relative). + GIT -- any git-clonable URL (HTTPS, SSH, git://, etc.). + GITHUB -- the ``github:owner/repo`` shorthand, expanded to an HTTPS URL. + """ + LOCAL = "local" GIT = "git" GITHUB = "github" @@ -28,7 +35,7 @@ def parse_extension_source(source: str) -> tuple[SourceType, str]: """Parse extension source into (SourceType, url). Args: - source: Plugin source string. Can be: + source: Extension source string. Can be: - "github:owner/repo" - GitHub repository shorthand - "https://github.com/owner/repo.git" - Full git URL - "git@github.com:owner/repo.git" - SSH git URL @@ -36,17 +43,17 @@ def parse_extension_source(source: str) -> tuple[SourceType, str]: Returns: Tuple of (source_type, normalized_url) where source_type is one of: - - "github": GitHub repository - - "git": Any git URL - - "local": Local filesystem path + - SourceType.GITHUB: GitHub repository + - SourceType.GIT: Any git URL + - SourceType.LOCAL: Local filesystem path Examples: - >>> parse_plugin_source("github:owner/repo") - ("github", "https://github.com/owner/repo.git") - >>> parse_plugin_source("https://gitlab.com/org/repo.git") - ("git", "https://gitlab.com/org/repo.git") - >>> parse_plugin_source("/local/path") - ("local", "/local/path") + >>> parse_extension_source("github:owner/repo") + (SourceType.GITHUB, "https://github.com/owner/repo.git") + >>> parse_extension_source("https://gitlab.com/org/repo.git") + (SourceType.GIT, "https://gitlab.com/org/repo.git") + >>> parse_extension_source("/local/path") + (SourceType.LOCAL, "/local/path") """ source = source.strip() @@ -106,7 +113,7 @@ def _apply_subpath(base_path: Path, subpath: str | None, context: str) -> Path: Args: base_path: The root path. subpath: Optional subdirectory path (may have leading/trailing slashes). - context: Description for error messages (e.g., "plugin repository"). + context: Description for error messages (e.g., "extension repository"). Returns: The final path (base_path if no subpath, otherwise base_path/subpath). @@ -193,10 +200,10 @@ def fetch_with_resolution( git = git_helper if git_helper is not None else GitHelper() - plugin_path, resolved_ref = _fetch_remote_source_with_resolution( + ext_path, resolved_ref = _fetch_remote_source_with_resolution( url, cache_dir, ref, update, repo_path, git, source ) - return plugin_path, resolved_ref + return ext_path, resolved_ref def get_cache_path(source: str, cache_dir: Path) -> Path: @@ -205,11 +212,11 @@ def get_cache_path(source: str, cache_dir: Path) -> Path: Creates a deterministic path based on a hash of the source URL. Args: - source: The plugin source (URL or path). + source: The extension source (URL or path). cache_dir: Base cache directory. Returns: - Path where the plugin should be cached. + Path where the extension should be cached. """ # Create a hash of the source for the directory name source_hash = hashlib.sha256(source.encode()).hexdigest()[:16] From e0990614f42adf389efcccde02edd5335ec7c535 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 10:42:28 -0600 Subject: [PATCH 06/10] Refactor plugin and skills fetch to delegate to extensions.fetch Both modules now delegate all fetch logic to the shared extensions.fetch module while preserving their public interfaces: - plugin/fetch.py: parse_plugin_source, get_cache_path, fetch_plugin, fetch_plugin_with_resolution, PluginFetchError, DEFAULT_CACHE_DIR - skills/fetch.py: fetch_skill, fetch_skill_with_resolution, SkillFetchError, DEFAULT_CACHE_DIR ExtensionFetchError is caught and re-raised as the domain-specific error type with 'extension' replaced by 'plugin'/'skill' in messages. Co-authored-by: openhands --- openhands-sdk/openhands/sdk/plugin/fetch.py | 237 +++----------------- openhands-sdk/openhands/sdk/skills/fetch.py | 16 +- 2 files changed, 43 insertions(+), 210 deletions(-) diff --git a/openhands-sdk/openhands/sdk/plugin/fetch.py b/openhands-sdk/openhands/sdk/plugin/fetch.py index 592236dfd7..0e78ca85af 100644 --- a/openhands-sdk/openhands/sdk/plugin/fetch.py +++ b/openhands-sdk/openhands/sdk/plugin/fetch.py @@ -1,16 +1,22 @@ -"""Plugin fetching utilities for remote plugin sources.""" +"""Plugin fetching utilities for remote plugin sources. + +Delegates to :mod:`openhands.sdk.extensions.fetch` for the actual fetch logic +and re-raises errors as :class:`PluginFetchError` to preserve the existing +public interface. +""" from __future__ import annotations -import hashlib from pathlib import Path -from openhands.sdk.git.cached_repo import GitHelper, try_cached_clone_or_update -from openhands.sdk.git.utils import extract_repo_name, is_git_url, normalize_git_url -from openhands.sdk.logger import get_logger - +from openhands.sdk.extensions.fetch import ( + ExtensionFetchError, + fetch_with_resolution as _ext_fetch_with_resolution, + get_cache_path as _ext_get_cache_path, + parse_extension_source, +) +from openhands.sdk.git.cached_repo import GitHelper -logger = get_logger(__name__) DEFAULT_CACHE_DIR = Path.home() / ".openhands" / "cache" / "plugins" @@ -18,8 +24,6 @@ class PluginFetchError(Exception): """Raised when fetching a plugin fails.""" - pass - def parse_plugin_source(source: str) -> tuple[str, str]: """Parse plugin source into (type, url). @@ -45,38 +49,11 @@ def parse_plugin_source(source: str) -> tuple[str, str]: >>> parse_plugin_source("/local/path") ("local", "/local/path") """ - source = source.strip() - - # GitHub shorthand: github:owner/repo - if source.startswith("github:"): - repo_path = source[7:] # Remove "github:" prefix - # Validate format - if "/" not in repo_path or repo_path.count("/") > 1: - raise PluginFetchError( - f"Invalid GitHub shorthand format: {source}. " - f"Expected format: github:owner/repo" - ) - url = f"https://github.com/{repo_path}.git" - return ("github", url) - - # Git URLs: detect by protocol/scheme rather than enumerating providers - # This handles GitHub, GitLab, Bitbucket, Codeberg, self-hosted instances, etc. - if is_git_url(source): - url = normalize_git_url(source) - return ("git", url) - - # Local path: starts with /, ~, . or contains / without a URL scheme - if source.startswith(("/", "~", ".")): - return ("local", source) - - if "/" in source and "://" not in source: - # Relative path like "plugins/my-plugin" - return ("local", source) - - raise PluginFetchError( - f"Unable to parse plugin source: {source}. " - f"Expected formats: 'github:owner/repo', git URL, or local path" - ) + try: + source_type, url = parse_extension_source(source) + except ExtensionFetchError as exc: + raise PluginFetchError(str(exc).replace("extension", "plugin")) from exc + return (source_type.value, url) def get_cache_path(source: str, cache_dir: Path | None = None) -> Path: @@ -91,102 +68,10 @@ def get_cache_path(source: str, cache_dir: Path | None = None) -> Path: Returns: Path where the plugin should be cached. """ - if cache_dir is None: - cache_dir = DEFAULT_CACHE_DIR - - # Create a hash of the source for the directory name - source_hash = hashlib.sha256(source.encode()).hexdigest()[:16] - - # Extract repo name for human-readable cache directory name - readable_name = extract_repo_name(source) - - cache_name = f"{readable_name}-{source_hash}" - return cache_dir / cache_name - - -def _resolve_local_source(url: str) -> Path: - """Resolve a local plugin source to a path. - - Args: - url: Local path string (may contain ~ for home directory). - - Returns: - Resolved absolute path to the plugin directory. - - Raises: - PluginFetchError: If path doesn't exist. - """ - local_path = Path(url).expanduser().resolve() - if not local_path.exists(): - raise PluginFetchError(f"Local plugin path does not exist: {local_path}") - return local_path - - -def _fetch_remote_source( - url: str, - cache_dir: Path, - ref: str | None, - update: bool, - subpath: str | None, - git_helper: GitHelper | None, - source: str, -) -> Path: - """Fetch a remote plugin source and cache it locally. - - Args: - url: Git URL to fetch. - cache_dir: Base directory for caching. - ref: Optional branch, tag, or commit to checkout. - update: Whether to update existing cache. - subpath: Optional subdirectory within the repository. - git_helper: GitHelper instance for git operations. - source: Original source string (for error messages). - - Returns: - Path to the cached plugin directory. - - Raises: - PluginFetchError: If fetching fails or subpath is invalid. - """ - plugin_path = get_cache_path(url, cache_dir) - cache_dir.mkdir(parents=True, exist_ok=True) - - result = try_cached_clone_or_update( - url=url, - repo_path=plugin_path, - ref=ref, - update=update, - git_helper=git_helper, + return _ext_get_cache_path( + source, cache_dir if cache_dir is not None else DEFAULT_CACHE_DIR ) - if result is None: - raise PluginFetchError(f"Failed to fetch plugin from {source}") - - return _apply_subpath(plugin_path, subpath, "plugin repository") - - -def _apply_subpath(base_path: Path, subpath: str | None, context: str) -> Path: - """Apply a subpath to a base path, validating it exists. - - Args: - base_path: The root path. - subpath: Optional subdirectory path (may have leading/trailing slashes). - context: Description for error messages (e.g., "plugin repository"). - - Returns: - The final path (base_path if no subpath, otherwise base_path/subpath). - - Raises: - PluginFetchError: If subpath doesn't exist. - """ - if not subpath: - return base_path - - final_path = base_path / subpath.strip("/") - if not final_path.exists(): - raise PluginFetchError(f"Subdirectory '{subpath}' not found in {context}") - return final_path - def fetch_plugin( source: str, @@ -261,75 +146,15 @@ def fetch_plugin_with_resolution( Raises: PluginFetchError: If fetching fails or repo_path doesn't exist. """ - source_type, url = parse_plugin_source(source) - - if source_type == "local": - if repo_path is not None: - raise PluginFetchError( - f"repo_path is not supported for local plugin sources. " - f"Specify the full path directly instead of " - f"source='{source}' + repo_path='{repo_path}'" - ) - return _resolve_local_source(url), None - - if cache_dir is None: - cache_dir = DEFAULT_CACHE_DIR - - git = git_helper if git_helper is not None else GitHelper() - - plugin_path, resolved_ref = _fetch_remote_source_with_resolution( - url, cache_dir, ref, update, repo_path, git, source - ) - return plugin_path, resolved_ref - - -def _fetch_remote_source_with_resolution( - url: str, - cache_dir: Path, - ref: str | None, - update: bool, - subpath: str | None, - git_helper: GitHelper, - source: str, -) -> tuple[Path, str]: - """Fetch a remote plugin source and return path + resolved commit SHA. - - Args: - url: Git URL to fetch. - cache_dir: Base directory for caching. - ref: Optional branch, tag, or commit to checkout. - update: Whether to update existing cache. - subpath: Optional subdirectory within the repository. - git_helper: GitHelper instance for git operations. - source: Original source string (for error messages). - - Returns: - Tuple of (path, resolved_ref) where resolved_ref is the commit SHA. - - Raises: - PluginFetchError: If fetching fails or subpath is invalid. - """ - repo_cache_path = get_cache_path(url, cache_dir) - cache_dir.mkdir(parents=True, exist_ok=True) - - result = try_cached_clone_or_update( - url=url, - repo_path=repo_cache_path, - ref=ref, - update=update, - git_helper=git_helper, - ) - - if result is None: - raise PluginFetchError(f"Failed to fetch plugin from {source}") - - # Get the actual commit SHA that was checked out + resolved_cache_dir = cache_dir if cache_dir is not None else DEFAULT_CACHE_DIR try: - resolved_ref = git_helper.get_head_commit(repo_cache_path) - except Exception as e: - logger.warning(f"Could not get commit SHA for {source}: {e}") - # Fall back to the requested ref if we can't get the SHA - resolved_ref = ref or "HEAD" - - final_path = _apply_subpath(repo_cache_path, subpath, "plugin repository") - return final_path, resolved_ref + return _ext_fetch_with_resolution( + source=source, + cache_dir=resolved_cache_dir, + ref=ref, + update=update, + repo_path=repo_path, + git_helper=git_helper, + ) + except ExtensionFetchError as exc: + raise PluginFetchError(str(exc).replace("extension", "plugin")) from exc diff --git a/openhands-sdk/openhands/sdk/skills/fetch.py b/openhands-sdk/openhands/sdk/skills/fetch.py index fea5a99f50..57618d0e15 100644 --- a/openhands-sdk/openhands/sdk/skills/fetch.py +++ b/openhands-sdk/openhands/sdk/skills/fetch.py @@ -1,11 +1,19 @@ -"""Skill fetching utilities for AgentSkills sources.""" +"""Skill fetching utilities for AgentSkills sources. + +Delegates to :mod:`openhands.sdk.extensions.fetch` for the actual fetch logic +and re-raises errors as :class:`SkillFetchError` to preserve the existing +public interface. +""" from __future__ import annotations from pathlib import Path +from openhands.sdk.extensions.fetch import ( + ExtensionFetchError, + fetch_with_resolution as _ext_fetch_with_resolution, +) from openhands.sdk.git.cached_repo import GitHelper -from openhands.sdk.plugin.fetch import PluginFetchError, fetch_plugin_with_resolution DEFAULT_CACHE_DIR = Path.home() / ".openhands" / "cache" / "skills" @@ -74,7 +82,7 @@ def fetch_skill_with_resolution( """ resolved_cache_dir = cache_dir if cache_dir is not None else DEFAULT_CACHE_DIR try: - return fetch_plugin_with_resolution( + return _ext_fetch_with_resolution( source=source, cache_dir=resolved_cache_dir, ref=ref, @@ -82,5 +90,5 @@ def fetch_skill_with_resolution( repo_path=repo_path, git_helper=git_helper, ) - except PluginFetchError as exc: + except ExtensionFetchError as exc: raise SkillFetchError(str(exc)) from exc From b3e72968707989bc22d8852af48982b8997012c1 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 10:54:38 -0600 Subject: [PATCH 07/10] Remove parse_plugin_source/get_cache_path, slim plugin tests - Remove parse_plugin_source and get_cache_path from plugin/fetch.py; callers should use extensions.fetch directly. - Slim test_plugin_fetch.py from 1038 to 100 lines: keep only PluginFetchError wrapping, DEFAULT_CACHE_DIR, and Plugin.fetch() tests. - Move git infrastructure tests (clone, update, checkout, locking, GitHelper errors, get_default_branch) to tests/sdk/git/test_cached_repo.py. - Update test_installed_plugins.py to import from extensions.fetch. Co-authored-by: openhands --- openhands-sdk/openhands/sdk/plugin/fetch.py | 50 - tests/sdk/git/test_cached_repo.py | 472 +++++++++ tests/sdk/plugin/test_installed_plugins.py | 7 +- tests/sdk/plugin/test_plugin_fetch.py | 1060 ++----------------- 4 files changed, 537 insertions(+), 1052 deletions(-) create mode 100644 tests/sdk/git/test_cached_repo.py diff --git a/openhands-sdk/openhands/sdk/plugin/fetch.py b/openhands-sdk/openhands/sdk/plugin/fetch.py index 0e78ca85af..2fcea18577 100644 --- a/openhands-sdk/openhands/sdk/plugin/fetch.py +++ b/openhands-sdk/openhands/sdk/plugin/fetch.py @@ -12,8 +12,6 @@ from openhands.sdk.extensions.fetch import ( ExtensionFetchError, fetch_with_resolution as _ext_fetch_with_resolution, - get_cache_path as _ext_get_cache_path, - parse_extension_source, ) from openhands.sdk.git.cached_repo import GitHelper @@ -25,54 +23,6 @@ class PluginFetchError(Exception): """Raised when fetching a plugin fails.""" -def parse_plugin_source(source: str) -> tuple[str, str]: - """Parse plugin source into (type, url). - - Args: - source: Plugin source string. Can be: - - "github:owner/repo" - GitHub repository shorthand - - "https://github.com/owner/repo.git" - Full git URL - - "git@github.com:owner/repo.git" - SSH git URL - - "/local/path" - Local path - - Returns: - Tuple of (source_type, normalized_url) where source_type is one of: - - "github": GitHub repository - - "git": Any git URL - - "local": Local filesystem path - - Examples: - >>> parse_plugin_source("github:owner/repo") - ("github", "https://github.com/owner/repo.git") - >>> parse_plugin_source("https://gitlab.com/org/repo.git") - ("git", "https://gitlab.com/org/repo.git") - >>> parse_plugin_source("/local/path") - ("local", "/local/path") - """ - try: - source_type, url = parse_extension_source(source) - except ExtensionFetchError as exc: - raise PluginFetchError(str(exc).replace("extension", "plugin")) from exc - return (source_type.value, url) - - -def get_cache_path(source: str, cache_dir: Path | None = None) -> Path: - """Get the cache path for a plugin source. - - Creates a deterministic path based on a hash of the source URL. - - Args: - source: The plugin source (URL or path). - cache_dir: Base cache directory. Defaults to ~/.openhands/cache/plugins/ - - Returns: - Path where the plugin should be cached. - """ - return _ext_get_cache_path( - source, cache_dir if cache_dir is not None else DEFAULT_CACHE_DIR - ) - - def fetch_plugin( source: str, cache_dir: Path | None = None, diff --git a/tests/sdk/git/test_cached_repo.py b/tests/sdk/git/test_cached_repo.py new file mode 100644 index 0000000000..b9e9e2b91e --- /dev/null +++ b/tests/sdk/git/test_cached_repo.py @@ -0,0 +1,472 @@ +"""Tests for git cached_repo helpers (clone, update, checkout, locking).""" + +import subprocess +from pathlib import Path +from unittest.mock import create_autospec, patch + +import pytest + +from openhands.sdk.git.cached_repo import ( + GitHelper, + _checkout_ref, + _clone_repository, + _update_repository, +) +from openhands.sdk.git.exceptions import GitCommandError + + +# -- _clone_repository --------------------------------------------------------- + + +def test_clone_calls_git_helper(tmp_path: Path): + mock_git = create_autospec(GitHelper) + dest = tmp_path / "repo" + + _clone_repository("https://github.com/owner/repo.git", dest, None, mock_git) + + mock_git.clone.assert_called_once_with( + "https://github.com/owner/repo.git", dest, depth=1, branch=None + ) + + +def test_clone_with_ref(tmp_path: Path): + mock_git = create_autospec(GitHelper) + dest = tmp_path / "repo" + + _clone_repository("https://github.com/owner/repo.git", dest, "v1.0.0", mock_git) + + mock_git.clone.assert_called_once_with( + "https://github.com/owner/repo.git", dest, depth=1, branch="v1.0.0" + ) + + +def test_clone_removes_existing_directory(tmp_path: Path): + mock_git = create_autospec(GitHelper) + dest = tmp_path / "repo" + dest.mkdir() + (dest / "some_file.txt").write_text("test") + + _clone_repository("https://github.com/owner/repo.git", dest, None, mock_git) + + mock_git.clone.assert_called_once() + + +# -- _update_repository -------------------------------------------------------- + + +def test_update_fetches_and_resets(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.get_current_branch.return_value = "main" + + _update_repository(tmp_path, None, mock_git) + + mock_git.fetch.assert_called_once_with(tmp_path) + mock_git.get_current_branch.assert_called_once_with(tmp_path) + mock_git.reset_hard.assert_called_once_with(tmp_path, "origin/main") + + +def test_update_with_ref_checks_out(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.get_current_branch.return_value = None + + _update_repository(tmp_path, "v1.0.0", mock_git) + + mock_git.fetch.assert_called_once_with(tmp_path) + mock_git.checkout.assert_called_once_with(tmp_path, "v1.0.0") + + +def test_update_detached_head_recovers_to_default_branch(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.get_current_branch.return_value = None + mock_git.get_default_branch.return_value = "main" + + _update_repository(tmp_path, None, mock_git) + + mock_git.fetch.assert_called_once() + mock_git.get_current_branch.assert_called_once() + mock_git.get_default_branch.assert_called_once_with(tmp_path) + mock_git.checkout.assert_called_once_with(tmp_path, "main") + mock_git.reset_hard.assert_called_once_with(tmp_path, "origin/main") + + +def test_update_detached_head_no_default_branch_logs_warning(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.get_current_branch.return_value = None + mock_git.get_default_branch.return_value = None + + _update_repository(tmp_path, None, mock_git) + + mock_git.fetch.assert_called_once() + mock_git.get_default_branch.assert_called_once() + mock_git.checkout.assert_not_called() + mock_git.reset_hard.assert_not_called() + + +def test_update_continues_on_fetch_error(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.fetch.side_effect = GitCommandError( + "Network error", command=["git", "fetch"], exit_code=1 + ) + + _update_repository(tmp_path, None, mock_git) + + mock_git.fetch.assert_called_once() + mock_git.get_current_branch.assert_not_called() + + +def test_update_continues_on_checkout_error(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.checkout.side_effect = GitCommandError( + "Invalid ref", command=["git", "checkout"], exit_code=1 + ) + + _update_repository(tmp_path, "nonexistent", mock_git) + + +# -- _checkout_ref ------------------------------------------------------------- + + +def test_checkout_branch_resets_to_origin(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.get_current_branch.return_value = "main" + + _checkout_ref(tmp_path, "main", mock_git) + + mock_git.checkout.assert_called_once_with(tmp_path, "main") + mock_git.get_current_branch.assert_called_once_with(tmp_path) + mock_git.reset_hard.assert_called_once_with(tmp_path, "origin/main") + + +def test_checkout_tag_skips_reset(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.get_current_branch.return_value = None + + _checkout_ref(tmp_path, "v1.0.0", mock_git) + + mock_git.checkout.assert_called_once_with(tmp_path, "v1.0.0") + mock_git.reset_hard.assert_not_called() + + +def test_checkout_commit_skips_reset(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.get_current_branch.return_value = None + + _checkout_ref(tmp_path, "abc123", mock_git) + + mock_git.checkout.assert_called_once_with(tmp_path, "abc123") + mock_git.reset_hard.assert_not_called() + + +def test_checkout_branch_handles_reset_error(tmp_path: Path): + mock_git = create_autospec(GitHelper) + mock_git.get_current_branch.return_value = "main" + mock_git.reset_hard.side_effect = GitCommandError( + "Reset failed", command=["git", "reset"], exit_code=1 + ) + + _checkout_ref(tmp_path, "main", mock_git) + + mock_git.checkout.assert_called_once() + mock_git.reset_hard.assert_called_once() + + +# -- GitHelper error handling -------------------------------------------------- + + +def test_git_clone_called_process_error(tmp_path: Path): + git = GitHelper() + dest = tmp_path / "repo" + + with pytest.raises(GitCommandError, match="git clone"): + git.clone("https://invalid.example.com/nonexistent.git", dest, timeout=5) + + +def test_git_clone_timeout(tmp_path: Path): + git = GitHelper() + dest = tmp_path / "repo" + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) + with pytest.raises(GitCommandError, match="timed out"): + git.clone("https://github.com/owner/repo.git", dest, timeout=1) + + +def test_git_fetch_with_ref_no_remote(tmp_path: Path): + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init"], cwd=repo, check=True) + subprocess.run( + ["git", "config", "user.email", "test@test.com"], + cwd=repo, + check=True, + ) + subprocess.run(["git", "config", "user.name", "Test"], cwd=repo, check=True) + (repo / "file.txt").write_text("content") + subprocess.run(["git", "add", "."], cwd=repo, check=True) + subprocess.run(["git", "commit", "-m", "Initial"], cwd=repo, check=True) + + git = GitHelper() + with pytest.raises(GitCommandError, match="git fetch"): + git.fetch(repo, ref="main") + + +def test_git_fetch_called_process_error(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "not-a-repo" + repo.mkdir() + + with pytest.raises(GitCommandError, match="git fetch"): + git.fetch(repo) + + +def test_git_fetch_timeout(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) + with pytest.raises(GitCommandError, match="timed out"): + git.fetch(repo, timeout=1) + + +def test_git_checkout_called_process_error(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init"], cwd=repo, check=True) + + with pytest.raises(GitCommandError, match="git checkout"): + git.checkout(repo, "nonexistent-ref") + + +def test_git_checkout_timeout(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) + with pytest.raises(GitCommandError, match="timed out"): + git.checkout(repo, "main", timeout=1) + + +def test_git_reset_hard_called_process_error(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + subprocess.run(["git", "init"], cwd=repo, check=True) + + with pytest.raises(GitCommandError, match="git reset"): + git.reset_hard(repo, "nonexistent-ref") + + +def test_git_reset_hard_timeout(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) + with pytest.raises(GitCommandError, match="timed out"): + git.reset_hard(repo, "HEAD", timeout=1) + + +def test_git_get_current_branch_error(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "not-a-repo" + repo.mkdir() + + with pytest.raises(GitCommandError, match="git rev-parse"): + git.get_current_branch(repo) + + +def test_git_get_current_branch_timeout(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) + with pytest.raises(GitCommandError, match="timed out"): + git.get_current_branch(repo, timeout=1) + + +# -- GitHelper.get_default_branch --------------------------------------------- + + +def test_get_default_branch_returns_main(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.return_value = subprocess.CompletedProcess( + args=["git"], + returncode=0, + stdout="refs/remotes/origin/main\n", + stderr="", + ) + result = git.get_default_branch(repo) + + assert result == "main" + call_args = mock_run.call_args[0][0] + assert call_args == ["git", "symbolic-ref", "refs/remotes/origin/HEAD"] + + +def test_get_default_branch_returns_master(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.return_value = subprocess.CompletedProcess( + args=["git"], + returncode=0, + stdout="refs/remotes/origin/master\n", + stderr="", + ) + result = git.get_default_branch(repo) + + assert result == "master" + + +def test_get_default_branch_returns_none_when_not_set(tmp_path: Path): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.return_value = subprocess.CompletedProcess( + args=["git"], + returncode=1, + stdout="", + stderr=("fatal: ref refs/remotes/origin/HEAD is not a symbolic ref"), + ) + result = git.get_default_branch(repo) + + assert result is None + + +def test_get_default_branch_returns_none_on_unexpected_format( + tmp_path: Path, +): + git = GitHelper() + repo = tmp_path / "repo" + repo.mkdir() + + with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: + mock_run.return_value = subprocess.CompletedProcess( + args=["git"], + returncode=0, + stdout="unexpected-format\n", + stderr="", + ) + result = git.get_default_branch(repo) + + assert result is None + + +# -- Cache locking ------------------------------------------------------------- + + +def test_lock_file_created_during_clone(tmp_path: Path): + from openhands.sdk.git.cached_repo import try_cached_clone_or_update + + cache_dir = tmp_path / "cache" + repo_path = cache_dir / "test-repo" + + mock_git = create_autospec(GitHelper, instance=True) + lock_existed_during_clone: list[bool] = [] + + def mock_clone(url, dest, depth=None, branch=None, timeout=120): + lock_path = repo_path.with_suffix(".lock") + lock_existed_during_clone.append(lock_path.exists()) + + mock_git.clone.side_effect = mock_clone + + try_cached_clone_or_update( + url="https://github.com/test/repo.git", + repo_path=repo_path, + git_helper=mock_git, + ) + + assert lock_existed_during_clone[0] is True + + +def test_lock_timeout_returns_none(tmp_path: Path): + from filelock import FileLock + + from openhands.sdk.git.cached_repo import try_cached_clone_or_update + + cache_dir = tmp_path / "cache" + cache_dir.mkdir(parents=True) + repo_path = cache_dir / "test-repo" + + lock_path = repo_path.with_suffix(".lock") + external_lock = FileLock(lock_path) + external_lock.acquire() + + try: + mock_git = create_autospec(GitHelper, instance=True) + + result = try_cached_clone_or_update( + url="https://github.com/test/repo.git", + repo_path=repo_path, + git_helper=mock_git, + lock_timeout=0.1, + ) + + assert result is None + mock_git.clone.assert_not_called() + finally: + external_lock.release() + + +def test_lock_released_after_operation(tmp_path: Path): + from filelock import FileLock + + from openhands.sdk.git.cached_repo import try_cached_clone_or_update + + cache_dir = tmp_path / "cache" + repo_path = cache_dir / "test-repo" + + mock_git = create_autospec(GitHelper, instance=True) + + try_cached_clone_or_update( + url="https://github.com/test/repo.git", + repo_path=repo_path, + git_helper=mock_git, + ) + + lock_path = repo_path.with_suffix(".lock") + lock = FileLock(lock_path) + lock.acquire(timeout=0) + lock.release() + + +def test_lock_released_on_error(tmp_path: Path): + from filelock import FileLock + + from openhands.sdk.git.cached_repo import try_cached_clone_or_update + + cache_dir = tmp_path / "cache" + repo_path = cache_dir / "test-repo" + + mock_git = create_autospec(GitHelper, instance=True) + mock_git.clone.side_effect = GitCommandError( + "Clone failed", command=["git", "clone"], exit_code=1, stderr="error" + ) + + result = try_cached_clone_or_update( + url="https://github.com/test/repo.git", + repo_path=repo_path, + git_helper=mock_git, + ) + + assert result is None + + lock_path = repo_path.with_suffix(".lock") + lock = FileLock(lock_path) + lock.acquire(timeout=0) + lock.release() diff --git a/tests/sdk/plugin/test_installed_plugins.py b/tests/sdk/plugin/test_installed_plugins.py index e829dfc2ae..a9dc3c00a9 100644 --- a/tests/sdk/plugin/test_installed_plugins.py +++ b/tests/sdk/plugin/test_installed_plugins.py @@ -14,6 +14,7 @@ import pytest +from openhands.sdk.extensions.fetch import get_cache_path, parse_extension_source from openhands.sdk.plugin import ( InstalledPluginInfo, InstalledPluginsMetadata, @@ -29,7 +30,7 @@ uninstall_plugin, update_plugin, ) -from openhands.sdk.plugin.fetch import get_cache_path, parse_plugin_source +from openhands.sdk.plugin.fetch import DEFAULT_CACHE_DIR as DEFAULT_PLUGIN_CACHE_DIR # ============================================================================ @@ -571,8 +572,8 @@ def test_install_document_skills_plugin(installed_dir: Path) -> None: installed_dir=installed_dir, ) - _, url = parse_plugin_source(source) - expected_name = get_cache_path(url).name + _, url = parse_extension_source(source) + expected_name = get_cache_path(url, DEFAULT_PLUGIN_CACHE_DIR).name assert info.name == expected_name assert info.source == source diff --git a/tests/sdk/plugin/test_plugin_fetch.py b/tests/sdk/plugin/test_plugin_fetch.py index 5d072787b7..23fb40e697 100644 --- a/tests/sdk/plugin/test_plugin_fetch.py +++ b/tests/sdk/plugin/test_plugin_fetch.py @@ -1,1038 +1,100 @@ -"""Tests for Plugin.fetch() functionality.""" +"""Tests for plugin-specific fetch behavior. + +Verifies that the plugin fetch layer correctly wraps extensions.fetch with +plugin-specific error types (PluginFetchError), the plugin DEFAULT_CACHE_DIR, +and the Plugin.fetch() classmethod. + +Core fetch logic (parsing, caching, git operations) is tested in +tests/sdk/extensions/test_fetch.py. Git infrastructure (clone, update, +checkout, locking) is tested in tests/sdk/git/test_cached_repo.py. +""" -import subprocess from pathlib import Path from unittest.mock import create_autospec, patch import pytest -from openhands.sdk.git.cached_repo import ( - GitHelper, - _checkout_ref, - _clone_repository, - _update_repository, -) +from openhands.sdk.git.cached_repo import GitHelper from openhands.sdk.git.exceptions import GitCommandError -from openhands.sdk.git.utils import extract_repo_name -from openhands.sdk.plugin import ( - Plugin, - PluginFetchError, -) -from openhands.sdk.plugin.fetch import ( - fetch_plugin, - get_cache_path, - parse_plugin_source, -) - - -class TestParsePluginSource: - """Tests for parse_plugin_source function.""" - - def test_github_shorthand(self): - """Test parsing GitHub shorthand format.""" - source_type, url = parse_plugin_source("github:owner/repo") - assert source_type == "github" - assert url == "https://github.com/owner/repo.git" - - def test_github_shorthand_with_whitespace(self): - """Test parsing GitHub shorthand with leading/trailing whitespace.""" - source_type, url = parse_plugin_source(" github:owner/repo ") - assert source_type == "github" - assert url == "https://github.com/owner/repo.git" - - def test_github_shorthand_invalid_format(self): - """Test that invalid GitHub shorthand raises error.""" - with pytest.raises(PluginFetchError, match="Invalid GitHub shorthand"): - parse_plugin_source("github:invalid") - - with pytest.raises(PluginFetchError, match="Invalid GitHub shorthand"): - parse_plugin_source("github:too/many/parts") - - def test_https_git_url(self): - """Test parsing HTTPS git URLs.""" - source_type, url = parse_plugin_source("https://github.com/owner/repo.git") - assert source_type == "git" - assert url == "https://github.com/owner/repo.git" - - def test_https_github_url_without_git_suffix(self): - """Test parsing GitHub HTTPS URL without .git suffix.""" - source_type, url = parse_plugin_source("https://github.com/owner/repo") - assert source_type == "git" - assert url == "https://github.com/owner/repo.git" - - def test_https_github_url_with_trailing_slash(self): - """Test parsing GitHub HTTPS URL with trailing slash.""" - source_type, url = parse_plugin_source("https://github.com/owner/repo/") - assert source_type == "git" - assert url == "https://github.com/owner/repo.git" - - def test_https_gitlab_url(self): - """Test parsing GitLab HTTPS URLs.""" - source_type, url = parse_plugin_source("https://gitlab.com/org/repo") - assert source_type == "git" - assert url == "https://gitlab.com/org/repo.git" - - def test_https_bitbucket_url(self): - """Test parsing Bitbucket HTTPS URLs.""" - source_type, url = parse_plugin_source("https://bitbucket.org/org/repo") - assert source_type == "git" - assert url == "https://bitbucket.org/org/repo.git" - - def test_ssh_git_url(self): - """Test parsing SSH git URLs.""" - source_type, url = parse_plugin_source("git@github.com:owner/repo.git") - assert source_type == "git" - assert url == "git@github.com:owner/repo.git" - - def test_git_protocol_url(self): - """Test parsing git:// protocol URLs.""" - source_type, url = parse_plugin_source("git://github.com/owner/repo.git") - assert source_type == "git" - assert url == "git://github.com/owner/repo.git" - - def test_absolute_local_path(self): - """Test parsing absolute local paths.""" - source_type, url = parse_plugin_source("/path/to/plugin") - assert source_type == "local" - assert url == "/path/to/plugin" - - def test_home_relative_path(self): - """Test parsing home-relative paths.""" - source_type, url = parse_plugin_source("~/plugins/my-plugin") - assert source_type == "local" - assert url == "~/plugins/my-plugin" - - def test_relative_path(self): - """Test parsing relative paths.""" - source_type, url = parse_plugin_source("./plugins/my-plugin") - assert source_type == "local" - assert url == "./plugins/my-plugin" - - def test_invalid_source(self): - """Test that unparseable sources raise error.""" - with pytest.raises(PluginFetchError, match="Unable to parse plugin source"): - parse_plugin_source("invalid-source-format") - - def test_self_hosted_git_url(self): - """Test parsing URLs from self-hosted/alternative git providers.""" - # Codeberg - source_type, url = parse_plugin_source("https://codeberg.org/user/repo") - assert source_type == "git" - assert url == "https://codeberg.org/user/repo.git" - - # Self-hosted GitLab - source_type, url = parse_plugin_source("https://git.mycompany.com/org/repo") - assert source_type == "git" - assert url == "https://git.mycompany.com/org/repo.git" - - # SourceHut - source_type, url = parse_plugin_source("https://sr.ht/~user/repo") - assert source_type == "git" - assert url == "https://sr.ht/~user/repo.git" - - def test_http_url(self): - """Test parsing plain HTTP URLs (some internal servers).""" - source_type, url = parse_plugin_source("http://internal-git.local/repo") - assert source_type == "git" - assert url == "http://internal-git.local/repo.git" - - def test_ssh_with_custom_user(self): - """Test SSH URLs with non-git usernames.""" - ssh_url = "deploy@git.example.com:project/repo.git" - source_type, url = parse_plugin_source(ssh_url) - assert source_type == "git" - assert url == ssh_url - - -class TestExtractRepoName: - """Tests for extract_repo_name function (in git.utils).""" - - def test_github_shorthand(self): - """Test extracting name from GitHub shorthand.""" - name = extract_repo_name("github:owner/my-plugin") - assert name == "my-plugin" - - def test_https_url(self): - """Test extracting name from HTTPS URL.""" - name = extract_repo_name("https://github.com/owner/my-plugin.git") - assert name == "my-plugin" - - def test_ssh_url(self): - """Test extracting name from SSH URL.""" - name = extract_repo_name("git@github.com:owner/my-plugin.git") - assert name == "my-plugin" - - def test_local_path(self): - """Test extracting name from local path.""" - name = extract_repo_name("/path/to/my-plugin") - assert name == "my-plugin" - - def test_special_characters_sanitized(self): - """Test that special characters are sanitized.""" - name = extract_repo_name("https://github.com/owner/my.special@plugin!.git") - assert name == "my-special-plugin" - - def test_long_name_truncated(self): - """Test that long names are truncated.""" - name = extract_repo_name( - "github:owner/this-is-a-very-long-plugin-name-that-should-be-truncated" - ) - assert len(name) <= 32 - - -class TestGetCachePath: - """Tests for get_cache_path function.""" - - def test_deterministic_path(self, tmp_path: Path): - """Test that cache path is deterministic for same source.""" - source = "https://github.com/owner/repo.git" - path1 = get_cache_path(source, tmp_path) - path2 = get_cache_path(source, tmp_path) - assert path1 == path2 - - def test_different_sources_different_paths(self, tmp_path: Path): - """Test that different sources get different paths.""" - path1 = get_cache_path("https://github.com/owner/repo1.git", tmp_path) - path2 = get_cache_path("https://github.com/owner/repo2.git", tmp_path) - assert path1 != path2 - - def test_path_includes_readable_name(self, tmp_path: Path): - """Test that cache path includes readable name.""" - source = "https://github.com/owner/my-plugin.git" - path = get_cache_path(source, tmp_path) - assert "my-plugin" in path.name - - def test_default_cache_dir(self): - """Test that default cache dir is under ~/.openhands/cache/plugins/.""" - source = "https://github.com/owner/repo.git" - path = get_cache_path(source) - assert ".openhands" in str(path) - assert "cache" in str(path) - assert "plugins" in str(path) - - -class TestCloneRepository: - """Tests for _clone_repository function.""" - - def test_clone_calls_git_helper(self, tmp_path: Path): - """Test that clone delegates to GitHelper.""" - mock_git = create_autospec(GitHelper) - dest = tmp_path / "repo" - - _clone_repository("https://github.com/owner/repo.git", dest, None, mock_git) - - mock_git.clone.assert_called_once_with( - "https://github.com/owner/repo.git", dest, depth=1, branch=None - ) - - def test_clone_with_ref(self, tmp_path: Path): - """Test clone with branch/tag ref.""" - mock_git = create_autospec(GitHelper) - dest = tmp_path / "repo" - - _clone_repository("https://github.com/owner/repo.git", dest, "v1.0.0", mock_git) - - mock_git.clone.assert_called_once_with( - "https://github.com/owner/repo.git", dest, depth=1, branch="v1.0.0" - ) - - def test_clone_removes_existing_directory(self, tmp_path: Path): - """Test that existing non-git directory is removed.""" - mock_git = create_autospec(GitHelper) - dest = tmp_path / "repo" - dest.mkdir() - (dest / "some_file.txt").write_text("test") - - _clone_repository("https://github.com/owner/repo.git", dest, None, mock_git) - - mock_git.clone.assert_called_once() - - -class TestUpdateRepository: - """Tests for _update_repository function.""" - - def test_update_fetches_and_resets(self, tmp_path: Path): - """Test update fetches from origin and resets to branch.""" - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = "main" - - _update_repository(tmp_path, None, mock_git) - - mock_git.fetch.assert_called_once_with(tmp_path) - mock_git.get_current_branch.assert_called_once_with(tmp_path) - mock_git.reset_hard.assert_called_once_with(tmp_path, "origin/main") - - def test_update_with_ref_checks_out(self, tmp_path: Path): - """Test update with ref checks out that ref.""" - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = None # Assume tag/commit (detached) - - _update_repository(tmp_path, "v1.0.0", mock_git) - - # fetch is called once in _update_repository (checkout_ref no longer fetches) - mock_git.fetch.assert_called_once_with(tmp_path) - mock_git.checkout.assert_called_once_with(tmp_path, "v1.0.0") - - def test_update_detached_head_recovers_to_default_branch(self, tmp_path: Path): - """Test update recovers from detached HEAD by checking out default branch. - - When a repo is in detached HEAD state (e.g., from a previous checkout of a - tag) and update is called without a ref, it should: - 1. Detect the detached HEAD state - 2. Determine the remote's default branch via origin/HEAD - 3. Checkout and reset to that default branch - - This ensures that `fetch(source, update=True)` without a ref means "get the - latest from the default branch", not "stay stuck on whatever was previously - checked out". - """ - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = None # Detached HEAD - mock_git.get_default_branch.return_value = "main" - - _update_repository(tmp_path, None, mock_git) - - mock_git.fetch.assert_called_once() - mock_git.get_current_branch.assert_called_once() - mock_git.get_default_branch.assert_called_once_with(tmp_path) - mock_git.checkout.assert_called_once_with(tmp_path, "main") - mock_git.reset_hard.assert_called_once_with(tmp_path, "origin/main") - - def test_update_detached_head_no_default_branch_logs_warning(self, tmp_path: Path): - """Test update logs warning when detached HEAD and default branch unknown. - - If origin/HEAD is not set (can happen with some git configurations), we - can't determine the default branch. In this case, log a warning and use - the cached version as-is. - """ - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = None # Detached HEAD - mock_git.get_default_branch.return_value = None # Can't determine default - - _update_repository(tmp_path, None, mock_git) - - mock_git.fetch.assert_called_once() - mock_git.get_default_branch.assert_called_once() - # Should not attempt checkout since we don't know the target - mock_git.checkout.assert_not_called() - mock_git.reset_hard.assert_not_called() - - def test_update_continues_on_fetch_error(self, tmp_path: Path): - """Test update logs warning on fetch failure but doesn't raise.""" - mock_git = create_autospec(GitHelper) - mock_git.fetch.side_effect = GitCommandError( - "Network error", command=["git", "fetch"], exit_code=1 - ) - - # Should not raise - graceful degradation - _update_repository(tmp_path, None, mock_git) - - # Fetch was attempted - mock_git.fetch.assert_called_once() - # No further operations since fetch failed - mock_git.get_current_branch.assert_not_called() - - def test_update_continues_on_checkout_error(self, tmp_path: Path): - """Test update logs warning on checkout failure but doesn't raise.""" - mock_git = create_autospec(GitHelper) - mock_git.checkout.side_effect = GitCommandError( - "Invalid ref", command=["git", "checkout"], exit_code=1 - ) - - # Should not raise - graceful degradation - _update_repository(tmp_path, "nonexistent", mock_git) - - -class TestCheckoutRef: - """Tests for _checkout_ref function. - - The function detects ref type AFTER checkout by checking HEAD state: - - Detached HEAD (None from get_current_branch) = tag or commit - - On a branch = reset to origin/{branch} - """ - - def test_checkout_branch_resets_to_origin(self, tmp_path: Path): - """Test checkout of a branch resets to origin.""" - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = "main" # On a branch - - _checkout_ref(tmp_path, "main", mock_git) - - mock_git.checkout.assert_called_once_with(tmp_path, "main") - mock_git.get_current_branch.assert_called_once_with(tmp_path) - mock_git.reset_hard.assert_called_once_with(tmp_path, "origin/main") - - def test_checkout_tag_skips_reset(self, tmp_path: Path): - """Test checkout of a tag (detached HEAD) skips reset.""" - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = None # Detached HEAD - - _checkout_ref(tmp_path, "v1.0.0", mock_git) - - mock_git.checkout.assert_called_once_with(tmp_path, "v1.0.0") - mock_git.get_current_branch.assert_called_once_with(tmp_path) - mock_git.reset_hard.assert_not_called() - - def test_checkout_commit_skips_reset(self, tmp_path: Path): - """Test checkout of a commit SHA (detached HEAD) skips reset.""" - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = None # Detached HEAD - - _checkout_ref(tmp_path, "abc123def", mock_git) - - mock_git.checkout.assert_called_once_with(tmp_path, "abc123def") - mock_git.reset_hard.assert_not_called() - - def test_checkout_branch_handles_reset_error(self, tmp_path: Path): - """Test checkout continues if reset fails (e.g., branch not on remote).""" - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = "local-only" - mock_git.reset_hard.side_effect = GitCommandError( - "Not found", command=["git", "reset"], exit_code=1 - ) - - # Should not raise - reset failure is logged but not fatal - _checkout_ref(tmp_path, "local-only", mock_git) - - mock_git.checkout.assert_called_once() - - -class TestFetchPlugin: - """Tests for fetch_plugin function.""" - - def test_fetch_local_path(self, tmp_path: Path): - """Test fetching from local path returns the path unchanged.""" - plugin_dir = tmp_path / "my-plugin" - plugin_dir.mkdir() - - result = fetch_plugin(str(plugin_dir)) - assert result == plugin_dir.resolve() - - def test_fetch_local_path_nonexistent(self, tmp_path: Path): - """Test fetching nonexistent local path raises error.""" - with pytest.raises(PluginFetchError, match="does not exist"): - fetch_plugin(str(tmp_path / "nonexistent")) - - def test_fetch_github_shorthand_clones(self, tmp_path: Path): - """Test fetching GitHub shorthand clones the repository.""" - mock_git = create_autospec(GitHelper) - - def clone_side_effect(url, dest, **kwargs): - dest.mkdir(parents=True, exist_ok=True) - (dest / ".git").mkdir() - - mock_git.clone.side_effect = clone_side_effect - - result = fetch_plugin( - "github:owner/repo", cache_dir=tmp_path, git_helper=mock_git - ) - - assert result.exists() - mock_git.clone.assert_called_once() - call_args = mock_git.clone.call_args - assert call_args[0][0] == "https://github.com/owner/repo.git" - - def test_fetch_with_ref(self, tmp_path: Path): - """Test fetching with specific ref.""" - mock_git = create_autospec(GitHelper) - - def clone_side_effect(url, dest, **kwargs): - dest.mkdir(parents=True, exist_ok=True) - (dest / ".git").mkdir() - - mock_git.clone.side_effect = clone_side_effect - - fetch_plugin( - "github:owner/repo", cache_dir=tmp_path, ref="v1.0.0", git_helper=mock_git - ) - - mock_git.clone.assert_called_once() - call_kwargs = mock_git.clone.call_args[1] - assert call_kwargs["branch"] == "v1.0.0" - - def test_fetch_updates_existing_cache(self, tmp_path: Path): - """Test that fetch updates existing cached repository.""" - mock_git = create_autospec(GitHelper) - mock_git.get_current_branch.return_value = "main" - - cache_path = get_cache_path("https://github.com/owner/repo.git", tmp_path) - cache_path.mkdir(parents=True) - (cache_path / ".git").mkdir() - - result = fetch_plugin( - "github:owner/repo", cache_dir=tmp_path, update=True, git_helper=mock_git - ) - - assert result == cache_path - mock_git.fetch.assert_called() - mock_git.clone.assert_not_called() - - def test_fetch_no_update_uses_cache(self, tmp_path: Path): - """Test that fetch with update=False uses cached version.""" - mock_git = create_autospec(GitHelper) - - cache_path = get_cache_path("https://github.com/owner/repo.git", tmp_path) - cache_path.mkdir(parents=True) - (cache_path / ".git").mkdir() - - result = fetch_plugin( - "github:owner/repo", cache_dir=tmp_path, update=False, git_helper=mock_git - ) - - assert result == cache_path - mock_git.clone.assert_not_called() - mock_git.fetch.assert_not_called() +from openhands.sdk.plugin import Plugin, PluginFetchError +from openhands.sdk.plugin.fetch import fetch_plugin - def test_fetch_no_update_with_ref_checks_out(self, tmp_path: Path): - """Test that fetch with update=False but ref still checks out.""" - mock_git = create_autospec(GitHelper) - cache_path = get_cache_path("https://github.com/owner/repo.git", tmp_path) - cache_path.mkdir(parents=True) - (cache_path / ".git").mkdir() +def test_fetch_git_error_raises_plugin_fetch_error(tmp_path: Path): + """ExtensionFetchError from git failures is wrapped as PluginFetchError.""" + mock_git = create_autospec(GitHelper, instance=True) + mock_git.clone.side_effect = GitCommandError( + "fatal: repository not found", + command=["git", "clone"], + exit_code=128, + ) + with pytest.raises(PluginFetchError, match="Failed to fetch plugin"): fetch_plugin( - "github:owner/repo", + "github:owner/nonexistent", cache_dir=tmp_path, - update=False, - ref="v1.0.0", git_helper=mock_git, ) - mock_git.checkout.assert_called_once_with(cache_path, "v1.0.0") - - def test_fetch_git_error_raises_plugin_fetch_error(self, tmp_path: Path): - """Test that git errors result in PluginFetchError.""" - mock_git = create_autospec(GitHelper) - mock_git.clone.side_effect = GitCommandError( - "fatal: repository not found", command=["git", "clone"], exit_code=128 - ) - - with pytest.raises(PluginFetchError, match="Failed to fetch plugin"): - fetch_plugin( - "github:owner/nonexistent", cache_dir=tmp_path, git_helper=mock_git - ) - - def test_fetch_generic_error_raises_plugin_fetch_error(self, tmp_path: Path): - """Test that generic errors result in PluginFetchError.""" - mock_git = create_autospec(GitHelper) - mock_git.clone.side_effect = RuntimeError("Unexpected error") - - with pytest.raises(PluginFetchError, match="Failed to fetch plugin"): - fetch_plugin("github:owner/repo", cache_dir=tmp_path, git_helper=mock_git) - - -class TestPluginFetchMethod: - """Tests for Plugin.fetch() classmethod.""" - - def test_fetch_delegates_to_fetch_plugin(self, tmp_path: Path): - """Test that Plugin.fetch() delegates to fetch_plugin.""" - plugin_dir = tmp_path / "my-plugin" - plugin_dir.mkdir() - - result = Plugin.fetch(str(plugin_dir)) - assert result == plugin_dir.resolve() - - def test_fetch_local_path_with_tilde(self, tmp_path: Path): - """Test fetching local path with ~ expansion.""" - plugin_dir = tmp_path / "my-plugin" - plugin_dir.mkdir() - - with patch("openhands.sdk.plugin.fetch.Path.home", return_value=tmp_path): - result = Plugin.fetch(str(plugin_dir)) - assert result.exists() - - -class TestRepoPathParameter: - """Tests for repo_path parameter in fetch_plugin() and Plugin.fetch().""" - - def test_fetch_local_path_with_repo_path_raises_error(self, tmp_path: Path): - """Test that repo_path is not supported for local sources.""" - plugin_dir = tmp_path / "monorepo" - plugin_dir.mkdir() - subplugin_dir = plugin_dir / "plugins" / "my-plugin" - subplugin_dir.mkdir(parents=True) - - with pytest.raises( - PluginFetchError, match="repo_path is not supported for local" - ): - fetch_plugin(str(plugin_dir), repo_path="plugins/my-plugin") - def test_fetch_local_path_without_repo_path(self, tmp_path: Path): - """Test fetching local path works without repo_path.""" - plugin_dir = tmp_path / "my-plugin" - plugin_dir.mkdir() +def test_fetch_generic_error_raises_plugin_fetch_error(tmp_path: Path): + """Generic runtime errors are also wrapped as PluginFetchError.""" + mock_git = create_autospec(GitHelper, instance=True) + mock_git.clone.side_effect = RuntimeError("Unexpected error") - result = fetch_plugin(str(plugin_dir)) - assert result == plugin_dir.resolve() - - def test_fetch_github_with_repo_path(self, tmp_path: Path): - """Test fetching from GitHub with repo_path returns subdirectory.""" - mock_git = create_autospec(GitHelper) - - def clone_side_effect(url, dest, **kwargs): - dest.mkdir(parents=True, exist_ok=True) - (dest / ".git").mkdir() - # Create the subdirectory structure - subdir = dest / "plugins" / "sub-plugin" - subdir.mkdir(parents=True) - - mock_git.clone.side_effect = clone_side_effect - - result = fetch_plugin( - "github:owner/monorepo", + with pytest.raises(PluginFetchError, match="Failed to fetch plugin"): + fetch_plugin( + "github:owner/repo", cache_dir=tmp_path, - repo_path="plugins/sub-plugin", git_helper=mock_git, ) - assert result.exists() - assert result.name == "sub-plugin" - assert "plugins" in str(result) - - def test_fetch_github_with_nonexistent_repo_path(self, tmp_path: Path): - """Test fetching from GitHub with nonexistent repo_path raises error.""" - mock_git = create_autospec(GitHelper) - - def clone_side_effect(url, dest, **kwargs): - dest.mkdir(parents=True, exist_ok=True) - (dest / ".git").mkdir() - - mock_git.clone.side_effect = clone_side_effect - - with pytest.raises(PluginFetchError, match="Subdirectory.*not found"): - fetch_plugin( - "github:owner/repo", - cache_dir=tmp_path, - repo_path="nonexistent", - git_helper=mock_git, - ) - - def test_fetch_with_repo_path_and_ref(self, tmp_path: Path): - """Test fetching with both repo_path and ref.""" - mock_git = create_autospec(GitHelper) - - def clone_side_effect(url, dest, **kwargs): - dest.mkdir(parents=True, exist_ok=True) - (dest / ".git").mkdir() - subdir = dest / "plugins" / "my-plugin" - subdir.mkdir(parents=True) - - mock_git.clone.side_effect = clone_side_effect - - result = fetch_plugin( - "github:owner/monorepo", - cache_dir=tmp_path, - ref="v1.0.0", - repo_path="plugins/my-plugin", - git_helper=mock_git, - ) - assert result.exists() - mock_git.clone.assert_called_once() - call_kwargs = mock_git.clone.call_args[1] - assert call_kwargs["branch"] == "v1.0.0" +def test_fetch_local_with_repo_path_raises_plugin_fetch_error( + tmp_path: Path, +): + """repo_path rejection for local sources surfaces as PluginFetchError.""" + plugin_dir = tmp_path / "monorepo" + plugin_dir.mkdir() - def test_plugin_fetch_local_with_repo_path_raises_error(self, tmp_path: Path): - """Test Plugin.fetch() raises error for local source with repo_path.""" - plugin_dir = tmp_path / "monorepo" - plugin_dir.mkdir() + with pytest.raises(PluginFetchError, match="repo_path is not supported for local"): + fetch_plugin(str(plugin_dir), repo_path="plugins/my-plugin") - with pytest.raises( - PluginFetchError, match="repo_path is not supported for local" - ): - Plugin.fetch(str(plugin_dir), repo_path="plugins/my-plugin") - def test_fetch_no_repo_path_returns_root(self, tmp_path: Path): - """Test that fetch without repo_path returns repository root.""" - mock_git = create_autospec(GitHelper) +def test_fetch_uses_default_cache_dir(tmp_path: Path): + """fetch_plugin uses the plugin-specific DEFAULT_CACHE_DIR.""" + mock_git = create_autospec(GitHelper, instance=True) - def clone_side_effect(url, dest, **kwargs): - dest.mkdir(parents=True, exist_ok=True) - (dest / ".git").mkdir() - (dest / "plugins").mkdir() + def clone_side_effect(url, dest, **kwargs): + dest.mkdir(parents=True, exist_ok=True) + (dest / ".git").mkdir() - mock_git.clone.side_effect = clone_side_effect + mock_git.clone.side_effect = clone_side_effect + with patch("openhands.sdk.plugin.fetch.DEFAULT_CACHE_DIR", tmp_path / "cache"): result = fetch_plugin( "github:owner/repo", - cache_dir=tmp_path, - repo_path=None, + cache_dir=None, git_helper=mock_git, ) - assert result.exists() - assert (result / ".git").exists() - - -class TestParsePluginSourceEdgeCases: - """Additional edge case tests for parse_plugin_source.""" - - def test_relative_path_with_slash(self): - """Test parsing paths like 'plugins/my-plugin' (line 108).""" - source_type, url = parse_plugin_source("plugins/my-plugin") - assert source_type == "local" - assert url == "plugins/my-plugin" - - def test_nested_relative_path(self): - """Test parsing nested relative paths.""" - source_type, url = parse_plugin_source("path/to/my/plugin") - assert source_type == "local" - assert url == "path/to/my/plugin" - - -class TestFetchPluginEdgeCases: - """Additional edge case tests for fetch_plugin.""" - - def test_fetch_uses_default_cache_dir(self, tmp_path: Path): - """Test fetch_plugin uses DEFAULT_CACHE_DIR when cache_dir is None.""" - mock_git = create_autospec(GitHelper) - - def clone_side_effect(url, dest, **kwargs): - dest.mkdir(parents=True, exist_ok=True) - (dest / ".git").mkdir() - - mock_git.clone.side_effect = clone_side_effect - - # Patch DEFAULT_CACHE_DIR to use tmp_path - with patch("openhands.sdk.plugin.fetch.DEFAULT_CACHE_DIR", tmp_path / "cache"): - result = fetch_plugin( - "github:owner/repo", - cache_dir=None, # Explicitly None to trigger line 225 - git_helper=mock_git, - ) - - assert result.exists() - assert str(tmp_path / "cache") in str(result) - - -class TestGitHelperErrors: - """Tests for GitHelper error handling paths. - - These tests verify that GitHelper methods properly propagate GitCommandError - from run_git_command when git operations fail. - """ - - def test_clone_called_process_error(self, tmp_path: Path): - """Test clone raises GitCommandError on failure.""" - git = GitHelper() - dest = tmp_path / "repo" - - # Try to clone a non-existent repo - with pytest.raises(GitCommandError, match="git clone"): - git.clone("https://invalid.example.com/nonexistent.git", dest, timeout=5) - - def test_clone_timeout(self, tmp_path: Path): - """Test clone raises GitCommandError on timeout.""" - git = GitHelper() - dest = tmp_path / "repo" - - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) - with pytest.raises(GitCommandError, match="timed out"): - git.clone("https://github.com/owner/repo.git", dest, timeout=1) - - def test_fetch_with_ref(self, tmp_path: Path): - """Test fetch with ref raises GitCommandError when no remote exists.""" - # Create a repo to fetch in - repo = tmp_path / "repo" - repo.mkdir() - subprocess.run(["git", "init"], cwd=repo, check=True) - subprocess.run( - ["git", "config", "user.email", "test@test.com"], cwd=repo, check=True - ) - subprocess.run(["git", "config", "user.name", "Test"], cwd=repo, check=True) - (repo / "file.txt").write_text("content") - subprocess.run(["git", "add", "."], cwd=repo, check=True) - subprocess.run(["git", "commit", "-m", "Initial"], cwd=repo, check=True) - - git = GitHelper() - # This will fail because there's no remote - with pytest.raises(GitCommandError, match="git fetch"): - git.fetch(repo, ref="main") - - def test_fetch_called_process_error(self, tmp_path: Path): - """Test fetch raises GitCommandError on failure.""" - git = GitHelper() - repo = tmp_path / "not-a-repo" - repo.mkdir() - - with pytest.raises(GitCommandError, match="git fetch"): - git.fetch(repo) - - def test_fetch_timeout(self, tmp_path: Path): - """Test fetch raises GitCommandError on timeout.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) - with pytest.raises(GitCommandError, match="timed out"): - git.fetch(repo, timeout=1) - - def test_checkout_called_process_error(self, tmp_path: Path): - """Test checkout raises GitCommandError on failure.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - subprocess.run(["git", "init"], cwd=repo, check=True) + assert result.exists() + assert str(tmp_path / "cache") in str(result) - with pytest.raises(GitCommandError, match="git checkout"): - git.checkout(repo, "nonexistent-ref") - def test_checkout_timeout(self, tmp_path: Path): - """Test checkout raises GitCommandError on timeout.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() +def test_plugin_fetch_delegates(tmp_path: Path): + """Plugin.fetch() delegates to fetch_plugin for local paths.""" + plugin_dir = tmp_path / "my-plugin" + plugin_dir.mkdir() - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) - with pytest.raises(GitCommandError, match="timed out"): - git.checkout(repo, "main", timeout=1) + result = Plugin.fetch(str(plugin_dir)) + assert result == plugin_dir.resolve() - def test_reset_hard_called_process_error(self, tmp_path: Path): - """Test reset_hard raises GitCommandError on failure.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - subprocess.run(["git", "init"], cwd=repo, check=True) - - with pytest.raises(GitCommandError, match="git reset"): - git.reset_hard(repo, "nonexistent-ref") - - def test_reset_hard_timeout(self, tmp_path: Path): - """Test reset_hard raises GitCommandError on timeout.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) - with pytest.raises(GitCommandError, match="timed out"): - git.reset_hard(repo, "HEAD", timeout=1) - - def test_get_current_branch_called_process_error(self, tmp_path: Path): - """Test get_current_branch raises GitCommandError on failure.""" - git = GitHelper() - repo = tmp_path / "not-a-repo" - repo.mkdir() - - with pytest.raises(GitCommandError, match="git rev-parse"): - git.get_current_branch(repo) - - def test_get_current_branch_timeout(self, tmp_path: Path): - """Test get_current_branch raises GitCommandError on timeout.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_run.side_effect = subprocess.TimeoutExpired(cmd=["git"], timeout=1) - with pytest.raises(GitCommandError, match="timed out"): - git.get_current_branch(repo, timeout=1) - - -class TestGetDefaultBranch: - """Tests for GitHelper.get_default_branch method.""" - - def test_get_default_branch_returns_main(self, tmp_path: Path): - """Test get_default_branch extracts branch name from origin/HEAD.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_result = subprocess.CompletedProcess( - args=["git"], - returncode=0, - stdout="refs/remotes/origin/main\n", - stderr="", - ) - mock_run.return_value = mock_result - - result = git.get_default_branch(repo) - - assert result == "main" - # Verify the correct command was called - call_args = mock_run.call_args[0][0] - assert call_args == ["git", "symbolic-ref", "refs/remotes/origin/HEAD"] - - def test_get_default_branch_returns_master(self, tmp_path: Path): - """Test get_default_branch works with master as default branch.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_result = subprocess.CompletedProcess( - args=["git"], - returncode=0, - stdout="refs/remotes/origin/master\n", - stderr="", - ) - mock_run.return_value = mock_result - - result = git.get_default_branch(repo) - - assert result == "master" - - def test_get_default_branch_returns_none_when_not_set(self, tmp_path: Path): - """Test get_default_branch returns None when origin/HEAD is not set. - - This can happen with: - - Bare clones - - Repos where origin/HEAD was never configured - - Some git server configurations - """ - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_result = subprocess.CompletedProcess( - args=["git"], - returncode=1, - stdout="", - stderr="fatal: ref refs/remotes/origin/HEAD is not a symbolic ref", - ) - mock_run.return_value = mock_result - - result = git.get_default_branch(repo) - - assert result is None - - def test_get_default_branch_returns_none_on_unexpected_format(self, tmp_path: Path): - """Test get_default_branch returns None for unexpected output format.""" - git = GitHelper() - repo = tmp_path / "repo" - repo.mkdir() - - with patch("openhands.sdk.git.utils.subprocess.run") as mock_run: - mock_result = subprocess.CompletedProcess( - args=["git"], - returncode=0, - stdout="unexpected-format\n", # Doesn't start with expected prefix - stderr="", - ) - mock_run.return_value = mock_result - - result = git.get_default_branch(repo) - - assert result is None - - -class TestCacheLocking: - """Tests for cache directory locking behavior.""" - - def test_lock_file_created_during_clone(self, tmp_path: Path): - """Test that a lock file is created when cloning.""" - from openhands.sdk.git.cached_repo import try_cached_clone_or_update - - cache_dir = tmp_path / "cache" - repo_path = cache_dir / "test-repo" - - mock_git = create_autospec(GitHelper, instance=True) - - # Track whether lock file exists during clone - lock_existed_during_clone = [] - - def mock_clone(url, dest, depth=None, branch=None, timeout=120): - lock_path = repo_path.with_suffix(".lock") - lock_existed_during_clone.append(lock_path.exists()) - - mock_git.clone.side_effect = mock_clone - - try_cached_clone_or_update( - url="https://github.com/test/repo.git", - repo_path=repo_path, - git_helper=mock_git, - ) - - # Lock file should have existed during the clone operation - assert lock_existed_during_clone[0] is True - - def test_lock_timeout_returns_none(self, tmp_path: Path): - """Test that lock timeout returns None gracefully.""" - from filelock import FileLock - - from openhands.sdk.git.cached_repo import try_cached_clone_or_update - - cache_dir = tmp_path / "cache" - cache_dir.mkdir(parents=True) - repo_path = cache_dir / "test-repo" - - # Pre-acquire the lock to simulate another process holding it - lock_path = repo_path.with_suffix(".lock") - external_lock = FileLock(lock_path) - external_lock.acquire() - - try: - mock_git = create_autospec(GitHelper, instance=True) - - # Try to clone with a very short timeout - result = try_cached_clone_or_update( - url="https://github.com/test/repo.git", - repo_path=repo_path, - git_helper=mock_git, - lock_timeout=0.1, # Very short timeout - ) - - # Should return None due to lock timeout - assert result is None - # Clone should not have been called - mock_git.clone.assert_not_called() - finally: - external_lock.release() - - def test_lock_released_after_operation(self, tmp_path: Path): - """Test that lock is released after successful operation.""" - from filelock import FileLock - - from openhands.sdk.git.cached_repo import try_cached_clone_or_update - - cache_dir = tmp_path / "cache" - repo_path = cache_dir / "test-repo" - - mock_git = create_autospec(GitHelper, instance=True) - - try_cached_clone_or_update( - url="https://github.com/test/repo.git", - repo_path=repo_path, - git_helper=mock_git, - ) - - # Lock should be released - we should be able to acquire it immediately - lock_path = repo_path.with_suffix(".lock") - lock = FileLock(lock_path) - lock.acquire(timeout=0) # Should not block - lock.release() - - def test_lock_released_on_error(self, tmp_path: Path): - """Test that lock is released even when operation fails.""" - from filelock import FileLock - - from openhands.sdk.git.cached_repo import try_cached_clone_or_update - - cache_dir = tmp_path / "cache" - repo_path = cache_dir / "test-repo" - - mock_git = create_autospec(GitHelper, instance=True) - mock_git.clone.side_effect = GitCommandError( - "Clone failed", command=["git", "clone"], exit_code=1, stderr="error" - ) - - result = try_cached_clone_or_update( - url="https://github.com/test/repo.git", - repo_path=repo_path, - git_helper=mock_git, - ) - assert result is None +def test_plugin_fetch_local_with_repo_path_raises_error(tmp_path: Path): + """Plugin.fetch() raises PluginFetchError for local + repo_path.""" + plugin_dir = tmp_path / "monorepo" + plugin_dir.mkdir() - # Lock should still be released - lock_path = repo_path.with_suffix(".lock") - lock = FileLock(lock_path) - lock.acquire(timeout=0) - lock.release() + with pytest.raises(PluginFetchError, match="repo_path is not supported for local"): + Plugin.fetch(str(plugin_dir), repo_path="plugins/my-plugin") From 64605356506a786f7e58995c661a8284120ee5ac Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 11:36:56 -0600 Subject: [PATCH 08/10] cleaning up error messages --- openhands-sdk/openhands/sdk/plugin/fetch.py | 2 +- openhands-sdk/openhands/sdk/skills/fetch.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/openhands-sdk/openhands/sdk/plugin/fetch.py b/openhands-sdk/openhands/sdk/plugin/fetch.py index 2fcea18577..1310e5717c 100644 --- a/openhands-sdk/openhands/sdk/plugin/fetch.py +++ b/openhands-sdk/openhands/sdk/plugin/fetch.py @@ -107,4 +107,4 @@ def fetch_plugin_with_resolution( git_helper=git_helper, ) except ExtensionFetchError as exc: - raise PluginFetchError(str(exc).replace("extension", "plugin")) from exc + raise PluginFetchError("Failed to fetch plugin") from exc diff --git a/openhands-sdk/openhands/sdk/skills/fetch.py b/openhands-sdk/openhands/sdk/skills/fetch.py index 57618d0e15..3c403c4a1a 100644 --- a/openhands-sdk/openhands/sdk/skills/fetch.py +++ b/openhands-sdk/openhands/sdk/skills/fetch.py @@ -91,4 +91,4 @@ def fetch_skill_with_resolution( git_helper=git_helper, ) except ExtensionFetchError as exc: - raise SkillFetchError(str(exc)) from exc + raise SkillFetchError("Failed to fetch skill") from exc From e1413f5bb7d72b69e1855327563df2cc786c5a38 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Fri, 10 Apr 2026 11:41:00 -0600 Subject: [PATCH 09/10] Fix PluginFetchError to preserve original error message The except block was discarding the ExtensionFetchError message and replacing it with a generic 'Failed to fetch plugin'. Now it carries the original message through with 'extension' replaced by 'plugin'. Co-authored-by: openhands --- openhands-sdk/openhands/sdk/plugin/fetch.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/plugin/fetch.py b/openhands-sdk/openhands/sdk/plugin/fetch.py index 1310e5717c..a0ee5d482c 100644 --- a/openhands-sdk/openhands/sdk/plugin/fetch.py +++ b/openhands-sdk/openhands/sdk/plugin/fetch.py @@ -107,4 +107,5 @@ def fetch_plugin_with_resolution( git_helper=git_helper, ) except ExtensionFetchError as exc: - raise PluginFetchError("Failed to fetch plugin") from exc + msg = str(exc).replace("extension", "plugin") + raise PluginFetchError(msg) from exc From 19cf0cbf04a82eef910706c8596b2f2650873054 Mon Sep 17 00:00:00 2001 From: Calvin Smith Date: Mon, 13 Apr 2026 10:21:12 -0600 Subject: [PATCH 10/10] addressing reviewer comments --- openhands-sdk/openhands/sdk/extensions/fetch.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/openhands-sdk/openhands/sdk/extensions/fetch.py b/openhands-sdk/openhands/sdk/extensions/fetch.py index c885eb2751..31d644a00b 100644 --- a/openhands-sdk/openhands/sdk/extensions/fetch.py +++ b/openhands-sdk/openhands/sdk/extensions/fetch.py @@ -1,9 +1,7 @@ """Fetching utilities for extensions.""" -from __future__ import annotations - import hashlib -from enum import Enum +from enum import StrEnum from pathlib import Path from openhands.sdk.git.cached_repo import GitHelper, try_cached_clone_or_update @@ -18,7 +16,7 @@ class ExtensionFetchError(Exception): """Raised when fetching an extension fails.""" -class SourceType(str, Enum): +class SourceType(StrEnum): """Classification of an extension source. LOCAL -- a filesystem path (absolute, home-relative, or dot-relative).