-
Notifications
You must be signed in to change notification settings - Fork 106
feat(plugin): Add Plugin.fetch() for remote plugin fetching and caching #1647
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Implements #1645 - adds the ability to fetch plugins from remote sources (GitHub repositories, git URLs) and cache them locally. Changes: - Add Plugin.fetch() classmethod to fetch from remote sources - Add parse_plugin_source() to parse various source formats: - GitHub shorthand: 'github:owner/repo' - Git URLs: HTTPS, SSH, git:// protocol - Local paths (returned as-is) - Add PluginFetchError exception for fetch failures - Implement caching at ~/.openhands/cache/plugins/ - Support shallow clones for efficiency - Support specific ref (branch/tag/commit) checkout - Add comprehensive unit tests (34 new tests) Co-authored-by: openhands <[email protected]>
- Add blank line after imports in fetch.py - Remove unused import PluginFetchError in plugin.py - Reformat long lines in test file Co-authored-by: openhands <[email protected]>
- Create GitHelper class to encapsulate all git operations (clone, fetch, checkout, etc.) - Refactor fetch.py to use GitHelper via dependency injection - Update unit tests to mock GitHelper instead of subprocess.run - Add integration tests for real git operations - Add support for file:// URLs for local testing This improves test coverage by: 1. Allowing unit tests to execute actual fetch.py logic while mocking git operations 2. Providing separate integration tests that use real git operations Co-authored-by: openhands <[email protected]>
Add comprehensive tests to improve coverage for the plugin module: - fetch.py: Add tests for set_git_helper(), relative path parsing, default cache_dir fallback, and PluginFetchError re-raise - git_helper.py: Add unit tests for error handling paths including CalledProcessError and TimeoutExpired for all git operations - plugin.py: Add tests for plugin loading edge cases including manifest parsing, skills/agents/commands loading, and error handling - types.py: Add tests for AgentDefinition and CommandDefinition frontmatter parsing including complex field handling Simplify integration test to avoid skill loading complexity. Co-authored-by: openhands <[email protected]>
Co-authored-by: openhands <[email protected]>
The --forked flag was preventing coverage from being collected properly because pytest-forked uses os.fork() which doesn't combine coverage data. Changes: - Split SDK test run: forked tests run separately, then non-forked tests with --cov-append to combine coverage data - Add 'forked' pytest marker to identify tests needing process isolation - Add coverage configuration with parallel mode and branch coverage - Add coverage report exclusions for common non-coverable patterns This allows most tests to run without forking for accurate coverage, while still supporting tests that need process isolation. Co-authored-by: openhands <[email protected]>
The change to run tests without --forked exposed pre-existing test pollution issues in test_state_serialization.py where tests share state incorrectly. Reverting to the original --forked approach until those tests are fixed. Coverage collection will remain affected by --forked, but tests will pass. Co-authored-by: openhands <[email protected]>
Requested Changes: Add
|
Add plugin_path field to StartConversationRequest and pass it as subpath parameter to Plugin.fetch(). This enables fetching plugins from subdirectories within repositories (e.g., monorepos with multiple plugins). Changes: - models.py: Add plugin_path optional field to StartConversationRequest - conversation_service.py: Pass plugin_path as subpath to Plugin.fetch() - test_conversation_service.py: Update tests to verify subpath handling Note: This depends on PR #1647 adding the subpath parameter to Plugin.fetch() Co-authored-by: openhands <[email protected]>
Add optional subpath parameter to both fetch_plugin() and Plugin.fetch() methods. This allows fetching plugins from subdirectories within a repository, which is needed when plugin paths are specified separately rather than embedded in the source string. Changes: - Add subpath parameter to fetch_plugin() in fetch.py - Add subpath parameter to Plugin.fetch() in plugin.py - Apply subpath to both local paths and remote repositories - Strip leading/trailing slashes from subpath - Raise PluginFetchError if subpath doesn't exist - Add 8 new unit tests for subpath functionality Addresses review comment on PR #1647. Co-authored-by: openhands <[email protected]>
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
Co-authored-by: openhands <[email protected]>
Add plugin_path field to StartConversationRequest and pass it as subpath parameter to Plugin.fetch(). This enables fetching plugins from subdirectories within repositories (e.g., monorepos with multiple plugins). Changes: - models.py: Add plugin_path optional field to StartConversationRequest - conversation_service.py: Pass plugin_path as subpath to Plugin.fetch() - test_conversation_service.py: Update tests to verify subpath handling Note: This depends on PR #1647 adding the subpath parameter to Plugin.fetch() Co-authored-by: openhands <[email protected]>
|
|
||
|
|
||
| class GitHelper: | ||
| """Abstraction for git operations, enabling easy mocking in tests.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already have git clone in the codebase?
| raise GitError(f"Fetch timed out after {timeout}s") from e | ||
|
|
||
| def checkout(self, repo_path: Path, ref: str, timeout: int = 30) -> None: | ||
| """Checkout a ref (branch, tag, or commit). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we already needed to do git checkout, so I wonder if this code duplicates it. Maybe in the workspace package?
I believe we checkout:
- user's repo (I think this is a
workspacething) - organization skills repo, e.g. github/acme/.openhands/skills or so
This kind of feature feels like the second, like checking out a repo for this user/org from some remote?
| except subprocess.TimeoutExpired as e: | ||
| raise GitError(f"Reset timed out after {timeout}s") from e | ||
|
|
||
| def get_current_branch(self, repo_path: Path, timeout: int = 10) -> str | None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, we must have this code already, is this a duplicate?
| ] | ||
|
|
||
| # Coverage configuration - needed for --forked tests to collect coverage data | ||
| [tool.coverage.run] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem part of this PR, maybe it could be in a separate PR if you'd like it in?
|
@OpenHands Install yourself GOAL: answer a code design question: does this feature implementation belong in the sdk package of the SDK (like this PR), in the workspace package of the SDK ( I want you to make a throughout research in the SDK packages and try to understand the git-related operations and responsibilities. Answer directly in a github comment. |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
Thanks for flagging this — I dug through the SDK packages to see where “git-ish” responsibilities currently live. What exists todaySDK (
Workspace (
So where should
|
Summary of work completed
Checklist vs user request
Code changes / pushes
|
Summary
Implements #1645 - adds the ability to fetch plugins from remote sources (GitHub repositories, git URLs) and cache them locally. This is a building block for the Plugin Directory feature (OpenHands/OpenHands#12088).
Changes
New Function:
Plugin.fetch()Source Parsing
The
parse_plugin_source()function handles various source formats:"github:owner/repo"- GitHub repository shorthand"https://github.com/owner/repo.git"- Full git URL"[email protected]:owner/repo.git"- SSH git URL"/local/path"- Local path (returned as-is)Caching Strategy
~/.openhands/cache/plugins/{name}-{hash}/--depth 1)Usage Example
Files Changed
openhands-sdk/openhands/sdk/plugin/fetch.py- New module with fetching logicopenhands-sdk/openhands/sdk/plugin/plugin.py- Addedfetch()classmethodopenhands-sdk/openhands/sdk/plugin/__init__.py- Export new typestests/sdk/plugin/test_plugin_fetch.py- 34 new unit testsChecklist
Plugin.fetch("github:owner/repo")clones and returns local pathPlugin.fetch("https://...")works with any git URLPlugin.fetch("/local/path")returns path unchangedrefparameter checks out specific branch/tagPluginFetchErrorRelated Issues
@jpshackelford can click here to continue refining the PR
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.12-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:4b4b564-pythonRun
All tags pushed for this build
About Multi-Architecture Support
4b4b564-python) is a multi-arch manifest supporting both amd64 and arm644b4b564-python-amd64) are also available if needed