From 92e1372d5a3439a74aa3c33ce7acd331446ed433 Mon Sep 17 00:00:00 2001 From: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Date: Sun, 25 Oct 2026 15:30:20 +0800 Subject: [PATCH] fix(skills): honor skills.container_path in user-scoped skill storage (#6770) * fix(skills): honor skills.container_path in user-scoped skill storage The user-scoped factory never passed container_path, so UserScopedSkillStorage kept the default /mnt/skills while public storage honored the configured root, and consumers reading the root from the storage (container registry, skill activation) built paths that do not exist once an operator moves the mount root. Seed container_path from the resolved app config, matching the public-storage factory; an explicit caller override still wins. Fixes #6767 * fix(skills): seed container_path in the legacy-visibility kwargs branch willem-bd review nit on #6770: user_should_see_legacy_skills constructs a one-off UserScopedSkillStorage when callers pass kwargs, and that branch skipped the app_config container_path seeding, so a future caller combining kwargs with a custom skills root would read the default /mnt/skills instead. Seed it the same way the factory does; an explicit caller override still wins. --- .../deerflow/skills/storage/__init__.py | 11 +++++++ .../tests/test_user_scoped_skill_storage.py | 29 ++++++++++++++++++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/backend/packages/harness/deerflow/skills/storage/__init__.py b/backend/packages/harness/deerflow/skills/storage/__init__.py index 7f669e63bb..6d7da6730e 100644 --- a/backend/packages/harness/deerflow/skills/storage/__init__.py +++ b/backend/packages/harness/deerflow/skills/storage/__init__.py @@ -128,6 +128,9 @@ def get_or_new_user_skill_storage(user_id: str, **kwargs) -> SkillStorage: if app_config is None: app_config = get_app_config() kwargs["app_config"] = app_config + # User-scoped storage must mount skills at the same configured root as + # public storage; an explicit caller override still wins. + kwargs.setdefault("container_path", app_config.skills.container_path) # Always acquire lock so move_to_end is safe — makes this a true LRU # cache instead of FIFO. The overhead is negligible since dict ops are @@ -161,6 +164,14 @@ def user_should_see_legacy_skills(user_id: str, **kwargs) -> bool: if kwargs: from deerflow.config.paths import make_safe_user_id + # One-off constructions mount at the configured root too, same as the + # factory path; an explicit caller override still wins. + app_config = kwargs.get("app_config") + if app_config is None: + from deerflow.config import get_app_config + + app_config = get_app_config() + kwargs.setdefault("container_path", app_config.skills.container_path) storage = UserScopedSkillStorage(make_safe_user_id(user_id), **kwargs) else: storage = get_or_new_user_skill_storage(user_id) diff --git a/backend/tests/test_user_scoped_skill_storage.py b/backend/tests/test_user_scoped_skill_storage.py index 37cff7eb4e..93a4215eec 100644 --- a/backend/tests/test_user_scoped_skill_storage.py +++ b/backend/tests/test_user_scoped_skill_storage.py @@ -13,7 +13,7 @@ import pytest from deerflow.config.paths import Paths from deerflow.skills.installer import SkillSecurityScanError from deerflow.skills.security_scanner import ScanResult -from deerflow.skills.storage import reset_skill_storage, reset_user_skill_storage +from deerflow.skills.storage import get_or_new_user_skill_storage, reset_skill_storage, reset_user_skill_storage, user_should_see_legacy_skills from deerflow.skills.storage.local_skill_storage import LocalSkillStorage from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage from deerflow.skills.types import SkillCategory @@ -103,6 +103,33 @@ class TestPathRedirection: assert user_storage.user_id == "test-user" +class TestFactoryContainerPath: + """get_or_new_user_skill_storage must mount skills at the configured root, + same as the public-storage factory.""" + + def test_factory_honors_configured_container_path(self, base_dir: Path, skills_root: Path, config): + config.skills.container_path = "/custom/skills/root" + with patch("deerflow.config.paths.get_paths", return_value=Paths(base_dir=base_dir)): + with patch("deerflow.config.paths._paths", None): + storage = get_or_new_user_skill_storage("test-user", app_config=config) + assert storage.get_container_root() == "/custom/skills/root" + + def test_factory_explicit_container_path_wins(self, base_dir: Path, skills_root: Path, config): + with patch("deerflow.config.paths.get_paths", return_value=Paths(base_dir=base_dir)): + with patch("deerflow.config.paths._paths", None): + storage = get_or_new_user_skill_storage("test-user", app_config=config, container_path="/explicit/root") + assert storage.get_container_root() == "/explicit/root" + + def test_legacy_visibility_kwargs_branch_seeds_container_path(self, config): + """user_should_see_legacy_skills must not fall back to the default root + when a caller passes kwargs while a custom root is configured.""" + config.skills.container_path = "/custom/skills/root" + with patch("deerflow.skills.storage.UserScopedSkillStorage") as mock_storage_cls: + mock_storage_cls.return_value.load_skills.return_value = [] + assert user_should_see_legacy_skills("test-user", app_config=config) is False + assert mock_storage_cls.call_args.kwargs["container_path"] == "/custom/skills/root" + + class TestWriteAndRead: """Writes go to user dir, reads from user dir when present."""