Skip to content

fix: rglob follow symlinks for skill discovery - #35299

Open
redpiggy-cyber wants to merge 1 commit into
NousResearch:mainfrom
redpiggy-cyber:pr/rglob-symlink-follow
Open

fix: rglob follow symlinks for skill discovery#35299
redpiggy-cyber wants to merge 1 commit into
NousResearch:mainfrom
redpiggy-cyber:pr/rglob-symlink-follow

Conversation

@redpiggy-cyber

Copy link
Copy Markdown
Contributor

Problem

Python's Path.rglob() does not descend into symlinked directories (see python/cpython#77609). This means skills located behind a symlink — such as ~/.hermes/skills/redpiggy -> ~/Documents/redpiggy-workspace/skills/ — are invisible to skill_manage and skill_usage tools.

The root cause is that iter_skill_index_files() uses os.walk(followlinks=True) (which follows symlinks), but _find_skill() and related functions in the skill manager and usage tools use Path.rglob() (which does not).

Fix

Add a rglob_follow() helper in agent/skill_utils.py that uses os.walk(followlinks=True) to traverse symlinked directories, matching the behavior already used by iter_skill_index_files().

Update all four rglob('SKILL.md') call sites across:

  • tools/skill_manager_tool.py_find_skill() and _find_skill_in_other_profiles()
  • tools/skill_usage.pylist_agent_created_skill_names() and _find_skill_dir()

Testing

  • Verified that symlinked skills (e.g., redpiggy) are now discoverable by skill_manage and skill_usage tools.
  • The rglob_follow() helper also prunes excluded directories (.git, .venv, node_modules, etc.) during traversal, consistent with is_excluded_skill_path().

Python's Path.rglob() does not descend into symlinked directories
(see https://bugs.python.org/issue40358). This means skills in
~/.hermes/skills/redpiggy/ (a symlink to ~/Documents/redpiggy-workspace/skills/)
are invisible to skill_manage and skill usage tools.

Add a rglob_follow() helper in agent/skill_utils.py that uses
os.walk(followlinks=True) to traverse symlinked skill directories,
matching the behavior already used by iter_skill_index_files().

Update all four rglob('SKILL.md') call sites across
tools/skill_manager_tool.py and tools/skill_usage.py to use
rglob_follow() instead.
@alt-glitch alt-glitch added type/bug Something isn't working tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have labels May 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of the symlink skill discovery fix chain — competes with #35244 (open, same approach using iter_skill_index_files), #7634 (open, walk_skill_files with cycle guard), and original #12624 (closed). All address the same Path.rglob() symlink limitation.

@Morad37 Morad37 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rglob_follow helper looks right — os.walk with followlinks=True is the standard workaround for Python's rglob not descending into symlinked dirs. The dirnames pruning for EXCLUDED_SKILL_DIRS inside walk is a nice touch. One thing I noticed: followlinks=True means a symlink-to-excluded-dir by a non-excluded name bypasses the pruning — but the original rglob has the same limitation, so this doesn't introduce a regression.

@alpindiay

Copy link
Copy Markdown

Automated Triage: Competing PRs Detected

Three PRs are independently fixing the same issue (symlink-discovery in skills, #35184):

They all modify tools/skill_management.py — only one can merge cleanly. Please coordinate and consolidate into a single PR.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the Path.rglob() / symlink-discovery mismatch. The premise is still valid on current main: tools/skill_manager_tool.py:597 uses rglob("SKILL.md"), while agent/skill_utils.py:796 uses os.walk(..., followlinks=True).

Problems

  • Please reuse iter_skill_index_files() rather than add a parallel rglob_follow() walker. The existing helper at agent/skill_utils.py:785-807 already follows links, returns sorted paths, and prunes support directories.
  • Current-head sibling paths remain unaddressed: tools/skill_usage.py:858 searches configured external directories with rglob(), and tools/skill_usage.py:925 does the same for usage_report().
  • The diff adds no regression tests. Symlink tests should gracefully skip where link creation is unsupported, following tests/tools/test_skill_manager_tool.py:357-360.

Suggested changes

  • Salvage this using iter_skill_index_files() at the applicable current-head call sites and add management/usage symlink regressions.

Automated hermes-sweeper review.

Comment thread agent/skill_utils.py
return any(part in EXCLUDED_SKILL_DIRS for part in parts)


def rglob_follow(root: Path, pattern: str):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iter_skill_index_files() already follows symlinks and also preserves sorted output plus support-directory pruning (agent/skill_utils.py:785-807 on current main). Reuse that existing iterator at these exact SKILL.md call sites rather than adding a divergent walker.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants