From c9b6131f8fc4beb186632556ea3d589488edc90f Mon Sep 17 00:00:00 2001 From: Huixin615 Date: Fri, 17 Jul 2026 23:22:16 +0800 Subject: [PATCH] fix(skills): reload mounted skills without restarting Gateway (#4264) * fix(skills): add admin-only reload endpoint * fix(skills): preserve cache when reload fails --- README.md | 2 + backend/AGENTS.md | 3 +- backend/app/gateway/routers/skills.py | 33 ++- backend/docs/API.md | 45 +++ .../deerflow/agents/lead_agent/prompt.py | 66 +++-- .../tests/blocking_io/test_skills_reload.py | 39 +++ backend/tests/test_lead_agent_prompt.py | 1 + backend/tests/test_skills_reload.py | 267 ++++++++++++++++++ backend/tests/test_skills_router_authz.py | 1 + 9 files changed, 437 insertions(+), 20 deletions(-) create mode 100644 backend/tests/blocking_io/test_skills_reload.py create mode 100644 backend/tests/test_skills_reload.py diff --git a/README.md b/README.md index 0040252db..8c47ed937 100644 --- a/README.md +++ b/README.md @@ -652,6 +652,8 @@ An enabled skill's `allowed-tools` policy applies only after that skill is expli When you install `.skill` archives through the Gateway, DeerFlow accepts standard optional frontmatter metadata such as `version`, `author`, and `compatibility` instead of rejecting otherwise valid external skills. +If a trusted operator manages the configured skills directory through an external mount such as MinIO, NFS, or CSI, an administrator can call `POST /api/skills/reload` after changing files. This invalidates skill prompt caches for the current Gateway process and waits up to the bounded refresh timeout so subsequent runs rescan the latest files; running tasks are unchanged. A loader-level filesystem failure returns a generic server error and preserves the last successfully loaded process cache rather than publishing an empty catalog. Uvicorn workers and Kubernetes Pods must each be targeted separately. Direct mount writes bypass the validation, SkillScan, and history applied by DeerFlow's install/edit APIs, so only operator-controlled systems should have write access. + Skill installs and agent-managed skill edits run through **SkillScan**, a native deterministic safety scanner before the LLM-based skill scanner. Phase 1 runs offline with no Semgrep/OpenGrep dependency, blocks high-confidence `CRITICAL` findings such as private keys or shell execution, and passes warning findings to the LLM scanner for contextual review. Set `skill_scan.enabled: false` in `config.yaml` to disable only the deterministic analyzers; safe archive extraction and the LLM scanner still run. DeerFlow also ships with **skill-reviewer**, a public skill for read-only skill quality review. It uses the built-in `review_skill_package` tool to inspect installed skills, local packages, archives, or pasted `SKILL.md` content without activating the target skill, binding its secrets, executing its scripts, or installing it. The tool returns a compact, tag-neutralized JSON payload to the model context and keeps the full raw review payload in the tool artifact for programmatic consumers. The deterministic review core reuses DeerFlow parsing and SkillScan facts, emits versioned JSON contracts under `contracts/skill_review/`, and can be run from the backend CLI: diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 9a22d09c7..c83c652f3 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -322,7 +322,7 @@ CORS is same-origin by default when requests enter through nginx on port 2026. S | **Features** (`/api/features`) | `GET /` - report config-gated feature availability (currently `agents_api.enabled`) for frontend UI gating | | **Console** (`/api/console`) | Read-only cross-thread observability for the current user (the data layer for an operations dashboard or external monitoring): `GET /stats` - headline counters (runs/threads/agents/tokens/cost); `GET /runs` - paginated run history joined with thread titles (per-run cost); `GET /usage` - zero-filled daily token series + per-model breakdown with spend. Queries `runs`/`threads_meta` directly as a reporting layer (no new `RunStore` methods); requires a SQL database backend — returns 503 on `database.backend: memory`. Real-cost estimation reads optional `models[*].pricing` (`currency`, `input_per_million`, `output_per_million`, `input_cache_hit_per_million`; `ModelConfig` is `extra="allow"`, so no schema change) and prices each run from its `token_usage_by_model` input/output split. Pricing is **cache-aware**: `RunJournal` accumulates prompt-cache hits from `usage_metadata.input_token_details.cache_read` into a sparse `cache_read_tokens` bucket key (also threaded through `SubagentTokenCollector` → `record_external_llm_usage_records`), and cache-hit input tokens are billed at `input_cache_hit_per_million` (omitted → billed at the miss price, a conservative upper bound). Legacy rows fall back to run-level totals at `model_name`; unpriced models yield `cost: null` and cost fields are null when no pricing is configured | | **MCP** (`/api/mcp`) | `GET /config` - get config; `PUT /config` - update config (saves to extensions_config.json) | -| **Skills** (`/api/skills`) | `GET /` - list skills; `GET /{name}` - details; `PUT /{name}` - update enabled; `POST /install` - install from .skill archive (accepts standard optional frontmatter like `version`, `author`, `compatibility`) | +| **Skills** (`/api/skills`) | `GET /` - list skills; `GET /{name}` - details; `PUT /{name}` - update enabled; `POST /install` - install from .skill archive (accepts standard optional frontmatter like `version`, `author`, `compatibility`); `POST /reload` - admin-only process-local prompt-cache invalidation after trusted external filesystem changes | | **Memory** (`/api/memory`) | `GET /` - memory data; `POST /reload` - force reload; `GET /config` - config; `GET /status` - config + data | | **Uploads** (`/api/threads/{id}/uploads`) | `POST /` - upload files (auto-converts PDF/PPT/Excel/Word); `GET /list` - list; `DELETE /{filename}` - delete | | **Threads** (`/api/threads/{id}`) | `DELETE /` - remove DeerFlow-managed local thread data after LangGraph thread deletion; `POST /branches` - create a new main-thread branch from a completed assistant turn checkpoint. Workspace files are not checkpointed, so the branch only best-effort copies the current workspace when branching from the **latest** turn (`workspace_clone_mode="current_thread_best_effort"`); branching from an older/historical turn skips the copy (`workspace_clone_mode="skipped_historical_turn"`) so the branch never inherits files that only exist in a later timeline; `GET /goal`, `PUT /goal`, `DELETE /goal` - read, set, and clear the active thread goal; `POST /compact` - manually summarize older active context into `summary_text` and retain the recent message window, blocked while a run is in flight; unexpected failures are logged server-side and return a generic 500 detail | @@ -455,6 +455,7 @@ Additional providers also live here (`boxlite`, `brave`, `browserless`, `crawl4a - **Location**: `deer-flow/skills/{public,custom}/` - **Format**: Directory with `SKILL.md` (YAML frontmatter: name, description, license, allowed-tools, required-secrets) - **Loading**: `load_skills()` recursively scans namespace directories under `skills/{public,custom}`, but stops descending once it finds a `SKILL.md`; that directory is a package boundary, so no nested `SKILL.md` is registered as a runtime skill. SkillScan has a deliberately narrower packaging rule: known eval fixtures are permitted as support data, while other nested `SKILL.md` files are reported as package defects. It parses runtime metadata and reads enabled state from extensions_config.json. +- **External reload**: `POST /api/skills/reload` is an admin-only, process-local invalidation hook for trusted MinIO/NFS/CSI writes. `SkillStorage` instances do not cache a catalog — `load_skills()` scans on every call — so the route clears all `(app_config, user_id)` entries and the rendered prompt-section LRU, then waits up to the shared refresh timeout for the existing off-loop single-flight refresh. Each invalidation receives a generation-bound result handle; a successful scan atomically replaces the global enabled-skills cache, while a loader-level failure propagates to the HTTP waiter and preserves the last-known-good global cache. Per-user/config scans capture the refresh version and cannot repopulate shared caches if invalidation occurs while they are loading. A timed-out HTTP wait fails generically while the daemon refresh worker continues. Subsequent runs rescan after a successful reload; active runs keep their existing snapshot. Each Uvicorn worker/Kubernetes Pod must be targeted separately. Direct mount writes bypass install/edit validation, SkillScan, and history, so mounted roots are an operator-controlled trust boundary. - **Tool policy**: Lead-agent `allowed-tools` declarations apply dynamically only to slash-activated skills and skills captured in `ThreadState.skill_context` through configured `read_file` loads; passive enabled skills and custom-agent skill allowlists remain discoverable without clamping the global toolset. Slash policy is dominant for its run, preventing subsequently read skills from widening explicit authority; autonomous captured skills use the existing union only when no slash source exists. `tool_search` and `describe_skill` stay available as framework discovery infrastructure, while every discovered or promoted business tool still requires active-policy permission for schema visibility and execution; `task` likewise requires an explicit declaration. Each active model call intentionally reloads the full live registry so enable/disable changes, frontmatter edits, and custom/public name-shadow winners take effect without a stale TTL or unsafe direct-path cache; all tool calls produced by that model step reuse the resulting source-and-path-signed decision. Registry failures and all-invalid active sets fail closed, while stale individual paths are skipped when another valid skill remains. This is best-effort behavioral scoping, not a hard security boundary: alternate loading paths are not captured and bounded autonomous context may evict entries. Subagents still filter statically because their configured skills are all loaded into the session at startup. - **Injection (legacy / default)**: Enabled skills are listed in the agent system prompt with full metadata and container paths (`` block). Controlled by `skills.deferred_discovery: false` (default). - **Deferred discovery** (`skills.deferred_discovery: true`): Skills are listed by name only in a compact `` block, keeping the system prompt prefix-cache friendly. The agent calls the `describe_skill` tool at runtime to fetch full metadata for skills it wants to use, then loads the SKILL.md via `read_file`. Two new modules support this path: diff --git a/backend/app/gateway/routers/skills.py b/backend/app/gateway/routers/skills.py index 25379e06e..caee64563 100644 --- a/backend/app/gateway/routers/skills.py +++ b/backend/app/gateway/routers/skills.py @@ -3,13 +3,14 @@ import json import logging import tempfile from pathlib import Path +from typing import Literal from fastapi import APIRouter, Depends, HTTPException, Request from pydantic import BaseModel, Field from app.gateway.deps import get_config, require_admin_user from app.gateway.path_utils import resolve_thread_virtual_path -from deerflow.agents.lead_agent.prompt import clear_skills_system_prompt_cache, refresh_user_skills_system_prompt_cache_async +from deerflow.agents.lead_agent.prompt import clear_skills_system_prompt_cache, refresh_skills_system_prompt_cache_async, refresh_user_skills_system_prompt_cache_async from deerflow.config.app_config import AppConfig from deerflow.config.extensions_config import ExtensionsConfig, SkillStateConfig, get_extensions_config, reload_extensions_config from deerflow.runtime.user_context import get_effective_user_id @@ -70,6 +71,14 @@ class SkillInstallResponse(BaseModel): message: str = Field(..., description="Installation result message") +class SkillReloadResponse(BaseModel): + """Response model for process-local skill cache invalidation.""" + + success: bool = Field(..., description="Whether the skill caches were invalidated") + scope: Literal["process"] = Field(..., description="Reload scope; only the current Gateway process is affected") + message: str = Field(..., description="Human-readable reload status") + + class CustomSkillContentResponse(SkillResponse): content: str = Field(..., description="Raw SKILL.md content") @@ -184,6 +193,28 @@ async def install_skill(request: Request, body: SkillInstallRequest, config: App raise HTTPException(status_code=500, detail=f"Failed to install skill: {str(e)}") +@router.post( + "/skills/reload", + response_model=SkillReloadResponse, + summary="Reload Skills", + description=("Invalidate skill prompt caches for all users in the current Gateway process. Subsequent runs rescan the configured skill directories; running tasks and other Gateway processes are unaffected."), +) +async def reload_skills(request: Request) -> SkillReloadResponse: + """Invalidate process-local skill prompt caches after external file changes.""" + await require_admin_user(request, detail=_ADMIN_REQUIRED_DETAIL) + try: + await refresh_skills_system_prompt_cache_async() + except Exception as exc: + logger.exception("Failed to invalidate skills cache") + raise HTTPException(status_code=500, detail="Failed to invalidate skills cache.") from exc + + return SkillReloadResponse( + success=True, + scope="process", + message="Skill caches invalidated; subsequent runs in this Gateway process will rescan the latest skills.", + ) + + @router.get("/skills/custom", response_model=SkillsListResponse, summary="List Custom Skills") async def list_custom_skills(config: AppConfig = Depends(get_config)) -> SkillsListResponse: """List only user-owned custom skills (SkillCategory.CUSTOM). diff --git a/backend/docs/API.md b/backend/docs/API.md index 8afa57d5b..7e9677b90 100644 --- a/backend/docs/API.md +++ b/backend/docs/API.md @@ -506,6 +506,51 @@ Content-Type: multipart/form-data } ``` +#### Reload Skills + +Invalidate the skill prompt caches for every user in the current Gateway +process. Subsequent runs rescan the configured public, custom, and legacy skill +directories; runs that have already started keep their existing skill snapshot. + +```http +POST /api/skills/reload +``` + +The request has no body and requires an authenticated administrator. For a +cookie-authenticated request, send the CSRF cookie value in the matching header: + +```bash +curl -X POST http://localhost:2026/api/skills/reload \ + -b cookies.txt \ + -H "X-CSRF-Token: " +``` + +**Response:** + +```json +{ + "success": true, + "scope": "process", + "message": "Skill caches invalidated; subsequent runs in this Gateway process will rescan the latest skills." +} +``` + +`success` confirms cache invalidation, not that every file on disk was valid: +malformed skills retain the existing parser behavior of being skipped and +logged. The endpoint returns `401` for unauthenticated callers, `403` for +non-admin users, and a generic `500` if the invalidation mechanism itself +fails or the process-local background scan does not finish within the cache +refresh timeout. A loader-level failure, such as an unavailable mounted root, +does not publish an empty catalog: the last successfully loaded process cache +remains available. A timed-out scan continues in its daemon worker and can +still populate the process cache when it finishes. + +The scope is deliberately process-local. Each Uvicorn worker or Kubernetes Pod +must be called directly; repeated requests through a load-balanced Service do +not guarantee that every instance is reached. External MinIO/NFS/CSI writes +bypass the validation, SkillScan, and history used by the install/edit APIs, so +the mounted directory must be writable only by trusted operators. + ### File Uploads #### Upload Files diff --git a/backend/packages/harness/deerflow/agents/lead_agent/prompt.py b/backend/packages/harness/deerflow/agents/lead_agent/prompt.py index a2b661cd5..157dab892 100644 --- a/backend/packages/harness/deerflow/agents/lead_agent/prompt.py +++ b/backend/packages/harness/deerflow/agents/lead_agent/prompt.py @@ -5,6 +5,7 @@ import html import logging import threading from collections import OrderedDict +from dataclasses import dataclass, field from functools import lru_cache from typing import TYPE_CHECKING @@ -44,6 +45,19 @@ _enabled_skills_refresh_version = 0 _enabled_skills_refresh_event = threading.Event() +@dataclass +class _EnabledSkillsRefreshHandle: + version: int + event: threading.Event = field(default_factory=threading.Event) + error: Exception | None = None + + def wait(self, timeout: float | None = None) -> bool: + return self.event.wait(timeout=timeout) + + +_enabled_skills_refresh_waiters: list[_EnabledSkillsRefreshHandle] = [] + + def _load_enabled_skills_sync() -> list[Skill]: return list(get_or_new_skill_storage().load_skills(enabled_only=True)) @@ -63,33 +77,41 @@ def _refresh_enabled_skills_cache_worker() -> None: with _enabled_skills_lock: target_version = _enabled_skills_refresh_version + refresh_error = None try: skills = _load_enabled_skills_sync() - except Exception: + except Exception as exc: logger.exception("Failed to load enabled skills for prompt injection") - skills = [] + skills = None + refresh_error = exc with _enabled_skills_lock: if _enabled_skills_refresh_version == target_version: - _enabled_skills_cache = skills + if refresh_error is None: + assert skills is not None + _enabled_skills_cache = skills _enabled_skills_refresh_active = False _enabled_skills_refresh_event.set() + completed_waiters = [waiter for waiter in _enabled_skills_refresh_waiters if waiter.version <= target_version] + _enabled_skills_refresh_waiters[:] = [waiter for waiter in _enabled_skills_refresh_waiters if waiter.version > target_version] + for waiter in completed_waiters: + waiter.error = refresh_error + waiter.event.set() return # A newer invalidation happened while loading. Keep the worker alive # and loop again so the cache always converges on the latest version. - _enabled_skills_cache = None def _ensure_enabled_skills_cache() -> threading.Event: global _enabled_skills_refresh_active with _enabled_skills_lock: + if _enabled_skills_refresh_active: + return _enabled_skills_refresh_event if _enabled_skills_cache is not None: _enabled_skills_refresh_event.set() return _enabled_skills_refresh_event - if _enabled_skills_refresh_active: - return _enabled_skills_refresh_event _enabled_skills_refresh_active = True _enabled_skills_refresh_event.clear() @@ -97,21 +119,22 @@ def _ensure_enabled_skills_cache() -> threading.Event: return _enabled_skills_refresh_event -def _invalidate_enabled_skills_cache() -> threading.Event: - global _enabled_skills_cache, _enabled_skills_refresh_active, _enabled_skills_refresh_version +def _invalidate_enabled_skills_cache() -> _EnabledSkillsRefreshHandle: + global _enabled_skills_refresh_active, _enabled_skills_refresh_version _get_cached_skills_prompt_section.cache_clear() with _enabled_skills_lock: - _enabled_skills_cache = None _enabled_skills_by_config_cache.clear() _enabled_skills_refresh_version += 1 + refresh_handle = _EnabledSkillsRefreshHandle(version=_enabled_skills_refresh_version) + _enabled_skills_refresh_waiters.append(refresh_handle) _enabled_skills_refresh_event.clear() if _enabled_skills_refresh_active: - return _enabled_skills_refresh_event + return refresh_handle _enabled_skills_refresh_active = True _start_enabled_skills_refresh_thread() - return _enabled_skills_refresh_event + return refresh_handle def prime_enabled_skills_cache() -> None: @@ -171,18 +194,20 @@ def get_enabled_skills_for_config(app_config: AppConfig | None = None, user_id: # next eviction cycle. _enabled_skills_by_config_cache.move_to_end(cache_key) return list(cached_skills) + load_version = _enabled_skills_refresh_version if user_id: skills = list(get_or_new_user_skill_storage(user_id, app_config=app_config).load_skills(enabled_only=True)) else: skills = list(get_or_new_skill_storage(app_config=app_config).load_skills(enabled_only=True)) with _enabled_skills_lock: - _enabled_skills_by_config_cache[cache_key] = (app_config, skills) - # Evict the least-recently-used entries when we exceed the cap. - # The cap is intentionally small (256) so a long-running process - # cannot leak one entry per distinct (config, user) pair seen. - while len(_enabled_skills_by_config_cache) > _ENABLED_SKILLS_BY_CONFIG_CACHE_MAXSIZE: - _enabled_skills_by_config_cache.popitem(last=False) + if _enabled_skills_refresh_version == load_version: + _enabled_skills_by_config_cache[cache_key] = (app_config, skills) + # Evict the least-recently-used entries when we exceed the cap. + # The cap is intentionally small (256) so a long-running process + # cannot leak one entry per distinct (config, user) pair seen. + while len(_enabled_skills_by_config_cache) > _ENABLED_SKILLS_BY_CONFIG_CACHE_MAXSIZE: + _enabled_skills_by_config_cache.popitem(last=False) return list(skills) @@ -210,7 +235,12 @@ def clear_skills_system_prompt_cache() -> None: async def refresh_skills_system_prompt_cache_async() -> None: - await asyncio.to_thread(_invalidate_enabled_skills_cache().wait) + refresh_handle = _invalidate_enabled_skills_cache() + refreshed = await asyncio.to_thread(refresh_handle.wait, _ENABLED_SKILLS_REFRESH_WAIT_TIMEOUT_SECONDS) + if not refreshed: + raise TimeoutError("Timed out waiting for enabled skills cache refresh") + if refresh_handle.error is not None: + raise RuntimeError("Enabled skills cache refresh failed") from refresh_handle.error def invalidate_user_skill_cache(user_id: str) -> None: diff --git a/backend/tests/blocking_io/test_skills_reload.py b/backend/tests/blocking_io/test_skills_reload.py new file mode 100644 index 000000000..b54a00d17 --- /dev/null +++ b/backend/tests/blocking_io/test_skills_reload.py @@ -0,0 +1,39 @@ +"""Regression anchor: the admin skills reload endpoint must not block ASGI.""" + +from __future__ import annotations + +import asyncio +from pathlib import Path + +import pytest + +from app.gateway.routers import skills as skills_router +from deerflow.agents.lead_agent import prompt as prompt_module +from deerflow.skills.storage.local_skill_storage import LocalSkillStorage + +pytestmark = pytest.mark.asyncio + + +def _seed_skill(skills_root: Path) -> None: + skill_dir = skills_root / "public" / "reload-anchor" + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: reload-anchor\ndescription: blocking IO regression anchor\n---\n# Reload anchor\n", + encoding="utf-8", + ) + + +async def test_reload_skills_offloads_directory_scan(tmp_path: Path, monkeypatch) -> None: + await asyncio.to_thread(_seed_skill, tmp_path) + storage = await asyncio.to_thread(LocalSkillStorage, host_path=str(tmp_path)) + + async def _noop_admin(_request, **_kwargs) -> None: + return None + + monkeypatch.setattr(skills_router, "require_admin_user", _noop_admin) + monkeypatch.setattr(prompt_module, "get_or_new_skill_storage", lambda **_kwargs: storage) + + response = await skills_router.reload_skills(request=None) + + assert response.success is True + assert response.scope == "process" diff --git a/backend/tests/test_lead_agent_prompt.py b/backend/tests/test_lead_agent_prompt.py index e6a0f3d94..dac928ecb 100644 --- a/backend/tests/test_lead_agent_prompt.py +++ b/backend/tests/test_lead_agent_prompt.py @@ -19,6 +19,7 @@ def _set_skills_cache_state(*, skills=None, active=False, version=0): prompt_module._enabled_skills_refresh_active = active prompt_module._enabled_skills_refresh_version = version prompt_module._enabled_skills_refresh_event.clear() + prompt_module._enabled_skills_refresh_waiters.clear() def test_build_self_update_section_empty_for_default_agent(): diff --git a/backend/tests/test_skills_reload.py b/backend/tests/test_skills_reload.py new file mode 100644 index 000000000..bd7ddbf61 --- /dev/null +++ b/backend/tests/test_skills_reload.py @@ -0,0 +1,267 @@ +from __future__ import annotations + +import threading +from pathlib import Path +from types import SimpleNamespace +from uuid import uuid4 + +import anyio +import pytest +from _router_auth_helpers import make_authed_test_app +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from app.gateway.auth.models import User +from app.gateway.deps import get_config +from app.gateway.routers import skills as skills_router +from deerflow.agents.lead_agent import prompt as prompt_module +from deerflow.skills.storage.local_skill_storage import LocalSkillStorage + +_SUCCESS_RESPONSE = { + "success": True, + "scope": "process", + "message": "Skill caches invalidated; subsequent runs in this Gateway process will rescan the latest skills.", +} + + +def _make_app(*, system_role: str) -> FastAPI: + config = SimpleNamespace( + skills=SimpleNamespace(get_skills_path=lambda: "/tmp/skills", container_path="/mnt/skills", use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage"), + skill_evolution=SimpleNamespace(enabled=True, moderation_model_name=None), + ) + app = make_authed_test_app( + user_factory=lambda: User( + email=f"{system_role}-reload-test@example.com", + password_hash="x", + system_role=system_role, + id=uuid4(), + ) + ) + app.state.config = config + app.dependency_overrides[get_config] = lambda: config + app.include_router(skills_router.router) + return app + + +def _write_skill(root: Path, name: str, description: str) -> None: + skill_dir = root / "public" / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: {description}\n---\n# {name}\n", + encoding="utf-8", + ) + + +def _reset_prompt_cache_state() -> None: + prompt_module._get_cached_skills_prompt_section.cache_clear() + with prompt_module._enabled_skills_lock: + prompt_module._enabled_skills_cache = None + prompt_module._enabled_skills_by_config_cache.clear() + prompt_module._enabled_skills_refresh_active = False + prompt_module._enabled_skills_refresh_version = 0 + prompt_module._enabled_skills_refresh_event.clear() + prompt_module._enabled_skills_refresh_waiters.clear() + + +def test_admin_can_reload_skills(monkeypatch) -> None: + calls = 0 + + async def _refresh() -> None: + nonlocal calls + calls += 1 + + monkeypatch.setattr(skills_router, "refresh_skills_system_prompt_cache_async", _refresh) + app = _make_app(system_role="admin") + + with TestClient(app) as client: + response = client.post("/api/skills/reload") + + assert response.status_code == 200 + assert response.json() == _SUCCESS_RESPONSE + assert calls == 1 + + +def test_reload_failure_returns_generic_error(monkeypatch) -> None: + async def _refresh() -> None: + raise RuntimeError("private mount failed at /srv/company/minio") + + monkeypatch.setattr(skills_router, "refresh_skills_system_prompt_cache_async", _refresh) + app = _make_app(system_role="admin") + + with TestClient(app) as client: + response = client.post("/api/skills/reload") + + assert response.status_code == 500 + assert response.json() == {"detail": "Failed to invalidate skills cache."} + assert "/srv/company/minio" not in response.text + + +def test_reload_worker_failure_returns_500_preserves_last_good_cache_and_can_retry(monkeypatch, tmp_path: Path) -> None: + _write_skill(tmp_path, "cached-skill", "last known good description") + storage = LocalSkillStorage(host_path=str(tmp_path)) + last_good_skills = storage.load_skills(enabled_only=True) + + _write_skill(tmp_path, "cached-skill", "recovered description") + recovered_skills = storage.load_skills(enabled_only=True) + load_count = 0 + + def _load_enabled_skills(): + nonlocal load_count + load_count += 1 + if load_count == 1: + raise PermissionError("mounted skills unavailable at /srv/company/minio") + return recovered_skills + + monkeypatch.setattr(prompt_module, "_load_enabled_skills_sync", _load_enabled_skills) + _reset_prompt_cache_state() + with prompt_module._enabled_skills_lock: + prompt_module._enabled_skills_cache = last_good_skills + + try: + app = _make_app(system_role="admin") + with TestClient(app) as client: + failed_response = client.post("/api/skills/reload") + + assert failed_response.status_code == 500 + assert failed_response.json() == {"detail": "Failed to invalidate skills cache."} + assert "mounted skills unavailable" not in failed_response.text + assert "/srv/company/minio" not in failed_response.text + + with prompt_module._enabled_skills_lock: + assert prompt_module._enabled_skills_cache == last_good_skills + assert prompt_module._enabled_skills_refresh_active is False + + recovered_response = client.post("/api/skills/reload") + + assert recovered_response.status_code == 200 + assert recovered_response.json() == _SUCCESS_RESPONSE + with prompt_module._enabled_skills_lock: + assert prompt_module._enabled_skills_cache == recovered_skills + finally: + _reset_prompt_cache_state() + + +def test_reload_invalidates_all_user_caches_and_rescans_external_changes(monkeypatch, tmp_path: Path) -> None: + _write_skill(tmp_path, "changed-skill", "old description") + _write_skill(tmp_path, "removed-skill", "removed description") + + storage = LocalSkillStorage(host_path=str(tmp_path)) + config = SimpleNamespace( + skills=SimpleNamespace( + container_path="/mnt/skills", + use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage", + get_skills_path=lambda: tmp_path, + ), + skill_evolution=SimpleNamespace(enabled=False), + ) + monkeypatch.setattr(prompt_module, "get_or_new_skill_storage", lambda **_kwargs: storage) + monkeypatch.setattr(prompt_module, "get_or_new_user_skill_storage", lambda _user_id, **_kwargs: storage) + _reset_prompt_cache_state() + + try: + alice_before = prompt_module.get_enabled_skills_for_config(config, user_id="alice") + bob_before = prompt_module.get_enabled_skills_for_config(config, user_id="bob") + assert {skill.name for skill in alice_before} == {"changed-skill", "removed-skill"} + assert {skill.name for skill in bob_before} == {"changed-skill", "removed-skill"} + + _write_skill(tmp_path, "changed-skill", "new description") + _write_skill(tmp_path, "added-skill", "added description") + (tmp_path / "public" / "removed-skill" / "SKILL.md").unlink() + + anyio.run(prompt_module.refresh_skills_system_prompt_cache_async) + + with prompt_module._enabled_skills_lock: + assert prompt_module._enabled_skills_by_config_cache == {} + + alice_after = prompt_module.get_enabled_skills_for_config(config, user_id="alice") + skills_by_name = {skill.name: skill for skill in alice_after} + assert set(skills_by_name) == {"added-skill", "changed-skill"} + assert skills_by_name["changed-skill"].description == "new description" + + rendered_prompt = prompt_module.get_skills_prompt_section(app_config=config, user_id="alice") + assert "added-skill" in rendered_prompt + assert "new description" in rendered_prompt + assert "removed-skill" not in rendered_prompt + finally: + _reset_prompt_cache_state() + + +def test_reload_does_not_allow_inflight_user_scan_to_repopulate_stale_cache(monkeypatch, tmp_path: Path) -> None: + _write_skill(tmp_path, "changed-skill", "old description") + + storage = LocalSkillStorage(host_path=str(tmp_path)) + config = SimpleNamespace( + skills=SimpleNamespace( + container_path="/mnt/skills", + use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage", + get_skills_path=lambda: tmp_path, + ), + skill_evolution=SimpleNamespace(enabled=False), + ) + first_scan_started = threading.Event() + release_first_scan = threading.Event() + user_load_count = 0 + + class BlockingFirstLoadStorage: + def load_skills(self, *, enabled_only: bool): + nonlocal user_load_count + user_load_count += 1 + skills = storage.load_skills(enabled_only=enabled_only) + if user_load_count == 1: + first_scan_started.set() + assert release_first_scan.wait(timeout=5) + return skills + + user_storage = BlockingFirstLoadStorage() + monkeypatch.setattr(prompt_module, "get_or_new_skill_storage", lambda **_kwargs: storage) + monkeypatch.setattr(prompt_module, "get_or_new_user_skill_storage", lambda _user_id, **_kwargs: user_storage) + _reset_prompt_cache_state() + + first_result = [] + first_error = [] + + def _load_for_alice() -> None: + try: + first_result.extend(prompt_module.get_enabled_skills_for_config(config, user_id="alice")) + except BaseException as exc: # pragma: no cover - surfaced by the main test thread + first_error.append(exc) + + load_thread = threading.Thread(target=_load_for_alice) + load_thread.start() + try: + assert first_scan_started.wait(timeout=5) + _write_skill(tmp_path, "changed-skill", "new description") + + anyio.run(prompt_module.refresh_skills_system_prompt_cache_async) + release_first_scan.set() + load_thread.join(timeout=5) + + assert not load_thread.is_alive() + assert first_error == [] + assert first_result[0].description == "old description" + with prompt_module._enabled_skills_lock: + assert prompt_module._enabled_skills_by_config_cache == {} + + next_result = prompt_module.get_enabled_skills_for_config(config, user_id="alice") + assert next_result[0].description == "new description" + assert user_load_count == 2 + finally: + release_first_scan.set() + load_thread.join(timeout=5) + _reset_prompt_cache_state() + + +def test_reload_refresh_raises_when_background_scan_wait_times_out(monkeypatch) -> None: + wait_timeouts = [] + + class NeverCompletesEvent: + def wait(self, timeout=None) -> bool: + wait_timeouts.append(timeout) + return False + + monkeypatch.setattr(prompt_module, "_invalidate_enabled_skills_cache", lambda: NeverCompletesEvent()) + + with pytest.raises(TimeoutError, match="Timed out waiting for enabled skills cache refresh"): + anyio.run(prompt_module.refresh_skills_system_prompt_cache_async) + + assert wait_timeouts == [prompt_module._ENABLED_SKILLS_REFRESH_WAIT_TIMEOUT_SECONDS] diff --git a/backend/tests/test_skills_router_authz.py b/backend/tests/test_skills_router_authz.py index 9e44afdea..4f1eb05d2 100644 --- a/backend/tests/test_skills_router_authz.py +++ b/backend/tests/test_skills_router_authz.py @@ -56,6 +56,7 @@ def _make_app(*, system_role: str) -> FastAPI: # tenant's injected skill set. _GUARDED_ENDPOINTS = [ ("post", "/api/skills/install", {"thread_id": "t1", "path": "mnt/user-data/outputs/x.skill"}), + ("post", "/api/skills/reload", None), ("get", "/api/skills/custom/demo", None), ("put", "/api/skills/custom/demo", {"content": "---\nname: demo\ndescription: hijacked\n---\n"}), ("delete", "/api/skills/custom/demo", None),