diff --git a/openhands-sdk/openhands/sdk/plugin/plugin.py b/openhands-sdk/openhands/sdk/plugin/plugin.py index c2583059e5..122f9a7a26 100644 --- a/openhands-sdk/openhands/sdk/plugin/plugin.py +++ b/openhands-sdk/openhands/sdk/plugin/plugin.py @@ -53,6 +53,14 @@ class Plugin(BaseModel): ├── .mcp.json # External tool configuration (optional) └── README.md # Plugin documentation ``` + + Coming from Claude Code? Note one behavioral difference: OpenHands does + **not** auto-discover plugins dropped into a skills directory + (``.agents/skills/``, ``.openhands/skills/``, ``~/.claude/skills/``...). + A plugin folder placed there is ignored (the skills loader will log a + warning). Load plugins explicitly instead, e.g. + ``plugins=[PluginSource(source="path/to/plugin")]`` or via + ``load_installed_plugins()``. """ manifest: PluginManifest = Field(description="Plugin manifest from plugin.json") diff --git a/openhands-sdk/openhands/sdk/skills/skill.py b/openhands-sdk/openhands/sdk/skills/skill.py index 86b27ddafa..36b07ffc6c 100644 --- a/openhands-sdk/openhands/sdk/skills/skill.py +++ b/openhands-sdk/openhands/sdk/skills/skill.py @@ -41,6 +41,7 @@ load_mcp_config, update_skills_repository, validate_skill_name, + warn_on_plugin_dirs, ) from openhands.sdk.utils import DEFAULT_TRUNCATE_NOTICE, maybe_truncate from openhands.sdk.utils.path import to_posix_path @@ -733,6 +734,11 @@ def load_skills_from_dir( skill_md_dirs = {skill_md.parent for skill_md in skill_md_files} regular_md_files = find_regular_md_files(skill_dir, skill_md_dirs) + # Skills directories do not auto-load plugins. Warn (instead of silently + # ignoring) when a plugin folder is dropped here, so users coming from + # Claude Code know to load it explicitly via PluginSource. + warn_on_plugin_dirs(skill_dir, skill_md_dirs) + # Load SKILL.md files (auto-detected and validated in Skill.load) # Wrap each load in try/except to ensure one bad skill doesn't break all loading for skill_md_path in skill_md_files: diff --git a/openhands-sdk/openhands/sdk/skills/utils.py b/openhands-sdk/openhands/sdk/skills/utils.py index 3ed62e40c1..610bb39fbf 100644 --- a/openhands-sdk/openhands/sdk/skills/utils.py +++ b/openhands-sdk/openhands/sdk/skills/utils.py @@ -149,6 +149,74 @@ def find_skill_md(skill_dir: Path) -> Path | None: return None +# Plugin manifest locations. Mirrors ``PLUGIN_MANIFEST_DIRS`` / +# ``PLUGIN_MANIFEST_FILE`` in ``openhands.sdk.plugin.plugin``; duplicated here +# (rather than imported) because that module imports from this one, which would +# create a circular import. Used only to detect plugin folders mistakenly +# dropped into a skills directory so we can warn the user. +PLUGIN_MANIFEST_DIRS = (".plugin", ".claude-plugin") +PLUGIN_MANIFEST_FILE = "plugin.json" + + +def find_plugin_manifest(plugin_dir: Path) -> Path | None: + """Find a plugin manifest (``plugin.json``) inside a directory. + + Checks both ``.plugin/`` and ``.claude-plugin/`` subdirectories. + + Args: + plugin_dir: Path to the directory to inspect. + + Returns: + Path to the manifest if found, None otherwise. + """ + if not plugin_dir.is_dir(): + return None + for manifest_dir in PLUGIN_MANIFEST_DIRS: + candidate = plugin_dir / manifest_dir / PLUGIN_MANIFEST_FILE + if candidate.is_file(): + return candidate + return None + + +def warn_on_plugin_dirs(skill_dir: Path, loaded_skill_dirs: set[Path]) -> list[Path]: + """Warn about plugin folders found inside a skills directory. + + Skills directories do not auto-load plugins. A folder containing only a + plugin manifest (``.claude-plugin/plugin.json`` or ``.plugin/plugin.json``) + but no ``SKILL.md`` would otherwise be silently ignored, which is confusing + for users coming from Claude Code. This emits one actionable warning per + such folder, pointing them at the explicit ``PluginSource`` load path. + + Args: + skill_dir: The skills directory being scanned. + loaded_skill_dirs: Directories already recognized as skills (skipped so + a plugin that also ships a top-level ``SKILL.md`` isn't flagged). + + Returns: + List of plugin directories that triggered a warning. + """ + if not skill_dir.is_dir(): + return [] + flagged: list[Path] = [] + for subdir in sorted(p for p in skill_dir.iterdir() if p.is_dir()): + if subdir in loaded_skill_dirs: + continue + manifest = find_plugin_manifest(subdir) + if manifest is None: + continue + flagged.append(subdir) + logger.warning( + "Found a plugin manifest at '%s' inside skills directory '%s', but " + "skills directories do not auto-load plugins; this folder is being " + "ignored. Load the plugin explicitly, e.g. " + 'plugins=[PluginSource(source="%s")].', + manifest, + skill_dir, + subdir, + ) + return flagged + + def find_mcp_config(skill_dir: Path) -> Path | None: """Find .mcp.json file in a skill directory. diff --git a/tests/sdk/skills/test_skill_utils.py b/tests/sdk/skills/test_skill_utils.py index 4948c074be..058719f155 100644 --- a/tests/sdk/skills/test_skill_utils.py +++ b/tests/sdk/skills/test_skill_utils.py @@ -1,5 +1,6 @@ """Tests for the skill system.""" +import json import tempfile from pathlib import Path @@ -13,10 +14,12 @@ load_skills_from_dir, ) from openhands.sdk.skills.utils import ( + find_plugin_manifest, find_regular_md_files, find_skill_md, find_skill_md_directories, find_third_party_files, + warn_on_plugin_dirs, ) from openhands.sdk.utils.path import to_posix_path from tests.platform_utils import require_case_sensitive_fs, symlink_or_skip @@ -838,3 +841,73 @@ def test_find_third_party_files_collision_winner_deterministic(tmp_path, monkeyp monkeypatch.setattr(Path, "iterdir", lambda self: iter(entries)) files = find_third_party_files(tmp_path, {"agents.md": "agents"}) assert [f.name for f in files] == ["AGENTS.md"] + + +def _make_plugin_dir(parent: Path, name: str, manifest_dir: str = ".claude-plugin"): + """Create a plugin folder containing a manifest but no SKILL.md.""" + plugin_dir = parent / name + (plugin_dir / manifest_dir).mkdir(parents=True) + (plugin_dir / manifest_dir / "plugin.json").write_text( + json.dumps({"name": name, "version": "1.0.0"}) + ) + return plugin_dir + + +@pytest.mark.parametrize("manifest_dir", [".claude-plugin", ".plugin"]) +def test_find_plugin_manifest_detects_both_layouts(tmp_path, manifest_dir): + plugin_dir = _make_plugin_dir(tmp_path, "my-plugin", manifest_dir) + manifest = find_plugin_manifest(plugin_dir) + assert manifest == plugin_dir / manifest_dir / "plugin.json" + + +def test_find_plugin_manifest_returns_none_without_manifest(tmp_path): + (tmp_path / "plain").mkdir() + assert find_plugin_manifest(tmp_path / "plain") is None + assert find_plugin_manifest(tmp_path / "does-not-exist") is None + + +def test_warn_on_plugin_dirs_flags_manifest_only_folder(tmp_path, caplog): + plugin_dir = _make_plugin_dir(tmp_path, "my-plugin") + + with caplog.at_level("WARNING"): + flagged = warn_on_plugin_dirs(tmp_path, set()) + + assert flagged == [plugin_dir] + assert "do not auto-load plugins" in caplog.text + assert "PluginSource" in caplog.text + + +def test_warn_on_plugin_dirs_skips_loaded_skill_dirs(tmp_path, caplog): + """A plugin that also ships a top-level SKILL.md must not be flagged.""" + plugin_dir = _make_plugin_dir(tmp_path, "skill-and-plugin") + (plugin_dir / "SKILL.md").write_text(CONTENT) + + with caplog.at_level("WARNING"): + flagged = warn_on_plugin_dirs(tmp_path, {plugin_dir}) + + assert flagged == [] + assert caplog.text == "" + + +def test_warn_on_plugin_dirs_ignores_plain_folders(tmp_path): + (tmp_path / "not-a-plugin").mkdir() + assert warn_on_plugin_dirs(tmp_path, set()) == [] + + +def test_load_skills_from_dir_warns_on_plugin_folder(tmp_path, caplog): + """Integration: loading a skills dir warns about a dropped-in plugin.""" + _make_plugin_dir(tmp_path, "my-plugin") + # A real skill alongside the plugin should still load normally. + real_skill = tmp_path / "real-skill" + real_skill.mkdir() + (real_skill / "SKILL.md").write_text(CONTENT) + + with caplog.at_level("WARNING"): + repo_skills, knowledge_skills, agent_skills = load_skills_from_dir(tmp_path) + + # The plugin folder is ignored (not loaded as a skill)... + all_loaded = {**repo_skills, **knowledge_skills, **agent_skills} + assert "my-plugin" not in all_loaded + assert "real-skill" in all_loaded + # ...but the user gets a clear, actionable warning. + assert "do not auto-load plugins" in caplog.text