fix(skills): follow symlinks with cycle guard across all skill discovery paths - #7634
fix(skills): follow symlinks with cycle guard across all skill discovery paths#7634graysonzeng wants to merge 1 commit into
Conversation
e18682e to
9971ddd
Compare
9971ddd to
e1a8540
Compare
|
Likely duplicate of #12624 — same fix: os.walk(followlinks=True) with cycle guard for symlinked skill directories. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real remaining gap in symlinked skill discovery. Current main already follows symlinks for the primary iterator (agent/skill_utils.py:796), but gateway/run.py:2253, tools/skill_manager_tool.py:597, and hermes_cli/profiles.py:789 still use rglob().
Problems
- Proposed
agent/skill_utils.py:455prunes onlyEXCLUDED_SKILL_DIRS. Current main also stops walkingreferences/,templates/,assets/, andscripts/below a skill root (agent/skill_utils.py:797-803). Removing that rule would rediscover support-packageSKILL.mdfiles as active skills, contrary totests/agent/test_skill_utils.py:195-218. - The new symlink setup at
tests/agent/test_skill_commands.py:58and:72is not portable where symlink creation is unavailable. Existing main coverage froma884f6d5dcatchesOSError/NotImplementedErrorand skips.
Suggested changes
- Add the inode cycle guard to the existing shared iterator while preserving its current support-directory pruning, then cover both cycle safety and the retained exclusion contract in
tests/agent/test_skill_utils.py. - Use the established skip-on-unavailable-symlink test helper.
- Audit remaining discovery-specific
rglob()paths before claiming all-path coverage.
Automated hermes-sweeper review.
| dirs[:] = [] | ||
| continue | ||
| seen_real.add(ident) | ||
| dirs[:] = [d for d in dirs if d not in EXCLUDED_SKILL_DIRS] |
There was a problem hiding this comment.
EXCLUDED_SKILL_DIRS intentionally does not contain references, templates, assets, or scripts, because they can be valid category names. Current main additionally prunes those only when they are support directories below a skill root (agent/skill_utils.py:797-803). Please preserve that condition here; otherwise archived/support-package SKILL.md files become active skills.
| ) | ||
| skills_dir = tmp_path / "skills" | ||
| skills_dir.mkdir() | ||
| (skills_dir / "linked-skill").symlink_to(real_dir, target_is_directory=True) |
There was a problem hiding this comment.
Please catch OSError and NotImplementedError here and skip when symlink creation is unavailable. Main's existing symlink coverage uses that pattern (a884f6d5d) so native Windows runners without symlink privilege do not fail.
Summary
Skills installed via directory symlink (e.g.
ln -s /path/to/skill ~/.hermes/skills/my-skill) are silently ignored because Python'spathlib.Path.rglob()does not follow symlinked directories by default. This affects all skill discovery paths — not just slash command registration but alsoskills list,skills categories,skill_viewfallback search,skill_manageroperations, and gateway unavailable-skill hints.Root Cause
Path.rglob()skips symlinked directories during traversal (documented Python behavior). When a skill directory itself is a symlink,rglob("SKILL.md")never enters it, so the skill is invisible to the entire system.Fix
Introduce a single shared
walk_skill_files()function inagent/skill_utils.pythat:os.walk(followlinks=True)to traverse into symlinked directories(st_dev, st_ino)visited set to detect and break symlink cycles.git,.github,.hubdirectories (consistent with existing behavior)Then replace all
rglob("SKILL.md")calls across the skill discovery surface:agent/skill_utils.pyiter_skill_index_fileswalk_skill_filesagent/skill_commands.pyscan_skill_commandsrglob→walk_skill_filestools/skills_tool.py_find_all_skillsrglob→walk_skill_filestools/skills_tool.pyskills_categoriesrglob→walk_skill_filestools/skills_tool.pyskill_view(name search)rglob→walk_skill_filestools/skill_manager_tool.py_find_skillrglob→walk_skill_filesgateway/run.py_check_unavailable_skillrglob→walk_skill_fileshermes_cli/profiles.py_count_skillsrglob→walk_skill_filesNot changed (repo-internal dirs, no user symlinks expected):
tools/skills_hub.py— scansoptional-skills/bundled in repotools/skills_sync.py— scans bundled skillshermes_cli/dump.py— export utilityReproduction
Cycle Safety
A symlink cycle (e.g.
skill-dir/loop -> ~/.hermes/skills/) would causeos.walk(followlinks=True)to recurse infinitely. Thewalk_skill_filesfunction tracks visited directories by(st_dev, st_ino)and prunes already-seen directories, preventing infinite loops.Test Plan
Two new tests added to
tests/agent/test_skill_commands.py:test_finds_symlinked_skill— symlinked skill directory is discovered byscan_skill_commands()test_symlink_cycle_does_not_hang— directory cycle does not cause infinite recursionExisting 26 tests continue to pass.
Known Limitation
Symlinked skills whose resolved path falls outside
~/.hermes/skills/will trigger a security warning log ("skill file is outside the trusted skills directory"). This is cosmetic and does not affect functionality. Addressing this is out of scope for this PR.