Skip to content

fix(skills): create new skills in external skill dir - #23562

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/21810-skill-create-external-dir
Open

fix(skills): create new skills in external skill dir#23562
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/21810-skill-create-external-dir

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes skill_manage(action='create') so new skills are created in the configured external skill directory when one is available, instead of always forcing creation into the local bundled skills path.

Related Issue

Fixes #21810

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • Updated tools/skill_manager_tool.py to consult external skill roots before falling back to SKILLS_DIR
  • Added regression coverage in tests/tools/test_skill_manager_tool.py

How to Test

  1. Run uv run --frozen pytest -q -o addopts='' tests/tools/test_skill_manager_tool.py
  2. Run uv run --frozen ruff check tools/skill_manager_tool.py tests/tools/test_skill_manager_tool.py
  3. Confirm both commands pass

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.x

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

  • uv run --frozen pytest -q -o addopts='' tests/tools/test_skill_manager_tool.py -> 86 passed

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) labels May 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #22236 — same fix routing skill_manage(action='create') to skills.external_dirs. Both fix #21810.

@alt-glitch alt-glitch added the duplicate This issue or pull request already exists label May 11, 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 addressing the inconsistency between external skill discovery and creation.

Problems

  • Current main explicitly documents the opposite contract: cli-config.yaml.example:668-672 says external skill directories are read-only and creation always writes locally. tests/tools/test_skill_manager_tool.py:866-879 enforces the same behavior. This needs a maintainer policy decision before the change can be salvaged.
  • The added fallback at tools/skill_manager_tool.py:279 is SKILLS_DIR. Current main commit c6a3d412d462 changed creation-path resolution to _skills_dir() so long-lived, profile-scoped runtimes do not use the import-time profile path. Any updated implementation must retain that call-time fallback.

Suggested changes

  • If maintainers choose to make external directories authoring roots, update the documented contract and regression test, preserve _skills_dir() for the local fallback, and cover category plus multiple-external-root selection.

Automated hermes-sweeper review.

external_dirs = get_external_skills_dirs()
except Exception:
external_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.

On current main, do not fall back to module-level SKILLS_DIR: commit c6a3d412d462 introduced _skills_dir() so long-lived profile-scoped runtimes resolve the active profile at call time. Preserve _skills_dir() here when no external root is selected.

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