Skip to content

fix(skills): follow symlinks during skill discovery (#35184) - #35213

Open
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/35184-review-rev1
Open

fix(skills): follow symlinks during skill discovery (#35184)#35213
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/35184-review-rev1

Conversation

@Tranquil-Flow

Copy link
Copy Markdown
Contributor

What does this PR do?

Skill discovery (_find_skill in tools/skill_manager_tool.py) uses
Path.rglob() which does not follow symlinks into directories. Skills
stored under symlinked category directories were invisible to the agent.

Fix: Replace Path.rglob() with os.walk(followlinks=True) via the
existing iter_skill_index_files() helper in the same module.

Related Issue

Fixes #35184

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • tools/skill_manager_tool.py: Replace Path.rglob() call with
    iter_skill_index_files() (which uses os.walk(followlinks=True))
    for symlink-aware directory traversal.
  • tests/tools/test_skill_symlink_discovery.py: New — regression
    test that creates a symlinked category directory and verifies skills
    under it are discovered.

How to Test

  1. Run python3 -m pytest tests/tools/test_skill_symlink_discovery.py -v
  2. Verify skills in symlinked directories are found

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)

@alt-glitch alt-glitch added type/bug Something isn't working tool/skills Skills system (list, view, manage) P2 Medium — degraded but workaround exists labels May 30, 2026
@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 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 targeting a confirmed residual skill-discovery path. Current main still uses rglob("SKILL.md") in tools/skill_manager_tool.py:597 and tools/skill_manager_tool.py:661, while agent/skill_utils.py:796 already provides the intended symlink-following traversal.

Problems

  • tests/tools/test_skill_symlink_discovery.py:42 calls os.symlink without handling unavailable symlink privileges. Existing tests skip cleanly on OSError/NotImplementedError in tests/tools/test_skills_tool.py:47-55.
  • iter_skill_index_files() follows links at agent/skill_utils.py:796 but has no visible cycle guard. A cyclic directory link could make the newly migrated management lookups non-terminating.
  • The same direct lookup pattern remains in tools/skill_usage.py:841 and tools/skill_usage.py:858; assess whether those curator/usage paths should share the iterator too.

Suggested changes

  • Make the regression test skip when symlink creation is unavailable.
  • Add cycle protection in the shared iterator and cover it with a regression test before extending its use.

Automated hermes-sweeper review.


# Symlink the external category into skills dir
symlink = skills_dir / "linked-cat"
os.symlink(symlinked_cat, symlink, target_is_directory=True)

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 handle OSError and NotImplementedError here and skip when symlinks are unavailable. The established helper in tests/tools/test_skills_tool.py:47-55 keeps these tests portable to restricted Windows environments.

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

Labels

P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows 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.

skill_manager_tool._find_skill uses Path.rglob() which does not follow symlinks, making symlinked skills invisible to skill_manage

4 participants