Skip to content

fix(skills): use os.walk(followlinks=True) in _find_skill for symlinked skill dirs - #44210

Open
NorethSea wants to merge 2 commits into
NousResearch:mainfrom
NorethSea:fix/skill-manage-symlink
Open

fix(skills): use os.walk(followlinks=True) in _find_skill for symlinked skill dirs#44210
NorethSea wants to merge 2 commits into
NousResearch:mainfrom
NorethSea:fix/skill-manage-symlink

Conversation

@NorethSea

Copy link
Copy Markdown
Contributor

On Python 3.11, Path.rglob does not follow directory symlinks (the
follow_symlinks parameter was added in 3.12). This means skill_manage
cannot see skills that are symlinked into ~/.hermes/skills/ from shared
directories like ~/.agents/skills/.

iter_skill_index_files already uses os.walk(followlinks=True) and
correctly finds these skills. This change aligns _find_skill with that
existing pattern.

Problem

skill_view(), skills_list(), and the system prompt builder all see
symlinked skills, but skill_manage(action=patch) and all other
skill_manage actions return "Skill X not found in active profile".

Example: read-project-code is a symlink ~/.hermes/skills/read-project-code
→ ~/.agents/skills/read-project-code. It appears in skills_list() and
can be loaded via skill_view(), but skill_manage(action=patch, name=
read-project-code, ...) fails.

Changes

Replace skills_dir.rglob("SKILL.md") with os.walk(skills_dir,
followlinks=True) in _find_skill(), matching the approach already used
by iter_skill_index_files in agent/skill_utils.py.

How to test

pytest tests/tools/test_skill_manager_tool.py -v
# 90 passed

Manually:

from tools.skill_manager_tool import _find_skill
_find_skill(read-project-code)
# Before: None
# After: {path: PosixPath(.../.hermes/skills/read-project-code)}

Platforms tested

  • macOS 26.5.1, Python 3.11.15

…ed skill dirs

On Python 3.11 Path.rglob does not follow directory symlinks (the
follow_symlinks parameter was added in 3.12). This makes skill_manage
unable to find skills symlinked from shared directories like
~/.agents/skills/. Replace rglob with os.walk(followlinks=True) to
match iter_skill_index_files which already handles this correctly.
@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verification — symlinked skill directory resolution looks correct

Checked:

  • os.walk(followlinks=True) correctly replaces Path.rglob which silently skips symlinked directories on Python 3.11 (the follow_symlinks parameter was added to rglob in 3.12)
  • The is_excluded_skill_path check is preserved and applied at the same point in the search
  • Directory name comparison (os.path.basename(root) == name) matches the previous skill_md.parent.name == name semantics
  • The Path(root) / "SKILL.md" construction is equivalent to the old skill_md path

One note: os.walk(followlinks=True) can follow circular symlinks. Python's os.walk tracks visited directories via os.stat to prevent infinite loops when followlinks=True, so this is safe in practice. LGTM.

@alt-glitch alt-glitch added type/bug Something isn't working tool/skills Skills system (list, view, manage) P3 Low — cosmetic, nice to have duplicate This issue or pull request already exists labels Jun 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #35244 — same fix (switch _find_skill from Path.rglob to os.walk(followlinks=True) so skill_manage sees symlinked skill dirs, matching iter_skill_index_files). #35244 is the earlier open PR; tracking lineage there. Both address #35184.

@EulaEzio

EulaEzio commented Jun 18, 2026

Copy link
Copy Markdown

This fix works for my setup. I have a skill installed via a category-level symlink (~/.hermes/skills/<category>/<skill-name> → external source), which skill_manage couldn't find before (returned "not found in active profile"), while skills_list and skill_view worked correctly.

I've run another test: using iter_skill_index_files() from agent.skill_utils instead of inline os.walk(followlinks=True):

from agent.skill_utils import iter_skill_index_files
for skill_md in iter_skill_index_files(skills_dir, "SKILL.md"):
    if skill_md.parent.name == name:
        return {"path": skill_md.parent}

This centralizes the exclusion logic (EXCLUDED_SKILL_DIRS, SKILL_SUPPORT_DIRS) and guarantees behavioral consistency with skills_list, skill_view, and prompt_builder which already use this function.

Tested locally — works.

@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 identifying a real inconsistency: current main still uses Path.rglob("SKILL.md") in tools/skill_manager_tool.py:597-601, while the shared index scanner follows symlinked directories in agent/skill_utils.py:796-807.

Problems

  • The added inline walk at tools/skill_manager_tool.py:302 duplicates the shared scanner and does not prune excluded/support directories before descent as iter_skill_index_files() does at agent/skill_utils.py:797-803.
  • _find_skill_in_other_profiles still uses rglob at tools/skill_manager_tool.py:661-666, so cross-profile not-found diagnostics remain inconsistent for symlinked skills.
  • Please add a tests/tools/test_skill_manager_tool.py regression test covering direct and category-level skill-directory symlinks.

Suggested changes

  • Reuse iter_skill_index_files(skills_dir, "SKILL.md") in both discovery paths, preserving the existing parent-name comparison.

Automated hermes-sweeper review.

Comment thread tools/skill_manager_tool.py Outdated
if os.path.basename(root) == name:
skill_md_path = Path(root) / "SKILL.md"
if is_excluded_skill_path(skill_md_path):
continue

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.

Please reuse iter_skill_index_files(skills_dir, "SKILL.md") here. It already follows symlinks and prunes excluded/support directories before descent (agent/skill_utils.py:796-807); this inline walk duplicates that logic without the pruning behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review. I updated both _find_skill and _find_skill_in_other_profiles to reuse iter_skill_index_files(skills_dir, "SKILL.md"), so symlink traversal and excluded-directory pruning stay centralized. I also added regression coverage for direct and category-level symlinked skill directories, including the cross-profile lookup. tests/tools/test_skill_manager_tool.py passes all 93 tests. The changes are in commit 7504ed8469.

@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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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