diff --git a/nanobot/channels/websocket/tests/test_websocket_http_routes.py b/nanobot/channels/websocket/tests/test_websocket_http_routes.py index 3c058e7de..d69b60cc5 100644 --- a/nanobot/channels/websocket/tests/test_websocket_http_routes.py +++ b/nanobot/channels/websocket/tests/test_websocket_http_routes.py @@ -765,6 +765,151 @@ async def test_webui_skills_marketplace_routes_search_and_install( await server_task +@pytest.mark.asyncio +async def test_webui_skill_install_rejects_overlapping_requests( + bus: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + started = asyncio.Event() + finish = asyncio.Event() + + async def install( + source: str, + skill_id: str, + workspace: Path, + *, + provider: str, + version: str, + ) -> dict[str, Any]: + started.set() + await finish.wait() + skill_dir = workspace / "skills" / skill_id + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: react-testing\ndescription: Test React apps.\n---\n", + encoding="utf-8", + ) + return {"installed": True, "already_installed": False, "name": skill_id} + + install_mock = AsyncMock(side_effect=install) + monkeypatch.setattr("nanobot.webui.ws_http.install_marketplace_skill", install_mock) + channel = _ch( + bus, + session_manager=_seed_session(tmp_path), + workspace_path=tmp_path, + port=_free_port(), + ) + token = channel.gateway.tokens.issue_api_token(300) + path = ( + "/api/webui/skills/install" + "?source=acme%2Fagent-skills&skill=react-testing" + ) + request = _FakeReq( + { + "Authorization": f"Bearer {token}", + "Host": "127.0.0.1:8765", + }, + path=path, + ) + + first = asyncio.create_task(channel.gateway.http.dispatch(_LOCAL, request)) + await started.wait() + overlapping = await channel.gateway.http.dispatch(_LOCAL, request) + + assert overlapping.status_code == 409 + assert "already in progress" in overlapping.body.decode() + assert install_mock.await_count == 1 + + finish.set() + completed = await first + assert completed.status_code == 200 + assert install_mock.await_count == 1 + + +@pytest.mark.asyncio +async def test_webui_skill_delete_remains_local_only( + bus: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + delete = MagicMock() + policy = MagicMock() + policy.tools.webui_allow_remote_package_install = True + monkeypatch.setattr("nanobot.config.loader.load_config", lambda: policy) + monkeypatch.setattr("nanobot.webui.ws_http.delete_webui_skill", delete) + channel = _ch( + bus, + session_manager=_seed_session(tmp_path), + workspace_path=tmp_path, + port=_free_port(), + ) + token = channel.gateway.tokens.issue_api_token(300) + response = await channel.gateway.http.dispatch( + _REMOTE, + _FakeReq( + {"Authorization": f"Bearer {token}"}, + path="/api/webui/skills/delete?name=custom-skill", + ), + ) + + assert response.status_code == 403 + assert "remote skill deletion is disabled" in response.body.decode() + delete.assert_not_called() + + +@pytest.mark.asyncio +async def test_webui_skill_install_honors_remote_install_opt_in( + bus: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + policy = MagicMock() + policy.tools.webui_allow_remote_package_install = True + monkeypatch.setattr("nanobot.config.loader.load_config", lambda: policy) + + async def install( + source: str, + skill_id: str, + workspace: Path, + *, + provider: str, + version: str, + ) -> dict[str, Any]: + skill_dir = workspace / "skills" / skill_id + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: react-testing\ndescription: Test React apps.\n---\n", + encoding="utf-8", + ) + return {"installed": True, "already_installed": False, "name": skill_id} + + monkeypatch.setattr( + "nanobot.webui.ws_http.install_marketplace_skill", + AsyncMock(side_effect=install), + ) + channel = _ch( + bus, + session_manager=_seed_session(tmp_path), + workspace_path=tmp_path, + port=_free_port(), + ) + token = channel.gateway.tokens.issue_api_token(300) + response = await channel.gateway.http.dispatch( + _REMOTE, + _FakeReq( + {"Authorization": f"Bearer {token}"}, + path=( + "/api/webui/skills/install" + "?source=acme%2Fagent-skills&skill=react-testing" + ), + ), + ) + + assert response.status_code == 200 + assert json.loads(response.body.decode())["last_action"]["name"] == "react-testing" + + @pytest.mark.asyncio async def test_cli_apps_routes_require_token_and_return_payload( bus: MagicMock, diff --git a/nanobot/config/schema.py b/nanobot/config/schema.py index deb312bcb..e0a388b53 100644 --- a/nanobot/config/schema.py +++ b/nanobot/config/schema.py @@ -403,7 +403,7 @@ class ToolsConfig(Base): "webuiAllowRemotePackageInstall", "webui_allow_remote_package_install", ), - ) # allow non-local WebUI clients to install optional Python packages + ) # allow non-local WebUI clients to install optional packages and agent skills mcp_servers: dict[str, MCPServerConfig] = Field(default_factory=dict) ssrf_whitelist: list[str] = Field(default_factory=list) # CIDR ranges to exempt from SSRF blocking (e.g. ["100.64.0.0/10"] for Tailscale) diff --git a/nanobot/webui/skills_api.py b/nanobot/webui/skills_api.py index 68de970be..e082a7f6b 100644 --- a/nanobot/webui/skills_api.py +++ b/nanobot/webui/skills_api.py @@ -4,7 +4,7 @@ from __future__ import annotations import json import shlex -import shutil +import tempfile from pathlib import Path from typing import Any @@ -113,19 +113,25 @@ def delete_webui_skill( target = skills_root / name if target.parent != skills_root: raise SkillManagementError("invalid skill name") - if target.is_symlink(): - target.unlink() - elif target.is_dir(): - shutil.rmtree(target) - else: + if not target.is_symlink() and not target.is_dir(): raise SkillManagementError("skill directory was not found", status=404) config = load_config() - next_disabled = set(config.agents.defaults.disabled_skills) + original_disabled = list(config.agents.defaults.disabled_skills) + next_disabled = set(original_disabled) if name in next_disabled: next_disabled.remove(name) - config.agents.defaults.disabled_skills = sorted(next_disabled) - save_config(config) + with tempfile.TemporaryDirectory(prefix=".nanobot-delete-", dir=skills_root) as staging: + staged_target = Path(staging) / name + target.replace(staged_target) + try: + if next_disabled != set(original_disabled): + config.agents.defaults.disabled_skills = sorted(next_disabled) + save_config(config) + except Exception: + config.agents.defaults.disabled_skills = original_disabled + staged_target.replace(target) + raise disabled_skills.clear() disabled_skills.update(next_disabled) return {"name": name, "enabled": False, "deleted": True} diff --git a/nanobot/webui/skills_marketplace.py b/nanobot/webui/skills_marketplace.py index 3dea4976d..75d722d67 100644 --- a/nanobot/webui/skills_marketplace.py +++ b/nanobot/webui/skills_marketplace.py @@ -530,7 +530,6 @@ async def _install_skillhub_skill( "name": skill_id, "provider": _PROVIDER_SKILLHUB, "version": version, - "verified": bool(signature.get("signed")), } diff --git a/nanobot/webui/ws_http.py b/nanobot/webui/ws_http.py index d5ba320c4..d5f5ea5db 100644 --- a/nanobot/webui/ws_http.py +++ b/nanobot/webui/ws_http.py @@ -199,6 +199,7 @@ class GatewayHTTPHandler: self.skills_workspace_path = skills_workspace_path self.disabled_skills = disabled_skills if disabled_skills is not None else set() self.skill_state_action = skill_state_action + self._skill_install_lock = asyncio.Lock() self.cron_service = cron_service self.local_trigger_store = local_trigger_store self.cron_pending_job_ids = cron_pending_job_ids @@ -912,25 +913,28 @@ class GatewayHTTPHandler: return _http_error(401, "Unauthorized") if not self._allow_webui_package_install(connection, request): return _http_error(403, "remote skill installation is disabled") + if self._skill_install_lock.locked(): + return _http_error(409, "another skill installation is already in progress") query = _parse_query(request.path) provider = _query_first(query, "provider") or "skills_sh" source = _query_first(query, "source") or "" skill_id = _query_first(query, "skill") or "" version = _query_first(query, "version") or "" - try: - action = await install_marketplace_skill( - source, - skill_id, - self.skills_workspace_path, - provider=provider, - version=version, - ) - except SkillsMarketplaceError as exc: - return _http_error(exc.status, exc.message) - except Exception: - self._log.exception("skill installation failed") - return _http_error(500, "skill installation failed") + async with self._skill_install_lock: + try: + action = await install_marketplace_skill( + source, + skill_id, + self.skills_workspace_path, + provider=provider, + version=version, + ) + except SkillsMarketplaceError as exc: + return _http_error(exc.status, exc.message) + except Exception: + self._log.exception("skill installation failed") + return _http_error(500, "skill installation failed") return _http_json_response({ **webui_skills_payload( self.skills_workspace_path, @@ -983,7 +987,7 @@ class GatewayHTTPHandler: ) -> Response: if not self.check_api_token(request): return _http_error(401, "Unauthorized") - if not self._allow_webui_package_install(connection, request): + if not _is_local_browser_request(connection, request.headers): return _http_error(403, "remote skill deletion is disabled") name = _query_first(_parse_query(request.path), "name") or "" try: diff --git a/tests/webui/test_skills_api.py b/tests/webui/test_skills_api.py index f69c685f0..1601dec1c 100644 --- a/tests/webui/test_skills_api.py +++ b/tests/webui/test_skills_api.py @@ -150,3 +150,30 @@ def test_delete_webui_skill_rejects_symlinked_skills_root( assert exc_info.value.status == 403 assert (outside / "custom-skill" / "SKILL.md").is_file() + + +def test_delete_webui_skill_restores_directory_when_config_save_fails( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + directory = _write_skill(tmp_path, "custom-skill") + config = _config("custom-skill") + monkeypatch.setattr("nanobot.webui.skills_api.load_config", lambda: config) + + def fail_save(_config: object) -> None: + raise OSError("disk full") + + monkeypatch.setattr("nanobot.webui.skills_api.save_config", fail_save) + disabled = {"custom-skill"} + + with pytest.raises(OSError, match="disk full"): + delete_webui_skill( + tmp_path, + "custom-skill", + disabled_skills=disabled, + ) + + assert directory.is_dir() + assert (directory / "SKILL.md").is_file() + assert config.agents.defaults.disabled_skills == ["custom-skill"] + assert disabled == {"custom-skill"} diff --git a/tests/webui/test_skills_marketplace.py b/tests/webui/test_skills_marketplace.py index 993ffbc0d..edee85a99 100644 --- a/tests/webui/test_skills_marketplace.py +++ b/tests/webui/test_skills_marketplace.py @@ -477,7 +477,6 @@ async def test_install_skillhub_skill_checks_fingerprint_and_extracts_safely( "name": "ima-skills", "provider": "skillhub", "version": "1.1.8", - "verified": True, } assert (tmp_path / "skills" / "ima-skills" / "SKILL.md").read_bytes() == skill_content diff --git a/webui/src/components/settings/SkillsCatalogSettings.tsx b/webui/src/components/settings/SkillsCatalogSettings.tsx index 022b5e76b..99b1a23bb 100644 --- a/webui/src/components/settings/SkillsCatalogSettings.tsx +++ b/webui/src/components/settings/SkillsCatalogSettings.tsx @@ -41,6 +41,7 @@ export function SkillsCatalogSettings({ skills }: { skills: SkillSummary[] }) { ).length; const [selectedSkill, setSelectedSkill] = useState(null); const [view, setView] = useState<"installed" | "discover">("installed"); + const [installingSkill, setInstallingSkill] = useState(""); const [installedQuery, setInstalledQuery] = useState(""); const [installedFilter, setInstalledFilter] = useState<"all" | "enabled" | "disabled">( "all", @@ -209,7 +210,11 @@ export function SkillsCatalogSettings({ skills }: { skills: SkillSummary[] }) { )} ) : ( - + )} void; +}) { const { token } = useClient(); const { t } = useTranslation(); const [query, setQuery] = useState(""); @@ -48,7 +56,6 @@ export function SkillsMarketplace({ installedSkills }: { installedSkills: SkillS const [error, setError] = useState(""); const [provider, setProvider] = useState("all"); const [selected, setSelected] = useState(null); - const [installing, setInstalling] = useState(""); const installedNames = useMemo( () => new Set(installedSkills.map((skill) => skill.name)), [installedSkills], @@ -149,7 +156,7 @@ export function SkillsMarketplace({ installedSkills }: { installedSkills: SkillS const install = async (skill: MarketplaceSkillSummary) => { setSelected(null); - setInstalling(skill.id); + onInstallingChange(skill.id); setError(""); try { const payload = await installMarketplaceSkill( @@ -179,7 +186,7 @@ export function SkillsMarketplace({ installedSkills }: { installedSkills: SkillS }), ); } finally { - setInstalling(""); + onInstallingChange(""); } }; @@ -396,6 +403,7 @@ function MarketplaceSkillGroups({ grouped: boolean; onSelect: (skill: MarketplaceSkillSummary) => void; }) { + const { t } = useTranslation(); const providers: Array> = [ "skills_sh", "skillhub", @@ -425,7 +433,10 @@ function MarketplaceSkillGroups({ target="_blank" rel="noreferrer" className="text-muted-foreground transition-colors hover:text-foreground" - aria-label={`Open ${providerLabel(provider)}`} + aria-label={t("settings.skills.marketplaceOpenProvider", { + provider: providerLabel(provider), + defaultValue: "Open {{provider}}", + })} > @@ -613,6 +624,9 @@ function providerUrl(provider: Exclude): string { function TrendSparkline({ values }: { values?: number[] }) { const { t } = useTranslation(); + const trendLabel = t("settings.skills.marketplaceTrendLabel", { + defaultValue: "8-week install trend", + }); if (values === undefined) { return ; @@ -647,9 +661,9 @@ function TrendSparkline({ values }: { values?: number[] }) { viewBox={`0 0 ${width} ${height}`} className="hidden h-[30px] w-24 shrink-0 overflow-visible text-foreground/40 sm:block" role="img" - aria-label="8-week install trend" + aria-label={trendLabel} > - 8-week install trend + {trendLabel} ): void { vi.stubGlobal( "fetch", vi.fn(async (input: RequestInfo | URL) => { - const body = routes[String(input)]; + const route = routes[String(input)]; + const body = + typeof route === "function" + ? await (route as () => unknown | Promise)() + : route; return body === undefined ? ({ ok: false, status: 404, json: async () => ({}) } as Response) : jsonResponse(body); @@ -597,6 +601,26 @@ describe("App layout", () => { }); it("discovers and installs a skill from skills.sh", async () => { + let finishInstall!: (value: unknown) => void; + const pendingInstall = new Promise((resolve) => { + finishInstall = resolve; + }); + const installedPayload = { + skills: [ + { + name: "react-testing", + description: "Test React apps.", + source: "workspace", + available: true, + }, + { name: "cron", description: "Schedule reminders.", source: "builtin", available: true }, + ], + last_action: { + installed: true, + already_installed: false, + name: "react-testing", + }, + }; mockFetchRoutes({ "/api/settings": baseSettingsPayload(), "/api/settings/cli-apps": { apps: [], installed_count: 0, catalog_updated_at: "2026-04-18" }, @@ -683,22 +707,8 @@ describe("App layout", () => { "/api/webui/skills/trends?id=acme%2Fagent-skills%2Freact-testing": { trends: { "acme/agent-skills/react-testing": [] }, }, - "/api/webui/skills/install?provider=skills_sh&source=acme%2Fagent-skills&skill=react-testing": { - skills: [ - { - name: "react-testing", - description: "Test React apps.", - source: "workspace", - available: true, - }, - { name: "cron", description: "Schedule reminders.", source: "builtin", available: true }, - ], - last_action: { - installed: true, - already_installed: false, - name: "react-testing", - }, - }, + "/api/webui/skills/install?provider=skills_sh&source=acme%2Fagent-skills&skill=react-testing": + () => pendingInstall, }); render(); @@ -751,10 +761,20 @@ describe("App layout", () => { }), ); }); + fireEvent.click(screen.getByRole("tab", { name: "Installed" })); + fireEvent.click(screen.getByRole("tab", { name: "Discover" })); expect( - await screen.findByRole("button", { name: "Installed React Testing" }), + await screen.findByRole("button", { name: "Install find-skills" }), ).toBeDisabled(); + await act(async () => { + finishInstall(installedPayload); + await pendingInstall; + }); + + await waitFor(() => { + expect(screen.getByRole("button", { name: "Install find-skills" })).toBeEnabled(); + }); fireEvent.click(screen.getByRole("tab", { name: "Installed" })); expect(screen.getByText("react-testing")).toBeInTheDocument(); }); @@ -1996,9 +2016,8 @@ describe("App layout", () => { expect(screen.getByTestId("provider-logo-openai")).toBeInTheDocument(); expect(screen.queryByText(/Product names, logos, and brands/)).not.toBeInTheDocument(); expect(screen.queryByText("Not configured")).not.toBeInTheDocument(); - const clickProviderRow = (label: string) => { - const providerLabel = screen - .getAllByText(label) + const clickProviderRow = async (label: string) => { + const providerLabel = (await screen.findAllByText(label)) .find((element) => element.className.includes("font-semibold")); expect(providerLabel).toBeTruthy(); fireEvent.click(providerLabel!); @@ -2009,21 +2028,21 @@ describe("App layout", () => { ); fireEvent.click(await screen.findByRole("menuitem", { name: label })); }; - clickProviderRow("OpenAI"); + await clickProviderRow("OpenAI"); fireEvent.click(screen.getByRole("button", { name: "Edit" })); fireEvent.change(screen.getByPlaceholderText("Leave blank to keep the current key"), { target: { value: "unsaved-openai-key" }, }); - clickProviderRow("OpenAI"); + await clickProviderRow("OpenAI"); await chooseProvider("OpenRouter"); - clickProviderRow("OpenRouter"); - clickProviderRow("OpenAI"); + await clickProviderRow("OpenRouter"); + await clickProviderRow("OpenAI"); expect(screen.getByText("open••••-key")).toBeInTheDocument(); expect(screen.queryByDisplayValue("unsaved-openai-key")).not.toBeInTheDocument(); - clickProviderRow("OpenAI"); + await clickProviderRow("OpenAI"); await chooseProvider("Ant Ling"); expect(screen.getByDisplayValue("https://api.ant-ling.com/v1")).toBeInTheDocument(); - clickProviderRow("Ant Ling"); + await clickProviderRow("Ant Ling"); await chooseProvider("Atomic Chat"); expect(screen.getByDisplayValue("http://localhost:1337/v1")).toBeInTheDocument(); expect(screen.getByRole("button", { name: "Save provider" })).toBeEnabled(); diff --git a/webui/src/tests/i18n.test.tsx b/webui/src/tests/i18n.test.tsx index e5f3e9d4b..1b4328915 100644 --- a/webui/src/tests/i18n.test.tsx +++ b/webui/src/tests/i18n.test.tsx @@ -124,6 +124,7 @@ const LOCALIZED_SETTINGS_COPY_KEYS = [ "settings.skills.marketplaceConfirmDescription", "settings.skills.marketplaceConfirmInstall", "settings.skills.marketplaceOpen", + "settings.skills.marketplaceOpenProvider", "settings.skills.marketplaceInstalls24h", "settings.skills.marketplaceInstalls", "settings.skills.marketplaceNpxRequired", @@ -131,6 +132,7 @@ const LOCALIZED_SETTINGS_COPY_KEYS = [ "settings.skills.marketplaceInstalled", "settings.skills.marketplaceInstall", "settings.skills.marketplaceNoTrend", + "settings.skills.marketplaceTrendLabel", "settings.nanobotFeatures.disable", "settings.nanobotFeatures.ready", "settings.nanobotFeatures.missingDependency",