diff --git a/docs/configuration.md b/docs/configuration.md index 31547ca6e..5a9b937ee 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -2323,9 +2323,10 @@ plugins/ ``` Plugin skills use the same progressive loading and `$skill-name` invocation as workspace -skills. A workspace skill wins when it has the same name as a plugin skill; plugin skills win -over built-in skills. Invalid manifests, invalid Agent Skills, nested skill directories, and -paths that resolve outside the plugin root are ignored. +skills after the plugin is explicitly enabled. Disabling a plugin removes both its skills and +MCP servers from the agent. A workspace skill wins when it has the same name as an enabled +plugin skill; plugin skills win over built-in skills. Invalid manifests, invalid Agent Skills, +nested skill directories, and paths that resolve outside the plugin root are ignored. Portable MCP servers declared in `mcp.json` appear in **Apps**, but are never started merely because a package exists. Enabling a plugin there is the explicit trust decision that activates @@ -2334,15 +2335,18 @@ package paths before launch, and hot-reloads MCP connections. Explicit `tools.mc configuration wins over a plugin server if their host names collide. The v1 host currently supports plugin `stdio` servers; unsupported remote transports are skipped independently. +Treat enabled plugins as local code running with the nanobot user's privileges. Manifest +permissions are descriptive; nanobot does not currently enforce them with an OS sandbox. + Plugins may optionally declare a shell-free `extensions.dev.nanobot.installCommand` array. The local WebUI runs it once per plugin version before first enable; remote WebUI clients cannot run plugin setup unless remote package installation was explicitly allowed. Agent Plugins v1 does not define a registry, so package distribution remains separate from discovery and execution. CLI Apps installed from the WebUI use the same package layout. nanobot installs the CLI through -its catalog adapter, then writes a skills-only Agent Plugin under `/plugins/`; updates -refresh that package and uninstall removes it. The external executable remains managed by the -CLI Apps installer rather than by the Agent Plugins manifest. +its catalog adapter, then writes and enables a skills-only Agent Plugin under +`/plugins/`; updates refresh that package and uninstall removes it. The external +executable remains managed by the CLI Apps installer rather than by the Agent Plugins manifest. ## Tool Hint Max Length diff --git a/nanobot/agent/agent_plugins.py b/nanobot/agent/agent_plugins.py index 3af973958..493283a67 100644 --- a/nanobot/agent/agent_plugins.py +++ b/nanobot/agent/agent_plugins.py @@ -12,6 +12,7 @@ from pathlib import Path from typing import Any, cast import yaml +from filelock import FileLock from loguru import logger from nanobot.config.loader import get_config_path @@ -41,6 +42,7 @@ _MCP_SERVER_FIELDS = { "stdio": {"type", "command", "args", "env", "cwd"}, } _SETUP_ENV = {"HOME", "LANG", "LC_ALL", "LOGNAME", "PATH", "SHELL", "TMPDIR", "USER"} +_SETUP_TIMEOUT_SECONDS = 600 @dataclass(frozen=True) @@ -111,6 +113,15 @@ def discover_agent_plugin_skills(workspace: Path) -> list[AgentPluginSkill]: return skills +def enabled_agent_plugin_skills(workspace: Path) -> list[AgentPluginSkill]: + """Return skills from plugins the user has explicitly enabled.""" + skills: list[AgentPluginSkill] = [] + for plugin in discover_agent_plugins(workspace): + if _enabled(workspace, plugin.name): + skills.extend(_discover_plugin_skills(plugin.name, plugin.root)) + return skills + + def _load_manifest(plugin_root: Path) -> AgentPlugin | None: manifest = _contained_file(plugin_root / "plugin.json", plugin_root) if manifest is None: @@ -214,18 +225,21 @@ def agent_plugins_payload(workspace: Path) -> dict[str, Any]: def set_agent_plugin_enabled(workspace: Path, name: str, enabled: bool) -> dict[str, Any]: - """Enable or disable one installed plugin's executable MCP components.""" + """Enable or disable one installed plugin.""" plugin = next((item for item in discover_agent_plugins(workspace) if item.name == name), None) if plugin is None: raise ValueError(f"unknown Agent Plugin '{name}'") data = _plugin_data_dir(workspace, plugin.name, create=True) - if enabled: - if plugin.install_command and _setup_version(workspace, plugin.name) != (plugin.version or "unknown"): - _run_install(plugin, data) - _write_state(data / "setup-version", plugin.version or "unknown") - _write_state(data / "enabled", "1") - else: - (data / "enabled").unlink(missing_ok=True) + with FileLock(str(data / ".state.lock"), timeout=_SETUP_TIMEOUT_SECONDS + 10): + if enabled: + if plugin.install_command and _setup_version(workspace, plugin.name) != ( + plugin.version or "unknown" + ): + _run_install(plugin, data) + _write_state(data / "setup-version", plugin.version or "unknown") + _write_state(data / "enabled", "1") + else: + (data / "enabled").unlink(missing_ok=True) payload = agent_plugins_payload(workspace) payload["last_action"] = { "ok": True, @@ -478,7 +492,7 @@ def _run_install(plugin: AgentPlugin, data: Path) -> None: env=env, capture_output=True, text=True, - timeout=600, + timeout=_SETUP_TIMEOUT_SECONDS, check=False, ) except subprocess.TimeoutExpired as exc: diff --git a/nanobot/agent/skills.py b/nanobot/agent/skills.py index 84a7c2ee1..c9836d594 100644 --- a/nanobot/agent/skills.py +++ b/nanobot/agent/skills.py @@ -5,13 +5,10 @@ import os import re import shutil from pathlib import Path -from typing import TYPE_CHECKING, Any, cast +from typing import Any, cast import yaml -if TYPE_CHECKING: - from nanobot.agent.agent_plugins import AgentPluginSkill - # Default builtin skills directory (relative to this file) BUILTIN_SKILLS_DIR = Path(__file__).parent.parent / "skills" @@ -36,7 +33,6 @@ class SkillsLoader: self.workspace_skills = workspace / "skills" self.builtin_skills = builtin_skills_dir or BUILTIN_SKILLS_DIR self.disabled_skills = disabled_skills or set() - self.plugin_skills: list[AgentPluginSkill] = [] def _skill_entries_from_dir(self, base: Path, source: str, *, skip_names: set[str] | None = None) -> list[dict[str, str]]: if not base.exists(): @@ -64,12 +60,12 @@ class SkillsLoader: Returns: List of skill info dicts with 'name', 'path', 'source'. """ - from nanobot.agent.agent_plugins import discover_agent_plugin_skills + from nanobot.agent.agent_plugins import enabled_agent_plugin_skills - self.plugin_skills = discover_agent_plugin_skills(self.workspace) + plugin_skills = enabled_agent_plugin_skills(self.workspace) skills = self._skill_entries_from_dir(self.workspace_skills, "workspace") seen_names = {entry["name"] for entry in skills} - for plugin_skill in self.plugin_skills: + for plugin_skill in plugin_skills: if plugin_skill.name in seen_names: continue skills.append( @@ -106,11 +102,9 @@ class SkillsLoader: workspace_path = self.workspace_skills / name / "SKILL.md" if workspace_path.exists(): return workspace_path.read_text(encoding="utf-8") - if not self.plugin_skills: - from nanobot.agent.agent_plugins import discover_agent_plugin_skills + from nanobot.agent.agent_plugins import enabled_agent_plugin_skills - self.plugin_skills = discover_agent_plugin_skills(self.workspace) - for plugin_skill in self.plugin_skills: + for plugin_skill in enabled_agent_plugin_skills(self.workspace): if plugin_skill.name == name and plugin_skill.path.is_file(): return plugin_skill.path.read_text(encoding="utf-8") if self.builtin_skills: diff --git a/nanobot/apps/cli/service.py b/nanobot/apps/cli/service.py index eef3f5ad4..137d76fab 100644 --- a/nanobot/apps/cli/service.py +++ b/nanobot/apps/cli/service.py @@ -1149,11 +1149,14 @@ Use the `run_cli_app` tool with `name="{name}"` for command execution. Do not in shutil.rmtree(legacy_dir) def _record_installed(self, app: dict[str, Any]) -> dict[str, Any]: + from nanobot.agent.agent_plugins import set_agent_plugin_enabled + installed = self._load_installed() entry = self._installed_entry(app) installed[str(app["name"])] = entry self._save_installed(installed) self.install_skill(app) + set_agent_plugin_enabled(self.workspace, _safe_skill_name(str(app["name"])), True) return entry def install(self, name: str) -> dict[str, Any]: diff --git a/tests/agent/test_agent_plugins.py b/tests/agent/test_agent_plugins.py index d138870dd..29b7a76d3 100644 --- a/tests/agent/test_agent_plugins.py +++ b/tests/agent/test_agent_plugins.py @@ -1,7 +1,10 @@ import json import shutil import subprocess +import time +from concurrent.futures import ThreadPoolExecutor from pathlib import Path +from threading import Barrier from typing import Any, cast import pytest @@ -18,6 +21,15 @@ from nanobot.agent.agent_plugins import ( from nanobot.agent.skills import SkillsLoader +@pytest.fixture(autouse=True) +def _isolate_plugin_state(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + agent_plugins, + "get_config_path", + lambda: tmp_path / "config" / "config.json", + ) + + def _write_skill(root: Path, name: str, *, description: str = "Plugin skill.") -> Path: skill = root / "skills" / name skill.mkdir(parents=True) @@ -48,6 +60,7 @@ def _write_plugin( def test_skills_loader_discovers_agent_plugin_skill(tmp_path: Path) -> None: plugin = _write_plugin(tmp_path, "acme-tools") _write_skill(plugin, "release-notes", description="Draft release notes from changes.") + set_agent_plugin_enabled(tmp_path, "acme-tools", True) loader = SkillsLoader(tmp_path, builtin_skills_dir=tmp_path / "builtin") @@ -71,6 +84,7 @@ def test_skills_loader_sees_plugin_installed_after_startup(tmp_path: Path) -> No plugin = _write_plugin(tmp_path, "acme-tools") _write_skill(plugin, "release-notes") + set_agent_plugin_enabled(tmp_path, "acme-tools", True) assert [entry["name"] for entry in loader.list_skills()] == ["release-notes"] @@ -152,6 +166,7 @@ def test_invalid_agent_skill_is_skipped( def test_workspace_skill_overrides_plugin_skill(tmp_path: Path) -> None: plugin = _write_plugin(tmp_path, "demo") _write_skill(plugin, "shared", description="Plugin version.") + set_agent_plugin_enabled(tmp_path, "demo", True) workspace_skill = tmp_path / "skills" / "shared" workspace_skill.mkdir(parents=True) (workspace_skill / "SKILL.md").write_text( @@ -165,6 +180,36 @@ def test_workspace_skill_overrides_plugin_skill(tmp_path: Path) -> None: assert "Workspace version" in (loader.load_skill("shared") or "") +def test_disabled_plugin_skill_cannot_shadow_or_inject_builtin_skill(tmp_path: Path) -> None: + plugin = _write_plugin(tmp_path, "demo") + skill = _write_skill(plugin, "shared", description="Plugin version.") + (skill / "SKILL.md").write_text( + "---\nname: shared\ndescription: Plugin version.\nalways: true\n---\n\nPlugin body.\n", + encoding="utf-8", + ) + builtin = tmp_path / "builtin" + builtin_skill = builtin / "shared" + builtin_skill.mkdir(parents=True) + (builtin_skill / "SKILL.md").write_text( + "---\nname: shared\ndescription: Built-in version.\n---\n\nBuilt-in body.\n", + encoding="utf-8", + ) + loader = SkillsLoader(tmp_path, builtin_skills_dir=builtin) + + assert [entry["source"] for entry in loader.list_skills()] == ["builtin"] + assert "Built-in version" in (loader.load_skill("shared") or "") + assert loader.get_always_skills() == [] + + set_agent_plugin_enabled(tmp_path, "demo", True) + assert [entry["source"] for entry in loader.list_skills()] == ["plugin"] + assert "Plugin body" in (loader.load_skill("shared") or "") + assert loader.get_always_skills() == ["shared"] + + set_agent_plugin_enabled(tmp_path, "demo", False) + assert [entry["source"] for entry in loader.list_skills()] == ["builtin"] + assert "Built-in version" in (loader.load_skill("shared") or "") + + def test_plugin_skill_symlink_cannot_escape_plugin_root(tmp_path: Path) -> None: plugin = _write_plugin(tmp_path, "demo") outside = tmp_path / "outside" @@ -182,12 +227,7 @@ def test_plugin_skill_symlink_cannot_escape_plugin_root(tmp_path: Path) -> None: assert discover_agent_plugin_skills(tmp_path) == [] -def test_plugin_mcp_requires_explicit_enable(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr( - agent_plugins, - "get_config_path", - lambda: tmp_path / "config" / "config.json", - ) +def test_plugin_mcp_requires_explicit_enable(tmp_path: Path) -> None: plugin = _write_plugin(tmp_path, "desktop") executable = plugin / "bin" / "server" executable.parent.mkdir() @@ -228,11 +268,6 @@ def test_plugin_setup_command_runs_once_per_version( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: - monkeypatch.setattr( - agent_plugins, - "get_config_path", - lambda: tmp_path / "config" / "config.json", - ) monkeypatch.setenv("NANOBOT_TEST_SECRET", "do-not-inherit") plugin = _write_plugin( tmp_path, @@ -266,15 +301,46 @@ def test_plugin_setup_command_runs_once_per_version( assert agent_plugins_payload(tmp_path)["plugins"][0]["setup_required"] is False -def test_invalid_plugin_mcp_entries_do_not_block_valid_servers( +def test_concurrent_plugin_enable_runs_setup_once( tmp_path: Path, monkeypatch: pytest.MonkeyPatch, ) -> None: - monkeypatch.setattr( - agent_plugins, - "get_config_path", - lambda: tmp_path / "config" / "config.json", + plugin = _write_plugin( + tmp_path, + "desktop", + manifest={ + "$schema": AGENT_PLUGIN_SCHEMA, + "name": "desktop", + "version": "1.2.3", + "extensions": {"dev.nanobot": {"installCommand": ["./bin/install"]}}, + }, ) + executable = plugin / "bin" / "install" + executable.parent.mkdir() + executable.write_text("setup", encoding="utf-8") + calls: list[tuple[str, ...]] = [] + + def run(command: tuple[str, ...], **_: Any) -> subprocess.CompletedProcess[str]: + calls.append(command) + time.sleep(0.1) + return subprocess.CompletedProcess(command, 0, "ok", "") + + monkeypatch.setattr(agent_plugins.subprocess, "run", run) + ready = Barrier(2) + + def enable() -> None: + ready.wait() + set_agent_plugin_enabled(tmp_path, "desktop", True) + + with ThreadPoolExecutor(max_workers=2) as executor: + futures = [executor.submit(enable) for _ in range(2)] + for future in futures: + future.result() + + assert calls == [(str(executable),)] + + +def test_invalid_plugin_mcp_entries_do_not_block_valid_servers(tmp_path: Path) -> None: plugin = _write_plugin(tmp_path, "network") executable = plugin / "bin" / "server" executable.parent.mkdir() diff --git a/tests/cli_apps/test_service.py b/tests/cli_apps/test_service.py index dc9c9afbf..3377b71eb 100644 --- a/tests/cli_apps/test_service.py +++ b/tests/cli_apps/test_service.py @@ -9,10 +9,21 @@ from types import SimpleNamespace import pytest +from nanobot.agent import agent_plugins from nanobot.agent.agent_plugins import discover_agent_plugin_skills +from nanobot.agent.skills import SkillsLoader from nanobot.apps.cli.service import CliAppError, CliAppManager, CliAppsRuntimeConfig +@pytest.fixture(autouse=True) +def _isolate_plugin_state(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + agent_plugins, + "get_config_path", + lambda: tmp_path / "config" / "config.json", + ) + + def _write_cache(path: Path, registry: dict) -> None: path.parent.mkdir(parents=True, exist_ok=True) path.write_text( @@ -418,6 +429,11 @@ def test_install_dispatches_safe_pip_and_installs_skill( assert [item.name for item in discover_agent_plugin_skills(manager.workspace)] == [ "cli-app-gimp" ] + assert [ + item["name"] + for item in SkillsLoader(manager.workspace).list_skills() + if item["source"] == "plugin" + ] == ["cli-app-gimp"] assert not legacy.exists()