Skip to content

fix(skill_manager): use patchable SKILLS_DIR in _find_skill - #5317

Open
iRonin wants to merge 2 commits into
NousResearch:mainfrom
iRonin:fix/skill-manager-find-skill-v2
Open

fix(skill_manager): use patchable SKILLS_DIR in _find_skill#5317
iRonin wants to merge 2 commits into
NousResearch:mainfrom
iRonin:fix/skill-manager-find-skill-v2

Conversation

@iRonin

@iRonin iRonin commented Apr 5, 2026

Copy link
Copy Markdown
Contributor

Problem

_find_skill() in tools/skill_manager_tool.py called get_all_skills_dirs() which always returns the real ~/.hermes/skills/ as its first entry — ignoring any unittest.mock.patch on the module-level SKILLS_DIR constant.

Tests patch SKILLS_DIR to a tmp_path, so _create_skill wrote skills there but _find_skill never found them. Every skill operation that needed to locate an existing skill (edit, patch, delete, write_file, remove_file) silently failed with "skill not found".

Fix

Replace get_all_skills_dirs()[0] with the module-level SKILLS_DIR (patchable), keeping external skill dirs from config at indices 1+. In production behaviour is identical.

Tests

All 51 test_skill_manager_tool tests pass locally.

_find_skill() was calling get_all_skills_dirs() which always
returns the real ~/.hermes/skills/ as the first entry, ignoring
any unittest.mock.patch on the module-level SKILLS_DIR constant.

Tests patch SKILLS_DIR to a tmp_path, so _create_skill wrote
skills there but _find_skill never found them — causing every
subsequent edit/patch/delete/write_file/remove_file test to fail.

Fix: build search_dirs by replacing the first entry with the
module-level SKILLS_DIR (patchable) and keeping external dirs
from config (indices 1+) unchanged. In production the two are
identical, so no behaviour change.
@davidgovea

Copy link
Copy Markdown

I also landed on a similar fix while working with the test suite. Looks good!

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) labels May 1, 2026

@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 identifying the lookup/write-root mismatch. The premise still holds on current main: _resolve_skill_dir() writes through the patch-aware _skills_dir() (tools/skill_manager_tool.py:578-582), while _find_skill() gets its primary root from get_all_skills_dirs() (tools/skill_manager_tool.py:593-601), whose local entry comes from hermes_constants.get_skills_dir() (agent/skill_utils.py:503-510).

Problems

  • The PR's search_dirs = [SKILLS_DIR] + ... would bypass the call-time profile resolution added by c6a3d412d and implemented in tools/skill_manager_tool.py:156-168. In long-lived multi-profile runtimes, that would restore the import-time-path problem.
  • The new test file duplicates existing local/external-root fixtures in tests/tools/test_skill_manager_tool.py:26-32 and :740-747.

Suggested changes

  • Use [_skills_dir()] + list(all_dirs[1:]) for the primary search root.
  • Keep a focused regression in the existing skill-manager test module that proves a patched manager-local root wins even when get_all_skills_dirs() reports a different primary root.

Automated hermes-sweeper review.

# 1+) still come from the config via get_all_skills_dirs().
all_dirs = get_all_skills_dirs()
search_dirs = [SKILLS_DIR] + list(all_dirs[1:])
for skills_dir in search_dirs:

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.

Use _skills_dir() here rather than SKILLS_DIR. Current main resolves the local skills root at call time to support profile-scoped long-lived runtimes (tools/skill_manager_tool.py:156-168); the static constant would reintroduce the import-time-profile bug while fixing the mock seam.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/skills Skills system (list, view, manage) type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants