Skip to content

Fix _find_skill: use iter_skill_index_files to follow symlinks - #60960

Open
wildnewton wants to merge 5 commits into
NousResearch:mainfrom
wildnewton:fix/find-skill-follow-symlinks
Open

Fix _find_skill: use iter_skill_index_files to follow symlinks#60960
wildnewton wants to merge 5 commits into
NousResearch:mainfrom
wildnewton:fix/find-skill-follow-symlinks

Conversation

@wildnewton

Copy link
Copy Markdown

Problem

_find_skill() used Path.rglob("SKILL.md") which does NOT follow symlinks in Python 3.11. This meant any skill directory reached through a symlink was invisible to skill_manage, even though skill_view could load it (via iter_skill_index_filesos.walk(followlinks=True)).

Root Cause

_find_skillrglob() (no follow_symlinks) ≠ skill_viewiter_skill_index_filesos.walk(followlinks=True)

When a skill is symlinked (e.g., from a project directory into the profile's skills dir), skill_view can load it but skill_manage reports "not found." This asymmetry caused agents to fall back to action='create', which produced nested duplicate skills that broke cron scheduler resolution.

Fix

Replace rglob("SKILL.md") with iter_skill_index_files(skills_dir, "SKILL.md") — the same function skill_view uses.

TDD

  • 🔴 RED: test that _find_skill discovers a symlinked skill directory
  • 🟢 GREEN: 2-line change in _find_skill + import

Related

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

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #35244 (earliest open, canonical) which fixes #35184 with the identical mechanism: replace Path.rglob("SKILL.md") (doesn't follow symlinks) with iter_skill_index_files (the same os.walk(followlinks=True) helper skill_view uses). See also #56423 (same swap at both _find_skill call sites) and #35299 (competing rglob_follow() helper approach).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for tracing the active-profile discovery mismatch. The current main path still uses rglob() at tools/skill_manager_tool.py:597, while iter_skill_index_files() follows links at agent/skill_utils.py:796, so the proposed direction is sound.

Problems

  • The sibling cross-profile lookup still uses rglob() at tools/skill_manager_tool.py:661; it is consumed by _skill_not_found_error() at tools/skill_manager_tool.py:683. Apply the same iterator there and cover that path.
  • The added test creates a symlink without handling unavailable symlink support. Existing tests in this file use a graceful OSError skip pattern at tests/tools/test_skill_manager_tool.py:357-360 and 472-475.

Suggested changes

  • Use iter_skill_index_files() in both finders and add a cross-profile regression.
  • Make the new symlink test skip when link creation is unsupported.

Automated hermes-sweeper review.

@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:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 10, 2026
William Niu and others added 3 commits July 12, 2026 18:17
fix: relay codex exec --json progress as interim messages in delegate_task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets duplicate This issue or pull request already exists 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-platform-windows Sweeper risk: may break or behave differently on native Windows 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.

3 participants