fix(skills): prune ancestor cycles during discovery (#5921)

README.md

@@ -1074,6 +1074,8 @@ When deferred skill discovery is enabled, `describe_skill` ranks installed skill
 
 A skill directory is a package boundary: once DeerFlow finds its `SKILL.md`, nested `SKILL.md` files under that package (for example evaluation fixtures) remain supporting data and are not registered as runtime skills. This applies to managed integration packs as well as public and custom skills. Namespace directories without their own `SKILL.md` can still group nested skills.
 
+Discovery follows operator-managed directory symlinks, but skips links back to an ancestor directory so a cyclic namespace does not repeatedly rescan the same tree. Independent links to the same external skill tree remain supported.
+
 Skill Markdown and bundled text resources use UTF-8. Skill-creator CLI and review utilities read and write text explicitly as UTF-8 so localized skills behave consistently across operating systems.
 
 Users can explicitly activate an enabled skill for a single turn by starting the request with `/skill-name`, for example `/data-analysis analyze uploads/foo.csv`. DeerFlow loads that skill's `SKILL.md` as hidden current-turn context while leaving the base prompt limited to skill metadata. Slash activation respects disabled skills, custom-agent skill whitelists, and existing channel commands such as `/new` and `/help`.

backend/packages/harness/deerflow/skills/AGENTS.md

@@ -3,7 +3,7 @@
 - **Integration package boundaries**: recurse through provider and namespace directories, but stop at each package's `SKILL.md`. Nested fixtures must neither become catalog entries nor shadow real public skills; regression coverage lives in `tests/test_user_scoped_skill_storage.py`.
 - **Location**: global public skills live under `deer-flow/skills/public/`; user-authored custom skills live under `{DEER_FLOW_HOME}/users/{user_id}/skills/custom/`; globally managed integration skills live under `{DEER_FLOW_HOME}/integrations/skills/{provider}/`; per-user integration credentials remain under `{DEER_FLOW_HOME}/users/{user_id}/integrations/{provider}/{config,data}`
 - **Format**: Directory with `SKILL.md` (YAML frontmatter: name, description, license, allowed-tools as a spec-compatible string or YAML list, argument-hint, required-secrets). Exact portable spellings such as `Bash`, `WebFetch`, `WebSearch`, `Glob`, `Grep`, `Read`, `Write`, and `Edit` map to `bash`, `web_fetch`, `web_search`, `glob`, `grep`, `read_file`, `write_file`, and `str_replace`; lowercase or otherwise unknown scalar names and YAML-list entries preserve their exact runtime spelling. Argument-scoped entries remain literal and inactive because the tool policy does not inspect arguments; the scalar tokenizer keeps spaces, quotes, and escaped parentheses inside patterns intact.
-- **Loading**: `load_skills()` recursively scans public, per-user custom, global integration, and legacy custom locations for `SKILL.md`, parses metadata, and reads enabled state from extensions_config.json plus per-user skill state for non-public categories; that directory is a package boundary, so no nested `SKILL.md` is registered as a runtime skill. A custom skill directory may be a one-level symlink to an external directory for compatibility with operator-managed skill trees; activation still rejects a symlinked `SKILL.md` or deeper path escape. Deleting such a skill unlinks the entry and leaves the external tree untouched. 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.
+- **Loading**: `load_skills()` recursively scans public, per-user custom, global integration, and legacy custom locations for `SKILL.md`, parses metadata, and reads enabled state from extensions_config.json plus per-user skill state for non-public categories; that directory is a package boundary, so no nested `SKILL.md` is registered as a runtime skill. A custom skill directory may be a one-level symlink to an external directory for compatibility with operator-managed skill trees; activation still rejects a symlinked `SKILL.md` or deeper path escape. Deleting such a skill unlinks the entry and leaves the external tree untouched. 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. Ancestor-link cycles are pruned; independent aliases remain discoverable.
 - **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**: 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 skill allowlists do not clamp the baseline tool set. A lead custom Agent's explicit `skills` list is additionally enforced at the sandbox filesystem layer (see Sandbox projection); subagent skill lists still scope discovery and activation only because concurrently delegated subagents share the lead thread sandbox. Subagents render only skill discovery metadata at startup and reuse the same adjacent `SkillActivationMiddleware` + `SkillToolPolicyMiddleware` pair as the lead; their configured `skills` field limits discovery and activation instead of eagerly loading bodies or unioning policies. 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`, `list_background_tasks`, and `cancel_background_task` likewise require explicit declarations. 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. The dynamic `allowed-tools` policy remains best-effort behavioral scoping: alternate loading paths are not captured and bounded autonomous context may evict entries.
 - **Sandbox projection**: `skills/projection.py` materializes enabled-only shared trees at `{base_dir}/skills_view/public` and `{base_dir}/users/{user_id}/skills_view/{custom,legacy,integrations}`. A lead Agent with an explicit `skills` allowlist (including `[]`) gets the intersection of enabled public/user-visible skills and that allowlist at `{base_dir}/users/{user_id}/threads/{thread_id}/skills_view/{public,custom,legacy,integrations}`. `skills=None` keeps the shared zero-copy mount until a thread has used an explicit policy; later unrestricted runs repopulate the same stable thread root with all enabled skills. Rebuilds sign source state, view state, and normalized policy in a manifest, revoke every old category before adding the new policy, stage copies in temporary directories, and atomically replace files. Category root inodes stay stable for live bind mounts; concurrent readers can briefly see fewer skills during a policy change, never a skill revoked by the new policy. Policy-scoped copies reject absolute symlinks and relative symlinks that resolve outside their own skill package, preventing a permitted package from linking back to an omitted source. It copies files into the view (`_copy_into_view`) so a sandbox write cannot mutate the canonical skill inode; the operational trade-off is an O(total bytes) I/O and per-user/thread storage multiplier across rebuilds, prioritized for write isolation over zero-copy hardlinks. Steady-state freshness checks combine source and view metadata tree digests, so in-sandbox view tampering is detected and repaired on the next acquire. Storage writes, archive installs, deletes, and toggles rebuild shared scopes under a cross-process lock; Gateway boot ensures only the shared public view, user views are repaired lazily on acquire, and Agent thread views are recomputed before sandbox reuse. Managed integration packages are global, but their projected category is per-user because enabled state is isolated. User projection rebuilds re-read global enable state from disk instead of the process singleton, so a toggle handled by another Gateway worker is reflected on the next acquire. Gateway public-skill toggles take the public projection lock before the shared `extensions_config_write_lock`, re-read an existing config raw from disk, change only that skill's entry, and rebuild before responding; keep this as one worker-owned critical section so MCP writes cannot interleave and request cancellation cannot release either lock while the worker still runs. The shared public steady-state signature check runs without the global projection lock; stale/error paths take the lock and re-check before rebuilding or clearing. User and thread scope checks are serialized per scope. Projection failures clear the affected view before raising.

backend/packages/harness/deerflow/skills/storage/local_skill_storage.py

@@ -6,7 +6,6 @@ import asyncio
 import errno
 import json
 import logging
-import os
 import shutil
 import tempfile
 from collections.abc import Iterable
@@ -16,7 +15,7 @@ from pathlib import Path
 from deerflow.config.runtime_paths import resolve_path
 from deerflow.constants import DEFAULT_SKILLS_CONTAINER_PATH
 from deerflow.skills.permissions import make_skill_written_path_sandbox_readable
-from deerflow.skills.storage.skill_storage import SKILL_MD_FILE, SkillStorage
+from deerflow.skills.storage.skill_storage import SKILL_MD_FILE, SkillStorage, walk_skill_directories
 from deerflow.skills.types import SkillCategory
 
 logger = logging.getLogger(__name__)
@@ -79,7 +78,7 @@ class LocalSkillStorage(SkillStorage):
             category_path = self._host_root / category.value
             if not category_path.exists() or not category_path.is_dir():
                 continue
-            for current_root, dir_names, file_names in os.walk(category_path, followlinks=True):
+            for current_root, dir_names, file_names in walk_skill_directories(category_path):
                 dir_names[:] = sorted(name for name in dir_names if not name.startswith("."))
                 if SKILL_MD_FILE not in file_names:
                     continue

backend/packages/harness/deerflow/skills/storage/skill_storage.py

@@ -4,6 +4,7 @@ from __future__ import annotations
 
 import dataclasses
 import logging
+import os
 import re
 from abc import ABC, abstractmethod
 from collections.abc import Iterable
@@ -17,6 +18,31 @@ logger = logging.getLogger(__name__)
 _SKILL_NAME_PATTERN = re.compile(r"^[a-z0-9]+(?:-[a-z0-9]+)*$")
 
 
+def walk_skill_directories(root: Path) -> Iterable[tuple[str, list[str], list[str]]]:
+    """Follow directory links, but prune links back to the current ancestors.
+
+    Keep os.walk's mutable directory list so callers retain their namespace,
+    hidden-directory and package-boundary rules. Track only the current branch:
+    two independent aliases of an external skill tree must both be discoverable.
+    """
+    ancestors: list[tuple[Path, Path]] = []
+    for current_root, dir_names, file_names in os.walk(root, followlinks=True):
+        current_path = Path(current_root)
+        while ancestors and ancestors[-1][0] != current_path.parent:
+            ancestors.pop()
+        try:
+            resolved = current_path.resolve(strict=True)
+        except (OSError, RuntimeError):
+            # The directory may have disappeared or its link changed mid-scan.
+            dir_names.clear()
+            continue
+        if any(resolved == real_path for _, real_path in ancestors):
+            dir_names.clear()
+            continue
+        ancestors.append((current_path, resolved))
+        yield current_root, dir_names, file_names
+
+
 def read_text_or_none(path: Path) -> str | None:
     """Return *path*'s content as UTF-8 text, or ``None`` when it is not text.
 

backend/packages/harness/deerflow/skills/storage/user_scoped_skill_storage.py

@@ -41,7 +41,7 @@ from pathlib import Path
 from deerflow.constants import DEFAULT_SKILLS_CONTAINER_PATH
 from deerflow.skills.permissions import make_skill_written_path_sandbox_readable
 from deerflow.skills.storage.local_skill_storage import LocalSkillStorage
-from deerflow.skills.storage.skill_storage import SKILL_MD_FILE
+from deerflow.skills.storage.skill_storage import SKILL_MD_FILE, walk_skill_directories
 from deerflow.skills.types import SkillCategory
 
 logger = logging.getLogger(__name__)
@@ -274,7 +274,7 @@ class UserScopedSkillStorage(LocalSkillStorage):
         # 1. Public skills: always from global root
         public_path = self._host_root / SkillCategory.PUBLIC.value
         if public_path.exists() and public_path.is_dir():
-            for current_root, dir_names, file_names in os.walk(public_path, followlinks=True):
+            for current_root, dir_names, file_names in walk_skill_directories(public_path):
                 dir_names[:] = sorted(name for name in dir_names if not name.startswith("."))
                 if SKILL_MD_FILE not in file_names:
                     continue
@@ -285,7 +285,7 @@ class UserScopedSkillStorage(LocalSkillStorage):
         # enabled state is still merged from this user's _skill_states.json.
         integration_path = self._integrations_root
         if integration_path.exists() and integration_path.is_dir():
-            for current_root, dir_names, file_names in os.walk(integration_path, followlinks=True):
+            for current_root, dir_names, file_names in walk_skill_directories(integration_path):
                 dir_names[:] = sorted(name for name in dir_names if not name.startswith("."))
                 if SKILL_MD_FILE not in file_names:
                     continue
@@ -296,7 +296,7 @@ class UserScopedSkillStorage(LocalSkillStorage):
         user_custom_exists = False
         user_custom_path = self._user_custom_root
         if user_custom_path.exists() and user_custom_path.is_dir():
-            for current_root, dir_names, file_names in os.walk(user_custom_path, followlinks=True):
+            for current_root, dir_names, file_names in walk_skill_directories(user_custom_path):
                 dir_names[:] = sorted(name for name in dir_names if not name.startswith(".") and name != ".history")
                 if SKILL_MD_FILE not in file_names:
                     continue
@@ -312,7 +312,7 @@ class UserScopedSkillStorage(LocalSkillStorage):
         if not user_custom_exists:
             global_custom_path = self._global_custom_root
             if global_custom_path.exists() and global_custom_path.is_dir():
-                for current_root, dir_names, file_names in os.walk(global_custom_path, followlinks=True):
+                for current_root, dir_names, file_names in walk_skill_directories(global_custom_path):
                     dir_names[:] = sorted(name for name in dir_names if not name.startswith(".") and name != ".history")
                     if SKILL_MD_FILE not in file_names:
                         continue

backend/tests/test_skill_discovery_cycles.py

@@ -0,0 +1,94 @@
+"""Skill discovery must follow useful links without revisiting ancestors."""
+
+from pathlib import Path
+
+import pytest
+from support.symlinks import symlink_or_skip
+
+from deerflow.config.extensions_config import ExtensionsConfig
+from deerflow.config.paths import Paths
+from deerflow.skills.storage.local_skill_storage import LocalSkillStorage
+from deerflow.skills.storage.user_scoped_skill_storage import UserScopedSkillStorage
+
+
+@pytest.fixture(params=[(kind, category) for kind in ("local", "user") for category in ("public", "custom", "integrations", "legacy")])
+def storage_and_root(request, tmp_path, monkeypatch):
+    kind, category = request.param
+    host = tmp_path / "skills"
+    monkeypatch.setattr("deerflow.config.paths.get_paths", lambda: Paths(base_dir=tmp_path / "data"))
+    monkeypatch.setattr(ExtensionsConfig, "from_file", lambda: ExtensionsConfig())
+    if kind == "local":
+        storage = LocalSkillStorage(host_path=str(host))
+        root = host / category
+    else:
+        storage = UserScopedSkillStorage("test-user", host_path=str(host))
+        root = {
+            "public": host / "public",
+            "custom": storage._user_custom_root,
+            "integrations": storage._integrations_root,
+            "legacy": storage._global_custom_root,
+        }[category]
+    root.mkdir(parents=True)
+
+    # Keep regressions deterministic and bounded, even on the unfixed scanner.
+    # This delegates every entry and pruning decision to the real os.walk.
+    import os
+
+    real_walk = os.walk
+
+    def bounded_walk(*args, **kwargs):
+        for index, entry in enumerate(real_walk(*args, **kwargs)):
+            assert index < 32, "skill discovery keeps revisiting a cyclic directory"
+            yield entry
+
+    monkeypatch.setattr(os, "walk", bounded_walk)
+    return storage, root
+
+
+def _write_skill(path: Path, name: str):
+    path.mkdir(parents=True)
+    (path / "SKILL.md").write_text(f"---\nname: {name}\ndescription: Discovery regression\n---\n", encoding="utf-8")
+
+
+@pytest.mark.parametrize("cycle", ["self", "ancestor", "branching"])
+def test_load_skills_prunes_ancestor_cycles(storage_and_root, cycle):
+    storage, root = storage_and_root
+    namespace = root / "team"
+    _write_skill(namespace / "helper", "helper")
+    target = namespace if cycle == "self" else root
+    symlink_or_skip(namespace / "a-loop", target, target_is_directory=True)
+    if cycle == "branching":
+        symlink_or_skip(namespace / "b-loop", target, target_is_directory=True)
+
+    skills = storage.load_skills()
+
+    assert [skill.name for skill in skills] == ["helper"]
+    assert skills[0].relative_path == Path("team/helper")
+
+
+def test_discovery_preserves_external_aliases_and_package_boundaries(storage_and_root, tmp_path):
+    storage, root = storage_and_root
+    external = tmp_path / "operator-skills"
+    _write_skill(external / "helper", "helper")
+    _write_skill(external / "helper" / "evals" / "nested", "not-a-runtime-skill")
+    _write_skill(external / ".hidden", "hidden-skill")
+    for alias in ("a-linked", "b-linked"):
+        symlink_or_skip(root / alias, external, target_is_directory=True)
+
+    files = [path.relative_to(category_root) for _, category_root, path in storage._iter_skill_files()]
+
+    assert files == [Path("a-linked/helper/SKILL.md"), Path("b-linked/helper/SKILL.md")]
+    assert [skill.name for skill in storage.load_skills()] == ["helper"]
+
+
+def test_cycles_inside_external_namespaces_do_not_hide_aliases(storage_and_root, tmp_path):
+    storage, root = storage_and_root
+    external = tmp_path / "operator-skills"
+    _write_skill(external / "helper", "helper")
+    symlink_or_skip(external / "loop", external, target_is_directory=True)
+    for alias in ("a-linked", "b-linked"):
+        symlink_or_skip(root / alias, external, target_is_directory=True)
+
+    files = [path.relative_to(category_root) for _, category_root, path in storage._iter_skill_files()]
+
+    assert files == [Path("a-linked/helper/SKILL.md"), Path("b-linked/helper/SKILL.md")]