Skip to content

fix(skills): skill_manage create respects skills.external_dirs - #22236

Closed
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/skill-manage-external-dirs
Closed

fix(skills): skill_manage create respects skills.external_dirs#22236
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/skill-manage-external-dirs

Conversation

@wesleysimplicio

@wesleysimplicio wesleysimplicio commented May 9, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Closes #21810.

Root cause

The detailed rationale from the original PR body is preserved below. This template update keeps the review structure consistent with #29640.

Fix

  • New helper _default_creation_dir() returns the first entry of skills.external_dirs when configured, falling back to SKILLS_DIR.
  • _resolve_skill_dir(name, category) now routes through _default_creation_dir() instead of hardcoding SKILLS_DIR.
  • _create_skill computes the returned path relative to the actual containing root (via the existing _containing_skills_root helper) instead of always relativizing to SKILLS_DIR, which would have raised ValueError once the skill landed outside of it.

Public API of _resolve_skill_dir is unchanged.

Why this shape

This shape mirrors #29640 so reviewers can quickly compare scope, root cause, fix, tests, and related context without having to decode a custom PR description.

Tests

  • Veja a descrição original preservada abaixo para detalhes de validação, testes e notas de verificação.
Original body

Related PRs / issues

Original body

Summary

Closes #21810.

What Changed

  • Standardized this PR body to the current Hermes Turbo template.
  • Preserved the original detailed description below for reference.

Fluxo

A mudança continua seguindo o fluxo original descrito na seção preservada abaixo, sem ampliar o escopo funcional deste PR.

Visão

A padronização melhora a revisão, reduz ruído e evita deriva de formatação entre PRs abertos.

Test Plan

  • Veja a descrição original preservada abaixo para detalhes de validação, testes e notas de verificação.
Original body

Summary

Closes #21810.

skill_manage(action='create') previously hardcoded SKILLS_DIR (~/.hermes/skills/), dropping new skills into the user-local store even when skills.external_dirs was configured as the canonical authoring location (e.g. a checked-out skills monorepo). Other reads (_find_skill, etc.) already honor external dirs via _containing_skills_root/get_all_skills_dirs, so creates were the odd one out.

Changes

  • New helper _default_creation_dir() returns the first entry of skills.external_dirs when configured, falling back to SKILLS_DIR.
  • _resolve_skill_dir(name, category) now routes through _default_creation_dir() instead of hardcoding SKILLS_DIR.
  • _create_skill computes the returned path relative to the actual containing root (via the existing _containing_skills_root helper) instead of always relativizing to SKILLS_DIR, which would have raised ValueError once the skill landed outside of it.

Public API of _resolve_skill_dir is unchanged.

Test plan

  • New TestCreateSkillRespectsExternalDirs class with 4 cases:
    • no external dirs configured → uses SKILLS_DIR (regression guard, unchanged behavior)
    • external dir configured → skill lands under external root, not local
    • external dir + category= → category nesting still applies under external root, returned path is relative
    • multiple external dirs → first entry wins
  • All 90 existing tests/tools/test_skill_manager_tool.py tests still pass.
  • All 144 tests across files importing skill_manager_tool (test_registry, test_skill_improvements, test_skill_size_limits, test_skill_manager_tool) still pass.

Generated by Hermes Turbo


Generated by Hermes Turbo

Closes NousResearch#21810

skill_manage(action='create') previously hardcoded SKILLS_DIR (~/.hermes/skills/),
dropping new skills into the user-local store even when an external dir was
configured as the canonical authoring location.

Add _default_creation_dir() helper that prefers the first entry of
skills.external_dirs when configured, falling back to SKILLS_DIR. Route
_resolve_skill_dir through it, and compute the result 'path' relative to
the actual containing root (not always SKILLS_DIR).
Copilot AI review requested due to automatic review settings May 9, 2026 03:18

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

Pull request overview

This PR fixes skill_manage(action="create") so newly created skills are written to the configured authoring location when skills.external_dirs is set, aligning create behavior with existing read/lookup behavior that already honors external skill roots.

Changes:

  • Add _default_creation_dir() to prefer the first validated skills.external_dirs entry (fallback: local SKILLS_DIR).
  • Route _resolve_skill_dir() through _default_creation_dir() so creates land under the correct root (including category nesting).
  • Make _create_skill return a path relative to the actual containing skills root (instead of always SKILLS_DIR), preventing ValueError for external-root creates.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.

File Description
tools/skill_manager_tool.py Default create destination now respects skills.external_dirs, and returned path is relativized to the real containing skills root.
tests/tools/test_skill_manager_tool.py Adds unit tests ensuring create targets the first external dir when configured and preserves category-relative paths.
.gitignore Ignores .claude/scheduled_tasks.lock.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@wesleysimplicio

Copy link
Copy Markdown
Contributor Author

Closing — competing PR already merged or cleaner alternative exists.

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

skill_manage(action='create') ignores skills.external_dirs, always writes to ~/.hermes/skills/

3 participants