diff --git a/backend/app/gateway/capabilities.py b/backend/app/gateway/capabilities.py index 8dfc600657..d5eb6a95a7 100644 --- a/backend/app/gateway/capabilities.py +++ b/backend/app/gateway/capabilities.py @@ -194,6 +194,11 @@ class LarkAdapter: class SkillAdapter: async def list_installations(self, context: AdapterContext) -> list[CapabilityInstallation]: + # The listing reports the enabled flag, which the storage layer fails + # closed on an unreadable extensions config; keep this read surface + # loud about the broken config instead of serving the degraded list. + # Blocking filesystem IO: always off the event loop, like the scan below. + await asyncio.to_thread(skills._assert_extensions_config_readable) response = await asyncio.to_thread(lambda: skills._get_user_skill_storage(context.config).load_skills(enabled_only=False)) response = await skills._filter_visible_skills(context.request, context.config, response) return [ diff --git a/backend/app/gateway/routers/skills.py b/backend/app/gateway/routers/skills.py index 9d32f5a576..631e826ac9 100644 --- a/backend/app/gateway/routers/skills.py +++ b/backend/app/gateway/routers/skills.py @@ -338,6 +338,20 @@ async def _publish_skills_cache_reset(user_id: str | None) -> str | None: return await asyncio.to_thread(publish_skills_cache_reset, user_id=user_id) +def _assert_extensions_config_readable() -> None: + """Raise when the extensions config exists but cannot be read. + + The storage layer fails closed on an unreadable config (every skill stays + listed, none is enabled), which keeps the runtime activation gate safe + without taking the listing down. The read surface must stay loud instead: + the ``enabled`` column it reports is derived from this config, so serving + a degraded list would present a broken deployment as a legitimately empty + one. The raised error reaches each handler's generic 500 branch, which + keeps the host path in the server log and out of the response body. + """ + ExtensionsConfig.from_file() + + def _load_user_skills(config: AppConfig) -> list[Skill]: """Load every skill visible to the caller's storage, enabled or not. @@ -347,6 +361,7 @@ def _load_user_skills(config: AppConfig) -> list[Skill]: each ``SKILL.md`` -- work that scales with the number of installed skills (#5747 moved the same call off the loop for ``get_custom_skill``). """ + _assert_extensions_config_readable() return _get_user_skill_storage(config).load_skills(enabled_only=False) diff --git a/backend/packages/harness/deerflow/skills/storage/skill_storage.py b/backend/packages/harness/deerflow/skills/storage/skill_storage.py index c1ea77c810..66d08eb083 100644 --- a/backend/packages/harness/deerflow/skills/storage/skill_storage.py +++ b/backend/packages/harness/deerflow/skills/storage/skill_storage.py @@ -372,12 +372,24 @@ class SkillStorage(ABC): # to enabled when no explicit config entry exists (so newly # installed skills appear active without requiring a manual toggle). try: - from deerflow.config.extensions_config import ExtensionsConfig + from deerflow.config.extensions_config import ExtensionsConfig, _describe_load_failure extensions_config = ExtensionsConfig.from_file() skills = [dataclasses.replace(s, enabled=extensions_config.is_skill_enabled(s.name, s.category)) for s in skills] except Exception as e: - logger.warning("Failed to load extensions config: %s", e) + # A config that cannot be read says nothing about which skills are + # enabled, so the parsed default (``enabled=True``) must not stand + # in for an answer: that would turn every disabled skill back on + # because of an unrelated defect elsewhere in the file (a malformed + # ``mcpServers`` entry, a trailing comma). Fail closed instead — + # the skills stay listed, they just cannot be activated until the + # config is readable again. The sandbox projection already fails + # closed on the same error (``projection._rebuild_public_locked``); + # this keeps the runtime half of the gate consistent with it. + # ``from_file`` resolves ``$VAR`` placeholders before validation, so + # only the failure shape is safe to log (see ``_describe_load_failure``). + logger.warning("Failed to load extensions config; treating every skill as disabled (%s)", _describe_load_failure(e)) + skills = [dataclasses.replace(s, enabled=False) for s in skills] if enabled_only: skills = [s for s in skills if s.enabled] 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 dd8ce825fa..a7c6c23a5a 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 @@ -217,15 +217,27 @@ class UserScopedSkillStorage(LocalSkillStorage): # extensions_config (handled by ``super().load_skills`` above). Re-read # from disk here too so another worker's update cannot be masked by # this process's singleton cache while rebuilding a user projection. - from deerflow.config.extensions_config import ExtensionsConfig + from deerflow.config.extensions_config import ExtensionsConfig, _describe_load_failure - extensions_config = ExtensionsConfig.from_file() - skills = [ - dataclasses.replace(s, enabled=self.get_skill_enabled_state(s.name) and extensions_config.is_skill_enabled(s.name, s.category.value if hasattr(s.category, "value") else s.category)) - if dataclasses.is_dataclass(s) and not isinstance(s, type) and (s.category.value if hasattr(s.category, "value") else s.category) != SkillCategory.PUBLIC.value - else s - for s in skills - ] + try: + extensions_config = ExtensionsConfig.from_file() + skills = [ + dataclasses.replace(s, enabled=self.get_skill_enabled_state(s.name) and extensions_config.is_skill_enabled(s.name, s.category.value if hasattr(s.category, "value") else s.category)) + if dataclasses.is_dataclass(s) and not isinstance(s, type) and (s.category.value if hasattr(s.category, "value") else s.category) != SkillCategory.PUBLIC.value + else s + for s in skills + ] + except Exception as e: + # Same degraded mode as ``LocalSkillStorage.load_skills``: a config + # that cannot be read says nothing about which skills are enabled, + # so the per-user AND cannot be evaluated. Raising here would take + # the whole user-scoped listing down over an unrelated defect in the + # file, so fail closed instead — every skill is listed, none is + # enabled. ``from_file`` resolves ``$VAR`` placeholders before + # validation, so only the failure shape is safe to log (see + # ``_describe_load_failure``). + logger.warning("Failed to load extensions config; treating every user-scoped skill as disabled (%s)", _describe_load_failure(e)) + skills = [dataclasses.replace(s, enabled=False) for s in skills] if enabled_only: skills = [s for s in skills if s.enabled] diff --git a/backend/tests/test_skills_disabled_fail_closed.py b/backend/tests/test_skills_disabled_fail_closed.py new file mode 100644 index 0000000000..f39f4fabd7 --- /dev/null +++ b/backend/tests/test_skills_disabled_fail_closed.py @@ -0,0 +1,68 @@ +"""A broken extensions config must not re-enable a disabled skill. + +``SkillStorage.load_skills()`` merges the enabled state from ``extensions_config.json``. When that file +cannot be read, the parsed default (``enabled=True`` from ``parse_skill_file``) must not stand in for the +answer: an unrelated defect in the file — a malformed ``mcpServers`` entry, a trailing comma — would +otherwise silently re-enable every skill an operator disabled. + +The sandbox projection fails closed on the same error (``projection._rebuild_public_locked`` clears the +view), so the runtime half of the gate has to agree with it. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from deerflow.config.extensions_config import ExtensionsConfig, reset_extensions_config +from deerflow.skills.storage.local_skill_storage import LocalSkillStorage + +_SKILL_MD = """--- +name: {name} +description: {name} description +--- +body +""" + + +def _write_public_skill(root: Path, name: str) -> None: + skill_dir = root / "public" / name + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text(_SKILL_MD.format(name=name), encoding="utf-8") + + +@pytest.fixture() +def disabled_skill_root(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A skills root whose only skill is disabled in the extensions config.""" + root = tmp_path / "skills" + _write_public_skill(root, "demo") + config_path = tmp_path / "extensions_config.json" + config_path.write_text(json.dumps({"mcpServers": {}, "skills": {"demo": {"enabled": False}}}), encoding="utf-8") + monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path)) + reset_extensions_config() + return root + + +def test_disabled_skill_is_disabled_while_the_config_loads(disabled_skill_root: Path) -> None: + storage = LocalSkillStorage(str(disabled_skill_root)) + + assert [(skill.name, skill.enabled) for skill in storage.load_skills()] == [("demo", False)] + assert storage.load_skills(enabled_only=True) == [] + + +def test_disabled_skill_stays_disabled_when_the_config_cannot_be_read(disabled_skill_root: Path, monkeypatch: pytest.MonkeyPatch) -> None: + storage = LocalSkillStorage(str(disabled_skill_root)) + + def _raise(*_args: object, **_kwargs: object) -> None: + # What one schema-invalid ``mcpServers`` entry raises from ``McpServerConfig._validate_header_names``; + # a malformed JSON file raises ``ValueError`` from the same call site. + raise ValueError("headers maps the same HTTP header under two spellings ('X-Api-Key' and 'x-api-key')") + + monkeypatch.setattr(ExtensionsConfig, "from_file", classmethod(_raise)) + + # Still listed, so the skill does not silently disappear from the UI... + assert [(skill.name, skill.enabled) for skill in storage.load_skills()] == [("demo", False)] + # ...and never handed to the model as usable. + assert storage.load_skills(enabled_only=True) == [] diff --git a/backend/tests/test_user_scoped_skills_fail_closed.py b/backend/tests/test_user_scoped_skills_fail_closed.py new file mode 100644 index 0000000000..896bc79826 --- /dev/null +++ b/backend/tests/test_user_scoped_skills_fail_closed.py @@ -0,0 +1,91 @@ +"""A broken extensions config must not take down a user-scoped skill listing. + +``UserScopedSkillStorage._load_skills_unlocked`` re-reads the extensions config to AND the per-user state +with the global default. That second read is unguarded, so a config that cannot be read raised out of +``load_skills()`` for user-scoped deployments even after the local/global path learned to degrade. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from deerflow.config.extensions_config import ExtensionsConfig, reset_extensions_config +from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage + +_SKILL_MD = """--- +name: {name} +description: {name} description +--- +body +""" + + +def _raise_unreadable_config(*_args: object, **_kwargs: object) -> None: + # What one schema-invalid ``mcpServers`` entry raises from + # ``McpServerConfig._validate_header_names``; a malformed JSON file raises + # ``ValueError`` from the same call site. + raise ValueError("headers maps the same HTTP header under two spellings ('X-Api-Key' and 'x-api-key')") + + +@pytest.fixture() +def user_skills_root(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + """A skills root with one public skill and a readable extensions config.""" + root = tmp_path / "skills" + public = root / "public" / "demo" + public.mkdir(parents=True) + (public / "SKILL.md").write_text(_SKILL_MD.format(name="demo"), encoding="utf-8") + config_path = tmp_path / "extensions_config.json" + config_path.write_text(json.dumps({"mcpServers": {}, "skills": {}}), encoding="utf-8") + monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(config_path)) + reset_extensions_config() + return root + + +def test_user_scoped_listing_degrades_when_the_config_cannot_be_read(user_skills_root: Path, monkeypatch: pytest.MonkeyPatch) -> None: + storage = UserScopedSkillStorage("owner", host_path=str(user_skills_root)) + + assert [(skill.name, skill.enabled) for skill in storage.load_skills()] == [("demo", True)] + + monkeypatch.setattr(ExtensionsConfig, "from_file", classmethod(_raise_unreadable_config)) + + # The listing survives: no exception, every skill present and disabled. + assert [(skill.name, skill.enabled) for skill in storage.load_skills()] == [("demo", False)] + assert storage.load_skills(enabled_only=True) == [] + + +def test_user_scoped_custom_skill_with_per_user_state_degrades_when_the_config_cannot_be_read( + user_skills_root: Path, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """CUSTOM/LEGACY skills are the second read's own responsibility. + + Their per-user state exists only to be ANDed with the global default, so + the degraded branch has to cover them too: with a per-user state present, + an unreadable config still lists the custom skill — disabled — instead of + raising out of ``load_skills()``. Dropping the guard turns this red: the + second read raises out of the listing. + """ + from deerflow.config.paths import Paths + + # Route the user custom root (and the per-user states file) into tmp. + monkeypatch.setattr("deerflow.config.paths._paths", Paths(base_dir=tmp_path / "home")) + custom_dir = tmp_path / "home" / "users" / "owner" / "skills" / "custom" / "my-tool" + custom_dir.mkdir(parents=True) + (custom_dir / "SKILL.md").write_text(_SKILL_MD.format(name="my-tool"), encoding="utf-8") + states_file = tmp_path / "home" / "users" / "owner" / "skills" / "_skill_states.json" + states_file.write_text(json.dumps({"my-tool": {"enabled": True}}), encoding="utf-8") + + storage = UserScopedSkillStorage("owner", host_path=str(user_skills_root)) + + assert [(skill.name, skill.enabled) for skill in storage.load_skills()] == [("demo", True), ("my-tool", True)] + + monkeypatch.setattr(ExtensionsConfig, "from_file", classmethod(_raise_unreadable_config)) + + # The listing survives: every skill present (per-user state on disk for + # the custom one), none enabled, no exception. + assert [(skill.name, skill.enabled) for skill in storage.load_skills()] == [("demo", False), ("my-tool", False)] + assert storage.load_skills(enabled_only=True) == []