Repository navigation
fix(skills): resolve skill_manage targets through the symlink-following index walk - #17
Merged
Merged
Conversation
…ng index walk
`_iter_skill_dirs` walked the skills root with `Path.rglob("SKILL.md")`. rglob
does not descend into symlinked directories, so a profile that installs each
skill as a per-skill symlink into a shared tree had none of its skills visible
to the write path, while the read path (`skill_view` / `skills_list`, via
`iter_skill_index_files`) listed them all. Every write in such a profile failed
with "Skill '<name>' not found in active profile '<profile>'".
Route the walk through `iter_skill_index_files`, the same iterator the read path
uses, and accept either the lexical or the resolved form of the skills root when
deciding whether a hit belongs to the local root. Both finders (active profile,
and the other-profile search) share `_iter_skill_dirs`, so one change fixes both.
Regression tests cover a per-skill symlink, a symlinked category directory,
`patch` / `write_file` / `remove_file`, and the cross-profile lookup.
૮ >ﻌ< ა ci reviewrunning on c07fbe5 — fix(skills): resolve skill_manage targets through the symlin Still running 2 jobs:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Concern
skill_managerefuses every write in a profile that installs each skill as aper-skill symlink (the curated lanes: yoyodine-coder, yoyodine-financially,
yoyodine-research), while the read path resolves the very same skill fine.
Root cause (measured, not inferred)
tools/skill_manager_tool.py:_iter_skill_dirswalked the skills root withPath.rglob("SKILL.md").rglob` does not descend into symlinked directories,and in a curated lane each skill is a symlink into the shared tree:
so none of that lane's skills were reachable by the write path. Profiles whose
whole skills root is a symlink (
defaultat~/.hermes/skills,yoyodine-majordomo->~/.hermes/skills) work, becauserglobtraverses theroot it is handed and everything inside is a real directory. That asymmetry is
exactly why the error message names
defaultandyoyodine-majordomoas theprofiles that "have" the skill.
Measured on a live install (2026-09-15):
Fix
Route the walk through
iter_skill_index_files, the same symlink-following(
followlinks=True, support/excluded dirs pruned) iterator the read path alreadyuses, and accept either the lexical or the resolved form of the skills root when
deciding whether a hit belongs to the local root. Both the active-profile finder
and the other-profile search share
_iter_skill_dirs, so this one change fixesboth, and the cross-profile guard behaviour is unchanged.
Tests
New
TestSymlinkedSkillDirsintests/tools/test_skill_manager_tool.pycovers aper-skill symlink, a symlinked category directory,
patch/write_file/remove_file, the dispatcher patch path, and a symlinked whole skills root.Red -> green, production file reverted to the pre-fix commit with the tests left
at this branch (this branch's parent is the pre-fix tip, so the revert is exact):
No behaviour change elsewhere. Same-tree A/B on the skill-management surface
(
tests/tools tests/agent -k skill: 972 collected here vs 966 on the base tree,the delta being these tests): identical failure sets (4 pre-existing failures
in both trees, 967 passed here vs 961 on base). Focused guard-file A/B
(
tests/tools/test_cross_profile_guard.py+tests/tools/test_skill_manager_tool.py):79 passed here vs 73 on base, zero failures either side.
Scope
One concern: the
skill_managewrite path. The samerglob("SKILL.md")patternsurvives in ~14 other modules and several are user-visible in a curated lane
(
hermes_cli/profiles.pyper-profile skill count,hermes_cli/profile_describer.pysummary,
hermes_cli/web_routers/profiles.pyinstalled-skill names,tui_gateway/methods_profiles.pylisting,agent/skill_usage.py,agent/curator_backup.py,agent/learning_graph.py,gateway/run.pyhints) -each needs its own semantics check and belongs in its own change, not here. It
is flagged on the card, not bundled into this diff.
Upstream
No upstream PR from this branch:
NousResearch/hermes-agentissue NousResearch#35184describes this exact defect and sibling PR NousResearch#67748 (
fix/repo-backed-skill-resolution,open) makes the same substitution, so upstream is a HOLD per the fork rule.