From 9f407577458dcf7e2b23f6ab13860c62e2827718 Mon Sep 17 00:00:00 2001 From: haosenwang1018 <1293965075@qq.com> Date: Wed, 6 May 2026 18:26:30 +0800 Subject: [PATCH] fix(agent): detect symlink cycles in iter_skill_index_files Closes #18809 ``os.walk(skills_dir, followlinks=True)`` performs no cycle detection, so a self-referencing skills tree (e.g. a stray symlink such as ``~/.hermes/skills/test-cycle/circular -> ~/.hermes/skills``) caused infinite recursion until the OS rejected the path with ``ENAMETOOLONG`` / ``ELOOP``. This blocked agent startup and any ``/skill`` discovery path because skill listing happens during init. Track each visited subdirectory's resolved (canonical) path via ``os.path.realpath`` and skip any directory we've already entered. This is the standard cycle-safe idiom for ``os.walk(followlinks=True)`` and matches how ``shutil`` and similar stdlib walkers handle the case. ``EXCLUDED_SKILL_DIRS`` filtering is preserved as the first pass so behavior on non-cyclic trees is identical. Tests cover: - A self-referencing subdir cycle terminates and still returns the legitimate skill once. - A two-cycle through cross-linked siblings terminates and returns both real ``SKILL.md`` files exactly once. - A plain non-symlinked tree is unaffected (regression guard). Symlink-specific tests are skipped on Windows where the semantics differ. Co-Authored-By: Claude Opus 4.7 (1M context) --- agent/skill_utils.py | 23 ++++++++++ tests/agent/test_skill_utils.py | 79 ++++++++++++++++++++++++++++++++- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/agent/skill_utils.py b/agent/skill_utils.py index cecbb1fc6c291..1b0f40165338d 100644 --- a/agent/skill_utils.py +++ b/agent/skill_utils.py @@ -441,10 +441,33 @@ def iter_skill_index_files(skills_dir: Path, filename: str): """Walk skills_dir yielding sorted paths matching *filename*. Excludes ``.git``, ``.github``, ``.hub``, ``.archive`` directories. + Symlink cycles are detected by tracking each directory's resolved + (canonical) path, so a self-referencing tree under ``skills_dir`` will + not cause infinite recursion (#18809). """ matches = [] + try: + root_real = os.path.realpath(skills_dir) + except OSError: + root_real = str(skills_dir) + visited_realpaths = {root_real} for root, dirs, files in os.walk(skills_dir, followlinks=True): + # Filter excluded directories first to keep behavior unchanged. dirs[:] = [d for d in dirs if d not in EXCLUDED_SKILL_DIRS] + # Drop any subdirectory whose canonical path we've already visited — + # this is the only way to break os.walk's untracked symlink loops. + unique_dirs = [] + for d in dirs: + try: + real = os.path.realpath(os.path.join(root, d)) + except OSError: + # Treat unreadable entries as visited so we don't keep retrying. + continue + if real in visited_realpaths: + continue + visited_realpaths.add(real) + unique_dirs.append(d) + dirs[:] = unique_dirs if filename in files: matches.append(Path(root) / filename) for path in sorted(matches, key=lambda p: str(p.relative_to(skills_dir))): diff --git a/tests/agent/test_skill_utils.py b/tests/agent/test_skill_utils.py index 206cc5f4b11bd..0677414de70aa 100644 --- a/tests/agent/test_skill_utils.py +++ b/tests/agent/test_skill_utils.py @@ -1,6 +1,11 @@ -"""Tests for agent/skill_utils.py — extract_skill_conditions metadata handling.""" +"""Tests for agent/skill_utils.py.""" -from agent.skill_utils import extract_skill_conditions +import os +import sys + +import pytest + +from agent.skill_utils import extract_skill_conditions, iter_skill_index_files def test_metadata_as_dict_with_hermes(): @@ -56,3 +61,73 @@ def test_metadata_missing_entirely(): "fallback_for_tools": [], "requires_tools": [], } + + +# ── iter_skill_index_files cycle detection (#18809) ─────────────────────── + + +@pytest.mark.skipif( + sys.platform == "win32", + reason="symlink cycles require POSIX semantics; Windows handling differs", +) +class TestIterSkillIndexFilesSymlinkCycles: + """Regression tests for #18809 — ``os.walk(followlinks=True)`` does not + detect symlink cycles on its own. ``iter_skill_index_files`` must guard + against cyclic symlink trees so a malformed user skills directory does + not hang skill discovery / agent startup.""" + + def test_self_referencing_subdir_does_not_loop(self, tmp_path): + """A subdirectory symlinked back to its ancestor used to recurse + until the OS rejected the path. The function must now terminate.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + # A real skill so there is something to find. + real_skill = skills_dir / "real-skill" + real_skill.mkdir() + (real_skill / "SKILL.md").write_text("# real skill") + # Create a self-referencing cycle: + # skills/test-cycle/circular -> skills + cycle_dir = skills_dir / "test-cycle" + cycle_dir.mkdir() + (cycle_dir / "circular").symlink_to(skills_dir, target_is_directory=True) + + # Without cycle detection this never returns. + results = list(iter_skill_index_files(skills_dir, "SKILL.md")) + + # Real skill is still discovered. + assert any(p.name == "SKILL.md" for p in results) + # The cycle did not multiply the same SKILL.md. + assert len(results) == 1 + + def test_cycle_via_symlinked_sibling_does_not_loop(self, tmp_path): + """Cycles can also form via two sibling directories cross-linking + each other. The realpath-tracking guard should still catch this.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + a = skills_dir / "a" + b = skills_dir / "b" + a.mkdir() + b.mkdir() + (a / "SKILL.md").write_text("# a") + (b / "SKILL.md").write_text("# b") + # a/b_link -> b ; b/a_link -> a — creates a 2-cycle through symlinks. + (a / "b_link").symlink_to(b, target_is_directory=True) + (b / "a_link").symlink_to(a, target_is_directory=True) + + results = list(iter_skill_index_files(skills_dir, "SKILL.md")) + + # Both real SKILL.md files are still found, exactly once each. + names = sorted(str(p.relative_to(skills_dir)) for p in results) + assert names == [os.path.join("a", "SKILL.md"), os.path.join("b", "SKILL.md")] + + def test_normal_tree_unchanged(self, tmp_path): + """Sanity: a plain skills tree without symlinks is unaffected.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + for name in ("alpha", "beta", "gamma"): + (skills_dir / name).mkdir() + (skills_dir / name / "SKILL.md").write_text(f"# {name}") + + results = list(iter_skill_index_files(skills_dir, "SKILL.md")) + names = sorted(p.parent.name for p in results) + assert names == ["alpha", "beta", "gamma"]