diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index a20d23fcbd82..39a43243aae1 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -1,6 +1,7 @@ """Tests for tools/skill_manager_tool.py — skill creation, editing, and deletion.""" import json +import os from pathlib import Path from unittest.mock import patch @@ -17,6 +18,8 @@ _delete_skill, _write_file, _remove_file, + _is_external_skill, + _copy_on_write_to_local, skill_manage, VALID_NAME_RE, ALLOWED_SUBDIRS, @@ -411,3 +414,256 @@ def test_full_create_via_dispatcher(self, tmp_path): raw = skill_manage(action="create", name="test-skill", content=VALID_SKILL_CONTENT) result = json.loads(raw) assert result["success"] is True + + +# --------------------------------------------------------------------------- +# Copy-on-write for external (shared / external_dirs) skills +# --------------------------------------------------------------------------- + + +def _write_external_skill(root: Path, name: str, description: str = "Ext skill") -> Path: + """Helper: create a skill at root/name/SKILL.md and return the skill dir.""" + d = root / name + d.mkdir(parents=True) + (d / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: {description}\n---\n\n# {name}\n\nStep 1: original step.\n" + ) + return d + + +class TestIsExternalSkill: + def test_local_skill_is_not_external(self, tmp_path): + local = tmp_path / "local-skills" + local.mkdir() + skill = local / "a" + skill.mkdir() + with patch("tools.skill_manager_tool.SKILLS_DIR", local): + assert _is_external_skill(skill) is False + + def test_sibling_skill_is_external(self, tmp_path): + local = tmp_path / "local-skills" + ext = tmp_path / "external" + local.mkdir() + ext.mkdir() + skill = ext / "a" + skill.mkdir() + with patch("tools.skill_manager_tool.SKILLS_DIR", local): + assert _is_external_skill(skill) is True + + +class TestCopyOnWriteHelper: + def test_copies_directory_contents(self, tmp_path): + local = tmp_path / "local" + ext = tmp_path / "external" + local.mkdir() + source = _write_external_skill(ext, "shared-skill") + (source / "references").mkdir() + (source / "references" / "notes.md").write_text("notes") + with patch("tools.skill_manager_tool.SKILLS_DIR", local): + result = _copy_on_write_to_local(source) + assert result == local / "shared-skill" + assert (result / "SKILL.md").read_text() == (source / "SKILL.md").read_text() + assert (result / "references" / "notes.md").read_text() == "notes" + + def test_raises_when_local_already_exists(self, tmp_path): + local = tmp_path / "local" + ext = tmp_path / "external" + (local / "clash").mkdir(parents=True) + source = _write_external_skill(ext, "clash") + with patch("tools.skill_manager_tool.SKILLS_DIR", local): + try: + _copy_on_write_to_local(source) + except RuntimeError as exc: + assert "already exists" in str(exc) + else: + raise AssertionError("expected RuntimeError") + + +class TestEditExternalSkillCopyOnWrite: + """Verify _edit_skill copy-on-writes external skills instead of mutating them in place.""" + + def _setup(self, tmp_path): + local = tmp_path / "hermes" / "skills" + local.mkdir(parents=True) + ext = tmp_path / "shared-skills" + ext.mkdir() + _write_external_skill(ext, "shared-editable", "original desc") + hermes_home = tmp_path / "hermes" + (hermes_home / "config.yaml").write_text( + f"skills:\n external_dirs:\n - {ext}\n" + ) + return local, ext, hermes_home + + def test_edit_external_skill_creates_local_copy(self, tmp_path): + local, ext, hermes_home = self._setup(tmp_path) + new_content = ( + "---\nname: shared-editable\ndescription: edited from profile A\n---\n\n" + "# shared-editable\n\nStep 1: edited step.\n" + ) + with ( + patch("tools.skill_manager_tool.SKILLS_DIR", local), + patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}), + ): + result = _edit_skill("shared-editable", new_content) + # Edit succeeded + assert result["success"] is True + # A note was attached explaining the copy-on-write + assert "note" in result + assert "override" in result["note"].lower() or "profile-local" in result["note"].lower() + # Local copy was created with the new content + local_copy = local / "shared-editable" / "SKILL.md" + assert local_copy.exists() + assert "edited from profile A" in local_copy.read_text() + # External skill is untouched + external_original = ext / "shared-editable" / "SKILL.md" + assert "original desc" in external_original.read_text() + # Returned path points at the local copy, not the external + assert str(local / "shared-editable") == result["path"] + + def test_edit_local_skill_still_writes_in_place(self, tmp_path): + local, ext, hermes_home = self._setup(tmp_path) + with ( + patch("tools.skill_manager_tool.SKILLS_DIR", local), + patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}), + ): + _create_skill("local-only", VALID_SKILL_CONTENT) + result = _edit_skill("local-only", VALID_SKILL_CONTENT_2) + assert result["success"] is True + # No CoW note for local skills + assert "note" not in result + assert "Updated description" in (local / "local-only" / "SKILL.md").read_text() + + +class TestPatchExternalSkillCopyOnWrite: + def _setup(self, tmp_path): + local = tmp_path / "hermes" / "skills" + local.mkdir(parents=True) + ext = tmp_path / "shared-skills" + ext.mkdir() + _write_external_skill(ext, "shared-patchable") + hermes_home = tmp_path / "hermes" + (hermes_home / "config.yaml").write_text( + f"skills:\n external_dirs:\n - {ext}\n" + ) + return local, ext, hermes_home + + def test_patch_external_skill_creates_local_copy(self, tmp_path): + local, ext, hermes_home = self._setup(tmp_path) + with ( + patch("tools.skill_manager_tool.SKILLS_DIR", local), + patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}), + ): + result = _patch_skill( + "shared-patchable", "original step", "patched step", + ) + assert result["success"] is True + assert "note" in result + # Local copy has the patched content + assert "patched step" in (local / "shared-patchable" / "SKILL.md").read_text() + # External source is unchanged + assert "original step" in (ext / "shared-patchable" / "SKILL.md").read_text() + + def test_patch_external_skill_rolls_back_copy_on_invalid_match(self, tmp_path): + local, ext, hermes_home = self._setup(tmp_path) + with ( + patch("tools.skill_manager_tool.SKILLS_DIR", local), + patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}), + ): + result = _patch_skill( + "shared-patchable", "this does not exist", "whatever", + ) + assert result["success"] is False + # The CoW copy should have been rolled back; local dir is empty. + assert not (local / "shared-patchable").exists() + # External still untouched + assert "original step" in (ext / "shared-patchable" / "SKILL.md").read_text() + + +class TestWriteFileExternalSkillCopyOnWrite: + def test_write_file_on_external_copies_skill_first(self, tmp_path): + local = tmp_path / "hermes" / "skills" + local.mkdir(parents=True) + ext = tmp_path / "shared-skills" + ext.mkdir() + _write_external_skill(ext, "shared-writefile") + hermes_home = tmp_path / "hermes" + (hermes_home / "config.yaml").write_text( + f"skills:\n external_dirs:\n - {ext}\n" + ) + with ( + patch("tools.skill_manager_tool.SKILLS_DIR", local), + patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}), + ): + result = _write_file( + "shared-writefile", "references/notes.md", "local override notes", + ) + assert result["success"] is True + assert "note" in result + # New file is in the local copy + assert (local / "shared-writefile" / "references" / "notes.md").read_text() == "local override notes" + # Original SKILL.md was also copied over as part of the CoW + assert (local / "shared-writefile" / "SKILL.md").exists() + # External skill has no new file + assert not (ext / "shared-writefile" / "references" / "notes.md").exists() + + +class TestRemoveFileExternalSkillCopyOnWrite: + def test_remove_file_on_external_copies_then_removes(self, tmp_path): + local = tmp_path / "hermes" / "skills" + local.mkdir(parents=True) + ext = tmp_path / "shared-skills" + ext.mkdir() + source = _write_external_skill(ext, "shared-removefile") + (source / "references").mkdir() + (source / "references" / "notes.md").write_text("to be removed") + hermes_home = tmp_path / "hermes" + (hermes_home / "config.yaml").write_text( + f"skills:\n external_dirs:\n - {ext}\n" + ) + with ( + patch("tools.skill_manager_tool.SKILLS_DIR", local), + patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}), + ): + result = _remove_file("shared-removefile", "references/notes.md") + assert result["success"] is True + assert "note" in result + # Local copy exists, but the file is gone from the copy + assert (local / "shared-removefile" / "SKILL.md").exists() + assert not (local / "shared-removefile" / "references" / "notes.md").exists() + # External source still has the file + assert (ext / "shared-removefile" / "references" / "notes.md").exists() + + +class TestDeleteExternalSkillRejected: + def test_delete_external_skill_returns_error(self, tmp_path): + local = tmp_path / "hermes" / "skills" + local.mkdir(parents=True) + ext = tmp_path / "shared-skills" + ext.mkdir() + _write_external_skill(ext, "shared-undeletable") + hermes_home = tmp_path / "hermes" + (hermes_home / "config.yaml").write_text( + f"skills:\n external_dirs:\n - {ext}\n" + ) + with ( + patch("tools.skill_manager_tool.SKILLS_DIR", local), + patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}), + ): + result = _delete_skill("shared-undeletable") + assert result["success"] is False + assert "skills.disabled" in result["error"] + # External skill still exists + assert (ext / "shared-undeletable" / "SKILL.md").exists() + + def test_delete_local_skill_still_works(self, tmp_path): + local = tmp_path / "hermes" / "skills" + local.mkdir(parents=True) + hermes_home = tmp_path / "hermes" + with ( + patch("tools.skill_manager_tool.SKILLS_DIR", local), + patch.dict(os.environ, {"HERMES_HOME": str(hermes_home)}), + ): + _create_skill("local-goner", VALID_SKILL_CONTENT) + result = _delete_skill("local-goner") + assert result["success"] is True + assert not (local / "local-goner").exists() diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index b8d8d62232e7..bad7c3923e90 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -219,6 +219,55 @@ def _find_skill(name: str) -> Optional[Dict[str, Any]]: return None +def _is_external_skill(skill_path: Path) -> bool: + """True if the skill lives outside the profile-local SKILLS_DIR. + + An "external" skill is anything resolved from ``skills.external_dirs`` + (or any future shared-skills mechanism). Such skills are treated as + read-only by the agent: mutations are copy-on-written into the local + profile ``skills/`` directory so they become profile-local overrides, + matching the documented behavior at + https://hermes-agent.nousresearch.com/docs/user-guide/features/skills/#external-skill-directories + """ + try: + skill_path.resolve().relative_to(SKILLS_DIR.resolve()) + return False + except ValueError: + return True + + +def _copy_on_write_to_local(skill_path: Path) -> Path: + """Copy an external skill dir into the profile-local SKILLS_DIR. + + Preserves the skill's directory name. Returns the path to the new + local copy. Raises RuntimeError if a skill with the same name already + exists locally (which would indicate ``_find_skill`` should have + returned the local one instead). + """ + name = skill_path.name + local_dir = SKILLS_DIR / name + if local_dir.exists(): + raise RuntimeError( + f"Cannot copy-on-write: {local_dir} already exists. " + f"This suggests the skill resolver returned an external path " + f"despite a local copy being available." + ) + local_dir.parent.mkdir(parents=True, exist_ok=True) + shutil.copytree(skill_path, local_dir) + return local_dir + + +# Human-readable note attached to mutation responses when a copy-on-write +# occurred. The agent should surface this to the user so they understand +# why the change didn't propagate to other profiles using the shared copy. +_COPY_ON_WRITE_NOTE_TEMPLATE = ( + "This skill originally lived at {original} (external / shared). " + "Your change was saved as a profile-local override at {local} and the " + "original is unchanged. To propagate the change to every profile that " + "uses this skill, edit the source file directly outside the agent." +) + + def _validate_file_path(file_path: str) -> Optional[str]: """ Validate a file path for write_file/remove_file. @@ -339,7 +388,12 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An def _edit_skill(name: str, content: str) -> Dict[str, Any]: - """Replace the SKILL.md of any existing skill (full rewrite).""" + """Replace the SKILL.md of any existing skill (full rewrite). + + External / shared skills are copied to the profile-local SKILLS_DIR + before editing (copy-on-write) so the original remains untouched and + the edit only affects the current profile. + """ err = _validate_frontmatter(content) if err: return {"success": False, "error": err} @@ -352,23 +406,46 @@ def _edit_skill(name: str, content: str) -> Dict[str, Any]: if not existing: return {"success": False, "error": f"Skill '{name}' not found. Use skills_list() to see available skills."} - skill_md = existing["path"] / "SKILL.md" - # Back up original content for rollback + original_skill_dir = existing["path"] + was_external = _is_external_skill(original_skill_dir) + copy_on_write_note = None + copied_dir = None + + if was_external: + try: + copied_dir = _copy_on_write_to_local(original_skill_dir) + except RuntimeError as exc: + return {"success": False, "error": str(exc)} + skill_dir = copied_dir + copy_on_write_note = _COPY_ON_WRITE_NOTE_TEMPLATE.format( + original=original_skill_dir, local=skill_dir, + ) + else: + skill_dir = original_skill_dir + + skill_md = skill_dir / "SKILL.md" + # Back up original content for rollback (of the file we're about to write). original_content = skill_md.read_text(encoding="utf-8") if skill_md.exists() else None _atomic_write_text(skill_md, content) - # Security scan — roll back on block - scan_error = _security_scan_skill(existing["path"]) + # Security scan — roll back on block. + scan_error = _security_scan_skill(skill_dir) if scan_error: - if original_content is not None: + if was_external: + # Roll back the copy entirely; nothing was mutated in place. + shutil.rmtree(copied_dir, ignore_errors=True) + elif original_content is not None: _atomic_write_text(skill_md, original_content) return {"success": False, "error": scan_error} - return { + result = { "success": True, "message": f"Skill '{name}' updated.", - "path": str(existing["path"]), + "path": str(skill_dir), } + if copy_on_write_note: + result["note"] = copy_on_write_note + return result def _patch_skill( @@ -392,22 +469,47 @@ def _patch_skill( if not existing: return {"success": False, "error": f"Skill '{name}' not found."} - skill_dir = existing["path"] + original_skill_dir = existing["path"] + was_external = _is_external_skill(original_skill_dir) + copy_on_write_note = None + copied_dir = None if file_path: - # Patching a supporting file + # Patching a supporting file — validate the path up front so an + # invalid request doesn't trigger a copy-on-write. err = _validate_file_path(file_path) if err: return {"success": False, "error": err} - target = skill_dir / file_path + + # Read the current content from the *original* location before + # copying, so we can validate the patch target exists without + # allocating disk space for the copy on the error path. + src_target = original_skill_dir / (file_path or "SKILL.md") + if not src_target.exists(): + return { + "success": False, + "error": f"File not found: {src_target.relative_to(original_skill_dir)}", + } + content = src_target.read_text(encoding="utf-8") + + if was_external: + try: + copied_dir = _copy_on_write_to_local(original_skill_dir) + except RuntimeError as exc: + return {"success": False, "error": str(exc)} + skill_dir = copied_dir + copy_on_write_note = _COPY_ON_WRITE_NOTE_TEMPLATE.format( + original=original_skill_dir, local=skill_dir, + ) else: - # Patching SKILL.md - target = skill_dir / "SKILL.md" + skill_dir = original_skill_dir - if not target.exists(): - return {"success": False, "error": f"File not found: {target.relative_to(skill_dir)}"} + target = skill_dir / (file_path or "SKILL.md") - content = target.read_text(encoding="utf-8") + def _cleanup_cow_on_failure() -> None: + """If we copied the skill for CoW, delete the copy so nothing lingers.""" + if was_external and copied_dir is not None: + shutil.rmtree(copied_dir, ignore_errors=True) # Use the same fuzzy matching engine as the file patch tool. # This handles whitespace normalization, indentation differences, @@ -419,6 +521,7 @@ def _patch_skill( content, old_string, new_string, replace_all ) if match_error: + _cleanup_cow_on_failure() # Show a short preview of the file so the model can self-correct preview = content[:500] + ("..." if len(content) > 500 else "") return { @@ -431,12 +534,14 @@ def _patch_skill( target_label = "SKILL.md" if not file_path else file_path err = _validate_content_size(new_content, label=target_label) if err: + _cleanup_cow_on_failure() return {"success": False, "error": err} # If patching SKILL.md, validate frontmatter is still intact if not file_path: err = _validate_frontmatter(new_content) if err: + _cleanup_cow_on_failure() return { "success": False, "error": f"Patch would break SKILL.md structure: {err}", @@ -448,22 +553,48 @@ def _patch_skill( # Security scan — roll back on block scan_error = _security_scan_skill(skill_dir) if scan_error: - _atomic_write_text(target, original_content) + if was_external: + _cleanup_cow_on_failure() + else: + _atomic_write_text(target, original_content) return {"success": False, "error": scan_error} - return { + result = { "success": True, "message": f"Patched {'SKILL.md' if not file_path else file_path} in skill '{name}' ({match_count} replacement{'s' if match_count > 1 else ''}).", } + if copy_on_write_note: + result["note"] = copy_on_write_note + return result def _delete_skill(name: str) -> Dict[str, Any]: - """Delete a skill.""" + """Delete a skill. + + External / shared skills cannot be deleted via this tool — they live + outside the profile's own skills dir and may be in use by other + profiles. To hide a shared skill from the current profile, add it to + the ``skills.disabled`` list in ``config.yaml``. To actually remove a + shared skill, delete the source directory directly outside the agent. + """ existing = _find_skill(name) if not existing: return {"success": False, "error": f"Skill '{name}' not found."} skill_dir = existing["path"] + + if _is_external_skill(skill_dir): + return { + "success": False, + "error": ( + f"Skill '{name}' is external/shared (located at {skill_dir}) " + f"and cannot be deleted via this tool. To hide it from the " + f"current profile, add '{name}' to skills.disabled in " + f"config.yaml. To fully remove it, delete the source " + f"directory directly outside the agent." + ), + } + shutil.rmtree(skill_dir) # Clean up empty category directories (don't remove SKILLS_DIR itself) @@ -505,30 +636,58 @@ def _write_file(name: str, file_path: str, file_content: str) -> Dict[str, Any]: if not existing: return {"success": False, "error": f"Skill '{name}' not found. Create it first with action='create'."} - target = existing["path"] / file_path + original_skill_dir = existing["path"] + was_external = _is_external_skill(original_skill_dir) + copy_on_write_note = None + copied_dir = None + + if was_external: + try: + copied_dir = _copy_on_write_to_local(original_skill_dir) + except RuntimeError as exc: + return {"success": False, "error": str(exc)} + skill_dir = copied_dir + copy_on_write_note = _COPY_ON_WRITE_NOTE_TEMPLATE.format( + original=original_skill_dir, local=skill_dir, + ) + else: + skill_dir = original_skill_dir + + target = skill_dir / file_path target.parent.mkdir(parents=True, exist_ok=True) # Back up for rollback original_content = target.read_text(encoding="utf-8") if target.exists() else None _atomic_write_text(target, file_content) # Security scan — roll back on block - scan_error = _security_scan_skill(existing["path"]) + scan_error = _security_scan_skill(skill_dir) if scan_error: - if original_content is not None: + if was_external: + # Throw away the whole copy; nothing existed before it. + shutil.rmtree(copied_dir, ignore_errors=True) + elif original_content is not None: _atomic_write_text(target, original_content) else: target.unlink(missing_ok=True) return {"success": False, "error": scan_error} - return { + result = { "success": True, "message": f"File '{file_path}' written to skill '{name}'.", "path": str(target), } + if copy_on_write_note: + result["note"] = copy_on_write_note + return result def _remove_file(name: str, file_path: str) -> Dict[str, Any]: - """Remove a supporting file from any skill directory.""" + """Remove a supporting file from any skill directory. + + For external / shared skills, the whole skill is first copy-on-written + into the profile-local SKILLS_DIR, then the file is removed from the + copy. The original external skill is untouched. + """ err = _validate_file_path(file_path) if err: return {"success": False, "error": err} @@ -536,24 +695,41 @@ def _remove_file(name: str, file_path: str) -> Dict[str, Any]: existing = _find_skill(name) if not existing: return {"success": False, "error": f"Skill '{name}' not found."} - skill_dir = existing["path"] + original_skill_dir = existing["path"] - target = skill_dir / file_path - if not target.exists(): - # List what's actually there for the model to see + # Validate the target file exists in the original skill before doing + # anything destructive or disk-allocating. + if not (original_skill_dir / file_path).exists(): available = [] for subdir in ALLOWED_SUBDIRS: - d = skill_dir / subdir + d = original_skill_dir / subdir if d.exists(): for f in d.rglob("*"): if f.is_file(): - available.append(str(f.relative_to(skill_dir))) + available.append(str(f.relative_to(original_skill_dir))) return { "success": False, "error": f"File '{file_path}' not found in skill '{name}'.", "available_files": available if available else None, } + was_external = _is_external_skill(original_skill_dir) + copy_on_write_note = None + copied_dir = None + + if was_external: + try: + copied_dir = _copy_on_write_to_local(original_skill_dir) + except RuntimeError as exc: + return {"success": False, "error": str(exc)} + skill_dir = copied_dir + copy_on_write_note = _COPY_ON_WRITE_NOTE_TEMPLATE.format( + original=original_skill_dir, local=skill_dir, + ) + else: + skill_dir = original_skill_dir + + target = skill_dir / file_path target.unlink() # Clean up empty subdirectories @@ -561,10 +737,13 @@ def _remove_file(name: str, file_path: str) -> Dict[str, Any]: if parent != skill_dir and parent.exists() and not any(parent.iterdir()): parent.rmdir() - return { + result = { "success": True, "message": f"File '{file_path}' removed from skill '{name}'.", } + if copy_on_write_note: + result["note"] = copy_on_write_note + return result # ============================================================================= diff --git a/website/docs/user-guide/features/skills.md b/website/docs/user-guide/features/skills.md index 3d166b978269..cc7601385feb 100644 --- a/website/docs/user-guide/features/skills.md +++ b/website/docs/user-guide/features/skills.md @@ -184,7 +184,7 @@ Paths support `~` expansion and `${VAR}` environment variable substitution. ### How it works -- **Read-only**: External dirs are only scanned for skill discovery. When the agent creates or edits a skill, it always writes to `~/.hermes/skills/`. +- **Read-only (copy-on-write)**: External dirs are treated as read-only. When the agent creates a new skill, it always writes to `~/.hermes/skills/`. When the agent **edits an existing external skill** (via `skill_edit`, `skill_patch`, `skill_write_file`, or `skill_remove_file`), Hermes first copies the whole skill directory into the profile-local `~/.hermes/skills/` and then applies the change to the copy. The external source is never mutated by the agent — the edited skill becomes a profile-local override that shadows the shared version by name. Deleting an external skill is rejected with a message pointing to `skills.disabled` (to hide it from the current profile) or direct filesystem removal (to unshare it from all profiles). - **Local precedence**: If the same skill name exists in both the local dir and an external dir, the local version wins. - **Full integration**: External skills appear in the system prompt index, `skills_list`, `skill_view`, and as `/skill-name` slash commands — no different from local skills. - **Non-existent paths are silently skipped**: If a configured directory doesn't exist, Hermes ignores it without errors. Useful for optional shared directories that may not be present on every machine.