Skip to content

fix: classify builtin skills using bundled path override#6017

Closed
iacker wants to merge 3 commits into
NousResearch:mainfrom
iacker:fix/issue-5433-builtin-skill-classification
Closed

fix: classify builtin skills using bundled path override#6017
iacker wants to merge 3 commits into
NousResearch:mainfrom
iacker:fix/issue-5433-builtin-skill-classification

Conversation

@iacker

@iacker iacker commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • fix hermes skills list builtin detection to use the bundled skills directory resolved by tools.skills_sync._get_bundled_dir()
  • preserve correct builtin classification when Nix or other packaging layouts override the bundled skills path
  • keep the existing manifest-based classification intact while making the directory-name fallback consistent with sync logic

Testing

  • source venv/bin/activate && python -m pytest tests/hermes_cli/test_skills_hub.py -q
  • source venv/bin/activate && python -m pytest tests/ -q (fails in this worktree due to many pre-existing suite issues, notably OSError: [Errno 24] Too many open files, unrelated to this patch)

Closes #5433

This was referenced Apr 21, 2026
Closed
Closed
@teknium1

teknium1 commented Apr 27, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution, @iacker! The root bug this PR fixes has already been resolved independently on main.

Automated hermes-sweeper review

  • Commit b87e0f59c — fix(skills): read name from SKILL.md frontmatter in skills_sync — added _read_skill_name() to tools/skills_sync.py and wired it into _discover_bundled_skills() (line 147), so manifest keys now match frontmatter names exactly as this PR intended. It closed the related issue Bug: skills_sync.py uses directory name instead of SKILL.md name field, causing builtin skills misclassified as local #6835.
  • The fix shipped in v2026.4.13 (also present in v2026.4.16, v2026.4.23).
  • _get_bundled_dir() in tools/skills_sync.py (lines 40–46) already respects the HERMES_BUNDLED_SKILLS env var for Nix packaging layouts, covering the secondary concern in this PR.

Closing as implemented on main. If you see any remaining gap (e.g. the Nix flake.nix change or doc update), feel free to open a focused follow-up PR for just those pieces.

@teknium1 teknium1 closed this Apr 27, 2026
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) area/nix Nix flake, NixOS module, container packaging labels Apr 30, 2026
@alt-glitch

alt-glitch commented Apr 30, 2026

Copy link
Copy Markdown
Collaborator

Related to closed attempts #5966, #5543. Fixes #5433.

@alt-glitch alt-glitch mentioned this pull request Apr 30, 2026
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

area/nix Nix flake, NixOS module, container packaging P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: hermes skills list uses frontmatter name while bundled manifest uses directory name, causing builtin skills to appear as local

3 participants

@iacker @teknium1 @alt-glitch