fix(skills): resolve symlinks before relative_to in skill install - #53541
fix(skills): resolve symlinks before relative_to in skill install#53541AlexFucuson9 wants to merge 1 commit into
Conversation
Path.relative_to() does a lexical prefix check that fails when either path contains symlink components. When ~/.hermes or its skills/ dir is a symlink (common on systems where home dir is symlinked), the install succeeds but printing the relative path raises ValueError. Fix by resolving both paths before calling relative_to. Fixes NousResearch#53403
Duplicate of #49885 — this is the identical one-line |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Resolves symlinks before calling relative_to in skill install to prevent ValueError when the install path is a symlink pointing outside the skills directory.
Looks Good
.resolve()on both sides ensures the relative path computation works correctly- Minimal one-line change, correct fix
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused fix. The underlying backend failure still exists on current main: _resolve_lock_install_path() returns a resolved directory at tools/skills_hub.py:288, but lock recording compares it with unresolved _skills_dir() at tools/skills_hub.py:3644.
Problems
- The same bug class remains in the successful install display at
hermes_cli/skills_hub.py:730-731; after this PR's backend change, a symlinked root can still fail while printingInstalled:. - This branch predates the dynamic-path refactor in
10e60060d; current main uses_skills_dir()rather than the internalSKILLS_DIRexpression changed by this diff. - No regression test covers a symlinked skills root and both affected outputs.
Suggested changes
- Port the backend fix to
_skills_dir().resolve(), fix the CLI display site with the same resolved-root calculation, and add one regression covering the lock entry plus the success output.
Automated hermes-sweeper review.
| scan_verdict=scan_result.verdict, | ||
| skill_hash=content_hash(install_dir), | ||
| install_path=str(install_dir.relative_to(SKILLS_DIR)), | ||
| install_path=str(install_dir.resolve().relative_to(SKILLS_DIR.resolve())), |
There was a problem hiding this comment.
This addresses lock recording, but current main also computes an unresolved relative path in the successful CLI display at hermes_cli/skills_hub.py:731. Please fix that sibling site and cover both paths with one symlink-root regression.
Bug
After a skill is successfully installed,
do_installtries to print the install location relative toSKILLS_DIR. It callsPath.relative_to()on raw (unresolved) paths.When either
SKILLS_DIRor the install directory contains a symlink component (common when~/.hermesor itsskills/dir is itself a symlink), the lexical prefix check inrelative_tofails and raisesValueError, aborting an otherwise-successful install.Fix
Resolve both paths before calling
relative_to():1 line change.
Fixes #53403