Skip to content

fix(skills): support editing symlinked Skills - #83808

Open
kimyxx wants to merge 1 commit into
NousResearch:mainfrom
kimyxx:fix/skill-editor-symlink-discovery
Open

fix(skills): support editing symlinked Skills#83808
kimyxx wants to merge 1 commit into
NousResearch:mainfrom
kimyxx:fix/skill-editor-symlink-discovery

Conversation

@kimyxx

@kimyxx kimyxx commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix Skill editor lookup for skills exposed through directory symlinks.
  • Reuse the existing Skill index iterator so editor lookup follows the same discovery rules as Skill listing and viewing.
  • Add a regression test covering edits to a symlinked Skill directory.

Root cause

CC Switch imports Skills into Hermes through directory symlinks. Skill listing and viewing already use iter_skill_index_files(), which follows those links, while the editor used Path.rglob("SKILL.md"), which did not. The Skill appeared in the UI but the editor returned Skill not found.

Test plan

  • uv run --extra dev pytest -q tests/tools/test_skill_manager_tool.py tests/tools/test_skills_tool.py tests/tools/test_skills_tool_discovery_cache.py tests/tools/test_skills_tool_profile_scope.py tests/hermes_cli/test_web_server_skill_editor.py
  • scripts/run_tests.sh tests/tools/test_skill_manager_tool.py -q
  • uv run --extra dev ruff check tools/skill_manager_tool.py tests/tools/test_skill_manager_tool.py
  • Manually verified the running Desktop Skill content endpoint returns HTTP 200 for a symlinked Skill.

All Skill-related tests passed locally. This PR is intentionally limited to the Skill lookup fix and its regression test; the browser CLI PATH fix is kept in a separate PR.

Skill 列表使用会跟随符号链接的统一遍历器,而编辑查找仍使用不跟随目录符号链接的 Path.rglob,导致 CC Switch 导入的 Skill 无法编辑。统一编辑查找逻辑并增加回归测试。
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) duplicate This issue or pull request already exists labels Aug 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #35244: it makes the same skill-manager lookup change to follow directory symlinks through iter_skill_index_files(). #35244 was opened first.

@monerostar monerostar 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.

Ubuntu 26.04 on linux-5800x (kernel 7.0.0-28-generic, Python 3.11.15). Directory symlinks work here.

Live matrix for editing a skill whose skills/ entry is a directory symlink:

  • origin/main + the new test only: TestFindSkill::test_edit_follows_directory_symlink -> FAILED (result["success"] is False)
  • PR tip e247177: same test -> 1 passed
  • Full tests/tools/test_skill_manager_tool.py on PR -> 48 passed

Root cause matches the diff: main _find_skill still uses skills_dir.rglob("SKILL.md"), which skips symlink dirs. PR switches to iter_skill_index_files (already on main) so the target SKILL.md is found and updated.

Looks good. Useful Linux fix, small scope.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(skills): support editing symlinked Skills — the cc-switch use case (shared skills living behind a directory symlink) is legitimate and the change to iter_skill_index_files is minimal. Observations:

  1. Symlink targets can escape the skills namespace: following directory symlinks means _find_skill can now resolve a skill whose SKILL.md lives outside the skills roots (a symlink inside ~/.hermes/skills/ pointing at an arbitrary host dir). is_excluded_skill_path still applies per-file, but consider bounding traversal — e.g., resolve the target and verify containment under the configured roots, or document that symlinked skill dirs are trusted content. The test's target is a sibling tmp dir; the untrusted-link case isn't covered.

  2. Destructive operations on symlinked skills: _find_skill returns the resolved {"path": ...} used by the whole skill_manage surface. If delete/archive removes the parent directory of the found SKILL.md (as it does for regular skills), deleting a symlinked skill would delete the target's content rather than just the link — a much bigger blast radius than the user expects. Worth verifying delete/skill_manage/curator-archive paths treat symlinked skills as link-only (or resolve intentionally), and adding a delete-symlinked-skill test.

  3. Since the triage already notes this matches fix(skill_manager): follow symlinks when discovering skills (#35184) #35244's intent, consider documenting the supported shape (shared skill repos symlinked into the skills dir) in the skill-authoring docs so following symlinks becomes a stated convention rather than an rglob→iter_skill_index_files side effect.

  4. Test coverage: only the edit path is tested; find/list/create/delete on symlinked skills are untested.

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 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.

4 participants