diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 9918a826cbc..ab900ca61c5 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -566,3 +566,151 @@ class TestSecurityScanGate: with patch("hermes_cli.config.load_config", side_effect=RuntimeError("boom")): assert _guard_agent_created_enabled() is False + + +# --------------------------------------------------------------------------- +# External skills directories (skills.external_dirs) — mutations in place +# --------------------------------------------------------------------------- + + +@contextmanager +def _two_roots(local_dir: Path, external_dir: Path): + """Patch the skill manager so local SKILLS_DIR = local_dir and + get_all_skills_dirs() returns [local_dir, external_dir] in order.""" + with patch("tools.skill_manager_tool.SKILLS_DIR", local_dir), \ + patch("agent.skill_utils.get_all_skills_dirs", + return_value=[local_dir, external_dir]): + yield + + +def _write_external_skill(external_dir: Path, name: str = "ext-skill") -> Path: + skill_dir = external_dir / name + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + f"---\nname: {name}\ndescription: An external skill.\n---\n\n" + "# External\n\nBody with OLD_MARKER here.\n" + ) + return skill_dir + + +class TestExternalSkillMutations: + """Verify skill_manage can patch/edit/write/remove/delete skills that live + under skills.external_dirs — in place, without duplicating to local. + + Regression for issues #4759 and #4381: the read-only gate used to refuse + with 'Skill X is in an external directory and cannot be modified', which + caused agents to create duplicate copies in ~/.hermes/skills/ as a + workaround. + """ + + def test_patch_external_skill_writes_in_place(self, tmp_path): + local = tmp_path / "local" + external = tmp_path / "vault" + local.mkdir(); external.mkdir() + skill_dir = _write_external_skill(external) + + with _two_roots(local, external): + result = _patch_skill("ext-skill", "OLD_MARKER", "NEW_MARKER") + + assert result["success"] is True, result + assert "NEW_MARKER" in (skill_dir / "SKILL.md").read_text() + # No duplicate in local + assert not (local / "ext-skill").exists() + + def test_edit_external_skill_writes_in_place(self, tmp_path): + local = tmp_path / "local" + external = tmp_path / "vault" + local.mkdir(); external.mkdir() + skill_dir = _write_external_skill(external) + + new_content = ( + "---\nname: ext-skill\ndescription: Rewritten.\n---\n\n" + "# Rewritten\n\nBrand new body.\n" + ) + with _two_roots(local, external): + result = _edit_skill("ext-skill", new_content) + + assert result["success"] is True, result + assert "Brand new body" in (skill_dir / "SKILL.md").read_text() + assert not (local / "ext-skill").exists() + + def test_write_file_on_external_skill(self, tmp_path): + local = tmp_path / "local" + external = tmp_path / "vault" + local.mkdir(); external.mkdir() + skill_dir = _write_external_skill(external) + + with _two_roots(local, external): + result = _write_file("ext-skill", "references/notes.md", "# Notes\n") + + assert result["success"] is True, result + assert (skill_dir / "references" / "notes.md").read_text() == "# Notes\n" + assert not (local / "ext-skill").exists() + + def test_remove_file_on_external_skill(self, tmp_path): + local = tmp_path / "local" + external = tmp_path / "vault" + local.mkdir(); external.mkdir() + skill_dir = _write_external_skill(external) + (skill_dir / "references").mkdir() + (skill_dir / "references" / "notes.md").write_text("# Notes\n") + + with _two_roots(local, external): + result = _remove_file("ext-skill", "references/notes.md") + + assert result["success"] is True, result + assert not (skill_dir / "references" / "notes.md").exists() + + def test_delete_external_skill_removes_skill_not_root(self, tmp_path): + local = tmp_path / "local" + external = tmp_path / "vault" + local.mkdir(); external.mkdir() + skill_dir = _write_external_skill(external) + + with _two_roots(local, external): + result = _delete_skill("ext-skill") + + assert result["success"] is True, result + assert not skill_dir.exists() + # The external root must NOT be rmdir'd, even when empty after deletion + assert external.exists() and external.is_dir() + + def test_delete_external_skill_cleans_empty_category(self, tmp_path): + """When a skill lives under external//, deleting the + last skill in the category should rmdir the empty category dir but + stop at the external root.""" + local = tmp_path / "local" + external = tmp_path / "vault" + local.mkdir(); external.mkdir() + cat_dir = external / "team" + cat_dir.mkdir() + skill_dir = cat_dir / "ext-skill" + skill_dir.mkdir() + (skill_dir / "SKILL.md").write_text( + "---\nname: ext-skill\ndescription: An external skill.\n---\n\n" + "# External\n\nBody.\n" + ) + + with _two_roots(local, external): + result = _delete_skill("ext-skill") + + assert result["success"] is True, result + assert not skill_dir.exists() + assert not cat_dir.exists() # empty category cleaned up + assert external.exists() # but never the external root + + def test_create_still_writes_to_local_root(self, tmp_path): + """Creating a new skill always lands in local SKILLS_DIR, never + external_dirs — create is unchanged by this PR.""" + local = tmp_path / "local" + external = tmp_path / "vault" + local.mkdir(); external.mkdir() + + with _two_roots(local, external): + result = _create_skill("fresh-skill", VALID_SKILL_CONTENT.replace( + "name: test-skill", "name: fresh-skill")) + + assert result["success"] is True, result + assert (local / "fresh-skill" / "SKILL.md").exists() + assert not (external / "fresh-skill").exists() + diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index b1720632cb6..3fdefb2cce3 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -109,16 +109,28 @@ MAX_NAME_LENGTH = 64 MAX_DESCRIPTION_LENGTH = 1024 -def _is_local_skill(skill_path: Path) -> bool: - """Check if a skill path is within the local SKILLS_DIR. - - Skills found in external_dirs are read-only from the agent's perspective. +def _containing_skills_root(skill_path: Path) -> Path: + """Return the skills root directory (local or external_dirs entry) that + contains ``skill_path``. Falls back to the local ``SKILLS_DIR`` if no + match is found (defensive — callers should have located the skill via + ``_find_skill`` first). """ + from agent.skill_utils import get_all_skills_dirs + try: - skill_path.resolve().relative_to(SKILLS_DIR.resolve()) - return True - except ValueError: - return False + resolved = skill_path.resolve() + except OSError: + resolved = skill_path + + for root in get_all_skills_dirs(): + try: + resolved.relative_to(root.resolve()) + return root + except (ValueError, OSError): + continue + return SKILLS_DIR + + MAX_SKILL_CONTENT_CHARS = 100_000 # ~36k tokens at 2.75 chars/token MAX_SKILL_FILE_BYTES = 1_048_576 # 1 MiB per supporting file @@ -397,9 +409,6 @@ 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."} - if not _is_local_skill(existing["path"]): - return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be modified. Copy it to your local skills directory first."} - skill_md = existing["path"] / "SKILL.md" # Back up original content for rollback original_content = skill_md.read_text(encoding="utf-8") if skill_md.exists() else None @@ -440,9 +449,6 @@ def _patch_skill( if not existing: return {"success": False, "error": f"Skill '{name}' not found."} - if not _is_local_skill(existing["path"]): - return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be modified. Copy it to your local skills directory first."} - skill_dir = existing["path"] if file_path: @@ -522,15 +528,13 @@ def _delete_skill(name: str) -> Dict[str, Any]: if not existing: return {"success": False, "error": f"Skill '{name}' not found."} - if not _is_local_skill(existing["path"]): - return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be deleted."} - skill_dir = existing["path"] + skills_root = _containing_skills_root(skill_dir) shutil.rmtree(skill_dir) - # Clean up empty category directories (don't remove SKILLS_DIR itself) + # Clean up empty category directories (don't remove the skills root itself) parent = skill_dir.parent - if parent != SKILLS_DIR and parent.exists() and not any(parent.iterdir()): + if parent != skills_root and parent.exists() and not any(parent.iterdir()): parent.rmdir() return { @@ -567,9 +571,6 @@ 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'."} - if not _is_local_skill(existing["path"]): - return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be modified. Copy it to your local skills directory first."} - target, err = _resolve_skill_target(existing["path"], file_path) if err: return {"success": False, "error": err} @@ -604,9 +605,6 @@ def _remove_file(name: str, file_path: str) -> Dict[str, Any]: if not existing: return {"success": False, "error": f"Skill '{name}' not found."} - if not _is_local_skill(existing["path"]): - return {"success": False, "error": f"Skill '{name}' is in an external directory and cannot be modified."} - skill_dir = existing["path"] target, err = _resolve_skill_target(skill_dir, file_path)