Skip to content

fix(skills): replace symlink install destinations - #21739

Closed
Bartok9 wants to merge 1 commit into
NousResearch:mainfrom
Bartok9:fix/21735-skills-install-symlink
Closed

fix(skills): replace symlink install destinations#21739
Bartok9 wants to merge 1 commit into
NousResearch:mainfrom
Bartok9:fix/21735-skills-install-symlink

Conversation

@Bartok9

@Bartok9 Bartok9 commented May 8, 2026

Copy link
Copy Markdown
Contributor

Problem

hermes skills install --force crashes when the existing install destination is a symbolic link because the installer tries to pass that path to shutil.rmtree(), which refuses to operate on symlinks.

Closes #21735

Root cause

install_from_quarantine() only checked install_dir.exists() before cleanup, then always called shutil.rmtree(install_dir). For directory symlinks, that raises OSError instead of replacing the previous install path.

Fix

Add a small cleanup helper that unlinks symlinks and files, while preserving the existing recursive removal behavior for real directories. The regression test installs over a symlinked skill path and verifies the symlink target is left intact.

Testing

  • uv run --with ruff --with pytest --with pytest-xdist --python 3.11 ruff check tools/skills_hub.py tests/tools/test_skills_hub.py: clean
  • uv run --with pytest --with pytest-xdist --python 3.11 pytest tests/tools/test_skills_hub.py -q: 107 passed

Made with Cursor

Closes NousResearch#21735

Co-authored-by: Cursor <cursoragent@cursor.com>
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard tool/skills Skills system (list, view, manage) labels May 8, 2026
@Bartok9

Bartok9 commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

CI status note:

  • The ruff + ty diff failure is the known fork-token comment-posting failure on fork PRs.
  • Local ruff is clean for this PR's changed Python files: uv run --with ruff --python 3.11 ruff check tools/skills_hub.py tests/tools/test_skills_hub.py.

This PR does not introduce a lint failure.

@Bartok9

Bartok9 commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

The test CI failure is pre-existing on main — verified at main SHA 1997b3ba. Not introduced by this PR.

@Bartok9

Bartok9 commented May 9, 2026

Copy link
Copy Markdown
Contributor Author

The test CI failure is pre-existing on main — verified at main SHA 524cbabd. Not introduced by this PR.

@Bartok9

Bartok9 commented May 12, 2026

Copy link
Copy Markdown
Contributor Author

CI note: The failing checks (test, e2e, Windows footguns) are pre-existing failures on main at 3c23b15f815e — they fail without this PR's changes. Running pytest locally against main directly reproduces the same 12 failures. None of the failing tests touch the code changed in this PR. This PR is CI-clean with respect to its own scope.

@Bartok9

Bartok9 commented May 27, 2026

Copy link
Copy Markdown
Contributor Author

Closing to stay under the repo's 10-PR contributor limit. Will resubmit with updated code if the issue is still open in main.

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 P2 Medium — degraded but workaround exists 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

2 participants