fix(agent): detect symlink cycles in iter_skill_index_files#20658
Open
haosenwang1018 wants to merge 1 commit into
Open
fix(agent): detect symlink cycles in iter_skill_index_files#20658haosenwang1018 wants to merge 1 commit into
haosenwang1018 wants to merge 1 commit into
Conversation
Closes NousResearch#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) <noreply@anthropic.com>
Collaborator
|
Duplicate of #18815 — both add cycle detection to |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
Closes #18809
Root cause
os.walk(skills_dir, followlinks=True)performs no cycle detection on its own — it relies on the caller to filter cyclic entries. A stray symlink under `~/.hermes/skills` that pointed back to an ancestor (the bug report's reproduction does this withtest-cycle/circular -> ~/.hermes/skills) causediter_skill_index_filesto recurse indefinitely until the OS rejected the path withENAMETOOLONG/ELOOP. Because skill discovery runs during agent init, this blocked startup entirely, not just the/skillcommand.The pre-existing
EXCLUDED_SKILL_DIRSfilter only covers.git/.github/.hub/.archive— it provides no defense against a generic symlink loop.Fix
Track each visited subdirectory's resolved canonical path via
os.path.realpathand drop any candidate whose realpath has already been entered. This is the standard cycle-safe idiom foros.walk(followlinks=True). TheEXCLUDED_SKILL_DIRSfilter still runs first so behavior on plain non-cyclic trees is identical to before.OSErrorduringrealpathresolution is treated as "skip" rather than propagated, so a single broken or unreadable symlink can't take down skill discovery.Tests
Added in
tests/agent/test_skill_utils.py:test_self_referencing_subdir_does_not_loop— reproduces the bug report's scenario; without the fix this test never returns.test_cycle_via_symlinked_sibling_does_not_loop— covers the harder case of a two-cycle formed by sibling cross-links.test_normal_tree_unchanged— regression guard for plain trees.The symlink tests are skipped on Windows because POSIX
ELOOPsemantics don't apply.