fix(skill_manager): follow symlinks when discovering skills (#35184) - #35244
fix(skill_manager): follow symlinks when discovering skills (#35184)#35244Linux2010 wants to merge 3 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approve
👍 Looks Good
- Correctly replaces
rglob("SKILL.md")withiter_skill_index_files(skills_dir, "SKILL.md")which usesos.walk(followlinks=True)— fixes the inconsistent behavior where skills appeared in the system prompt index but were invisible toskill_manage - Minimal change: only 2 call sites modified in
_find_skill()and_find_skill_in_other_profiles(), 4 lines each (+1 import, -1 rglob, +1 iter_skill_index_files) - Reuses existing
iter_skill_index_files()which already handlesEXCLUDED_SKILL_DIRSfiltering — no new exclusion logic needed - Tests properly mock the skills directory and verify symlinked (including nested) skills are discovered
- Uses
tmp_pathwithsymlink_tofor clean, isolated test setup
✅ Verified
- 45 lines of tests covering both flat and nested symlink scenarios
- No behavioral change for non-symlinked skills
- Consistent with
iter_skill_index_files()already used inprompt_builder.py
Reviewed by Hermes Agent
Automated Triage: Competing PRs DetectedThree PRs are independently fixing the same issue (symlink-discovery in skills, #35184):
They all modify |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for aligning skill_manage discovery with the shared skill indexer. The current-main premise is valid: tools/skill_manager_tool.py:597 and :661 still use rglob("SKILL.md"), while agent/skill_utils.py:796 uses os.walk(..., followlinks=True).
Problems
- The new tests create symlinks without a platform-availability guard. Existing tests use
pytest.skipafterOSError/NotImplementedError(tests/agent/test_skill_utils.py:376-378); use the same pattern so restricted Windows environments do not fail the suite. - Both discovery functions are changed, but the added tests only call
_find_skill. Please cover_find_skill_in_other_profiles()as well; its current-mainrglobsite istools/skill_manager_tool.py:661.
Suggested changes
- Add a cross-profile symlink regression test and platform-safe symlink setup.
Automated hermes-sweeper review.
| real_category = tmp_path / "real_category" / "my-skill" | ||
| real_category.mkdir(parents=True) | ||
| (real_category / "SKILL.md").write_text(VALID_SKILL_CONTENT) | ||
|
|
There was a problem hiding this comment.
Please catch OSError and NotImplementedError here and pytest.skip when symlinks cannot be created. Existing symlink coverage uses that guard in tests/agent/test_skill_utils.py:376-378, which keeps restricted Windows environments from failing the suite.
…arch#35184) _find_skill() and _find_skill_in_other_profiles() used Path.rglob() which does NOT follow symbolic links. This caused skills under symlinked category directories to be invisible to skill_manage, even though they appeared in the system prompt index (prompt_builder.py uses os.walk(followlinks=True) via iter_skill_index_files). Fix: replace rglob("SKILL.md") with iter_skill_index_files(skills_dir, "SKILL.md") from agent.skill_utils, which already uses os.walk(followlinks=True) and handles EXCLUDED_SKILL_DIRS filtering. This aligns skill discovery in skill_manager_tool.py with how prompt_builder.py already discovers skills for the system prompt. Test: add TestFindSkillSymlinks class with two tests: - test_symlinked_skill_dir_is_found — verifies a skill under a symlinked category directory is discovered - test_nested_symlinked_skill_is_found — verifies deeply nested skills under symlinked dirs are discovered
- Add linux2010@github.com to AUTHOR_MAP for check-attribution - Fix test_symlinked_skill_dir_is_found: iter_skill_index_files returns path through symlink, not the target path Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…e test Address sweeper review feedback on NousResearch#35244: - Wrap symlink_to() calls in try/except OSError → pytest.skip for restricted Windows environments (matching pattern in test_skill_utils.py) - Add test_symlinked_skill_found_via_other_profile to cover _find_skill_in_other_profiles() symlink path Refs NousResearch#35244
c0f387a to
1af96b8
Compare
What broke
_find_skill()and_find_skill_in_other_profiles()intools/skill_manager_tool.pyusedPath.rglob("SKILL.md")to discover skills. Python'spathlib.rglob()does NOT follow symbolic links into directories. This causedskill_manageto fail with "Skill not found in active profile" for any skill located under a symlinked category directory.Meanwhile,
prompt_builder.pyusesiter_skill_index_files()→os.walk(followlinks=True), which DOES follow symlinks. This inconsistency meant skills appeared in the system prompt index but could not be managed viaskill_manage.Root cause
rglob()does not follow symlinks by design. The two discovery paths (skill_manager_tool.pyvsprompt_builder.py) used different mechanisms, and therglobpath missed symlinked skills.Why this fix is minimal
Replaces
rglob("SKILL.md")withiter_skill_index_files(skills_dir, "SKILL.md")fromagent.skill_utils, which already usesos.walk(followlinks=True)and handlesEXCLUDED_SKILL_DIRSfiltering. Only two call sites changed; no new dependencies or behavioral changes.What I tested
Added
TestFindSkillSymlinksclass with two tests:test_symlinked_skill_dir_is_found— verifies a skill under a symlinked category directory is discoveredtest_nested_symlinked_skill_is_found— verifies deeply nested skills under symlinked dirs are discoveredWhat I intentionally did not change