Skip to content

fix(skills): replace force-install symlink destinations - #23251

Closed
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/21735-skill-install-symlink
Closed

LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/21735-skill-install-symlink

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

What does this PR do?

This fixes #21735 with a narrow bugfix that keeps the affected path aligned with the current Hermes behavior without widening the change surface.

Related Issue

Fixes #21735

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • teach install_from_quarantine() to replace an existing symlink destination without deleting the real target behind it
  • add regression coverage for --force installs that land on a symlinked skill path
  • Files: tools/skills_hub.py, tests/tools/test_skills_hub.py

How to Test

  1. Run uv run --frozen pytest -q -o addopts='' tests/tools/test_skills_hub.py -k force_install_replaces_symlink_destination
  2. Run uv run --frozen ruff check tools/skills_hub.py tests/tools/test_skills_hub.py
  3. Confirm the affected workflow in hermes skills install crashes with OSError when install destination is a symlink #21735 now follows the expected behavior.

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

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

For New Skills

Screenshots / Logs

Targeted skill-install regression test and Ruff checks passed locally on macOS.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard tool/skills Skills system (list, view, manage) duplicate This issue or pull request already exists labels May 10, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

Duplicate of #21739 — same symlink replacement fix for #21735.

@teknium1

Copy link
Copy Markdown
Collaborator

Automated hermes-sweeper review: the #21735 crash path this duplicate PR targets is already fixed on current main.

Evidence:

  • tools/skills_hub.py:154 defines symlink/junction redirects as unsafe install-path redirects.
  • tools/skills_hub.py:180-184 walks the destination path component-by-component and raises ValueError before a symlinked destination can reach cleanup.
  • tools/skills_hub.py:3337-3343 routes install_from_quarantine() through that resolver before the existing shutil.rmtree(install_dir) call, so the reported OSError: Cannot call rmtree on a symbolic link path is no longer reached.
  • hermes_cli/skills_hub.py:681-689 catches that ValueError and prints a user-facing Installation blocked: ... message instead of bubbling a traceback.
  • The fix is in 3b9b9a7ad7b24cba3683b63db2e34a41668c5d29, which is contained in v2026.5.28.

Note: current main resolves this conservatively by blocking symlinked skill install destinations rather than unlinking/replacing them. That still satisfies #21735's expected behavior, which allowed either a successful install or a clear non-traceback error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main 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.

hermes skills install crashes with OSError when install destination is a symlink

3 participants