From d726ae60c3eef124befff82f415c29d2d5410fea Mon Sep 17 00:00:00 2001 From: Huixin615 Date: Wed, 29 Jul 2026 22:39:48 +0800 Subject: [PATCH] fix(skills): use managed integrations root for slash activation (#4570) * fix(skills): use managed integrations root for slash activation * refactor(skills): clarify integrations root getter --- AGENTS.md | 3 +- .../storage/user_scoped_skill_storage.py | 43 +++++++++++------ backend/tests/test_slash_skills.py | 46 +++++++++++++++++++ .../tests/test_user_scoped_skill_storage.py | 26 +++++++++++ 4 files changed, 102 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8907ac0b0..c9aab70db 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,7 +50,8 @@ deer-flow/ ├── frontend/ # Next.js frontend (pnpm) — see frontend/AGENTS.md ├── docker/ # docker-compose files, nginx config, provisioner ├── skills/ # Agent skills: public/ (committed), custom/ (gitignored) -│ # Managed integration skill packs are installed per user under .deer-flow/users/{user_id}/skills/integrations/ +│ # Managed integration skill packs are global at .deer-flow/integrations/skills/{provider}/ +│ # Integration credentials and enabled state remain per-user ├── contracts/ # Cross-component JSON contracts (e.g. subagent status, skill review) ├── scripts/ # Root orchestration scripts invoked by the Makefile (check, configure, doctor, support_bundle, serve, nginx, docker, deploy, setup_wizard) ├── tests/ # Root-level tests (currently tests/skills/ — public skill tests) diff --git a/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py b/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py index 123ee43a4..8cd0b99ef 100644 --- a/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py @@ -6,12 +6,12 @@ read from the global ``{base_dir}/skills/public/`` (read-only). Layout:: - /public//SKILL.md ← global, read-only - //SKILL.md ← per-user, read-write - ///SKILL.md ← per-user, read-only - /.history/.jsonl ← per-user history - /_skill_states.json ← per-user enabled state - //SKILL.md ← legacy fallback, read-only + /public//SKILL.md ← global, read-only + //SKILL.md ← per-user, read-write + ///SKILL.md ← global, read-only + /.history/.jsonl ← per-user history + /_skill_states.json ← per-user enabled state + //SKILL.md ← legacy fallback, read-only Fallback: when a user has no custom skills yet, global ``skills/custom/`` skills are yielded as ``SkillCategory.LEGACY`` (read-only) so they are @@ -385,26 +385,39 @@ class UserScopedSkillStorage(LocalSkillStorage): """Host path to this user's custom skills root directory.""" return self._user_custom_root + def get_integrations_root(self) -> Path: + """Host path to the global managed integration skills root directory.""" + return self._integrations_root + def get_user_integrations_root(self) -> Path: - """Host path to this user's managed integration skills root directory.""" - return self._user_integrations_root + """Compatibility alias for :meth:`get_integrations_root`.""" + return self.get_integrations_root() # ------------------------------------------------------------------ - # Path validation — accept per-user custom root as well as global root + # Path validation — accept public, per-user custom, and integration roots # ------------------------------------------------------------------ def validate_skill_file_path(self, skill_file: Path) -> Path: - """Accept files under *either* the global root or the per-user custom root. + """Accept files under the public, per-user custom, or integration root. - Custom skills live in ``_user_custom_root`` which is not a sub-path - of ``_host_root``, so the default implementation's single-root check - would reject them. This override allows both roots. + Custom and managed integration skills live outside ``_host_root``, so + the default implementation's single-root check would reject them. """ resolved_file = skill_file.resolve() - for allowed_root in (self._host_root.resolve(), self._user_custom_root.resolve(), self._user_integrations_root.resolve()): + allowed_roots = ( + self._host_root.resolve(), + self._user_custom_root.resolve(), + self._integrations_root.resolve(), + ) + for allowed_root in allowed_roots: try: resolved_file.relative_to(allowed_root) return resolved_file except ValueError: continue - raise ValueError(f"Resolved skill file {resolved_file} must stay within either the global skills root ({self._host_root.resolve()}) or the per-user custom root ({self._user_custom_root.resolve()}).") + raise ValueError( + f"Resolved skill file {resolved_file} must stay within the global skills root " + f"({self._host_root.resolve()}), the per-user custom root " + f"({self._user_custom_root.resolve()}), or the managed integration skills root " + f"({self._integrations_root.resolve()})." + ) diff --git a/backend/tests/test_slash_skills.py b/backend/tests/test_slash_skills.py index b033f3dcf..0a8a543fd 100644 --- a/backend/tests/test_slash_skills.py +++ b/backend/tests/test_slash_skills.py @@ -9,7 +9,10 @@ from langchain_core.messages import AIMessage, HumanMessage, ToolMessage from app.channels.commands import KNOWN_CHANNEL_COMMANDS from deerflow.agents.middlewares import skill_activation_middleware as middleware_module from deerflow.agents.middlewares.skill_activation_middleware import SkillActivationMiddleware, is_slash_skill_activation_reminder +from deerflow.config.extensions_config import ExtensionsConfig +from deerflow.config.paths import Paths from deerflow.skills.slash import RESERVED_SLASH_SKILL_NAMES, parse_slash_skill_reference, resolve_slash_skill +from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage from deerflow.skills.types import Skill, SkillCategory from deerflow.utils.messages import ORIGINAL_USER_CONTENT_KEY @@ -141,6 +144,49 @@ def test_skill_activation_middleware_injects_hidden_human_context_for_model_call assert request.state["messages"] == [original] +def test_skill_activation_middleware_reads_public_skill_from_real_user_scoped_storage(monkeypatch, tmp_path): + skills_root = tmp_path / "skills" + skill_dir = skills_root / "public" / "ppt-generation" + skill_dir.mkdir(parents=True) + skill_content = "---\nname: ppt-generation\ndescription: Create presentations\n---\n\n# Presentation workflow\n" + (skill_dir / "SKILL.md").write_text(skill_content, encoding="utf-8") + + app_config = SimpleNamespace( + skills=SimpleNamespace( + get_skills_path=lambda: skills_root, + container_path="/mnt/skills", + use="deerflow.skills.storage.local_skill_storage:LocalSkillStorage", + ), + ) + extensions_config = ExtensionsConfig() + monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: Paths(base_dir=tmp_path)) + monkeypatch.setattr(ExtensionsConfig, "from_file", classmethod(lambda cls, config_path=None: extensions_config)) + monkeypatch.setattr("deerflow.config.extensions_config.get_extensions_config", lambda: extensions_config) + storage = UserScopedSkillStorage("test-user", host_path=str(skills_root), app_config=app_config) + monkeypatch.setattr(middleware_module, "get_or_new_user_skill_storage", lambda user_id, **kwargs: storage) + + middleware = SkillActivationMiddleware( + app_config=app_config, + user_id="test-user", + slash_source_owner_token=_SLASH_SOURCE_OWNER_TOKEN, + ) + original = HumanMessage(content="/ppt-generation Create a simple deck", id="msg-real-storage") + request = _make_model_request([original]) + captured = {} + + def handler(model_request: ModelRequest): + captured["messages"] = model_request.messages + return AIMessage(content="ok") + + result = middleware.wrap_model_call(request, handler) + + assert isinstance(result, AIMessage) + activation_msg, user_msg = captured["messages"] + assert is_slash_skill_activation_reminder(activation_msg) + assert "Presentation workflow" in activation_msg.content + assert user_msg is original + + def test_skill_activation_middleware_does_not_duplicate_existing_activation(monkeypatch, tmp_path): skill = _make_skill(tmp_path, "data-analysis", content="# Data Analysis\nUse pandas.") monkeypatch.setattr(middleware_module, "get_or_new_skill_storage", lambda **kwargs: _make_storage(tmp_path, [skill])) diff --git a/backend/tests/test_user_scoped_skill_storage.py b/backend/tests/test_user_scoped_skill_storage.py index 532fb070f..d205a7d06 100644 --- a/backend/tests/test_user_scoped_skill_storage.py +++ b/backend/tests/test_user_scoped_skill_storage.py @@ -88,6 +88,12 @@ class TestPathRedirection: def test_public_skill_paths_still_use_global_root(self, user_storage: UserScopedSkillStorage, skills_root: Path): assert user_storage.get_skills_root_path() == skills_root + def test_managed_integration_skill_paths_use_global_root(self, user_storage: UserScopedSkillStorage, base_dir: Path): + assert user_storage.get_integrations_root() == base_dir / "integrations" / "skills" + + def test_user_integrations_root_is_compatibility_alias(self, user_storage: UserScopedSkillStorage): + assert user_storage.get_user_integrations_root() == user_storage.get_integrations_root() + def test_user_id_property(self, user_storage: UserScopedSkillStorage): assert user_storage.user_id == "test-user" @@ -282,6 +288,26 @@ class TestHistoryIsolation: class TestPathSafety: """UserScopedSkillStorage inherits path-traversal guards from LocalSkillStorage.""" + def test_accepts_skill_files_from_all_allowed_roots(self, user_storage: UserScopedSkillStorage, skills_root: Path, base_dir: Path): + skill_files = [ + skills_root / "public" / "public-skill" / "SKILL.md", + base_dir / "users" / "test-user" / "skills" / "custom" / "custom-skill" / "SKILL.md", + base_dir / "integrations" / "skills" / "lark-cli" / "lark-doc" / "SKILL.md", + ] + for skill_file in skill_files: + skill_file.parent.mkdir(parents=True, exist_ok=True) + skill_file.write_text(_skill_content(skill_file.parent.name), encoding="utf-8") + + assert [user_storage.validate_skill_file_path(skill_file) for skill_file in skill_files] == [skill_file.resolve() for skill_file in skill_files] + + def test_rejects_skill_file_outside_allowed_roots(self, user_storage: UserScopedSkillStorage, base_dir: Path): + skill_file = base_dir / "untrusted" / "escaped-skill" / "SKILL.md" + skill_file.parent.mkdir(parents=True) + skill_file.write_text(_skill_content("escaped-skill"), encoding="utf-8") + + with pytest.raises(ValueError, match="must stay within"): + user_storage.validate_skill_file_path(skill_file) + def test_rejects_invalid_skill_name(self, user_storage: UserScopedSkillStorage): with pytest.raises(ValueError, match="hyphen-case"): user_storage.get_custom_skill_dir("../../escaped")