fix(skills): let skill_manage modify skills in external_dirs in place

tests/tools/test_skill_manager_tool.py

@@ -566,3 +566,151 @@ def test_guard_flag_handles_config_error(self):
 
         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/<category>/<name>, 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()
+

tools/skill_manager_tool.py

@@ -109,16 +109,28 @@ def _security_scan_skill(skill_dir: Path) -> Optional[str]:
 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)