feat(skills): refuse skill_manage writes on pinned skills

tests/tools/test_skill_manager_tool.py

@@ -714,3 +714,114 @@ def test_create_still_writes_to_local_root(self, tmp_path):
         assert (local / "fresh-skill" / "SKILL.md").exists()
         assert not (external / "fresh-skill").exists()
 
+
+
+# ---------------------------------------------------------------------------
+# Pinned-skill guard — skill_manage refuses all writes to pinned skills.
+# The user unpins via `hermes curator unpin <name>`.
+# ---------------------------------------------------------------------------
+
+class TestPinnedGuard:
+    """Every mutation action must refuse when the skill is pinned."""
+
+    @staticmethod
+    def _pin(name: str):
+        """Return a patch context that marks *name* as pinned in skill_usage."""
+        def _fake_get_record(skill_name, _name=name):
+            return {"pinned": True} if skill_name == _name else {"pinned": False}
+        return patch("tools.skill_usage.get_record", side_effect=_fake_get_record)
+
+    def test_edit_refuses_pinned(self, tmp_path):
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            with self._pin("my-skill"):
+                result = _edit_skill("my-skill", VALID_SKILL_CONTENT_2)
+        assert result["success"] is False
+        assert "pinned" in result["error"].lower()
+        assert "hermes curator unpin my-skill" in result["error"]
+        # Original content preserved
+        content = (tmp_path / "my-skill" / "SKILL.md").read_text()
+        assert "A test skill" in content
+
+    def test_patch_refuses_pinned(self, tmp_path):
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            with self._pin("my-skill"):
+                result = _patch_skill("my-skill", "Do the thing.", "Do the new thing.")
+        assert result["success"] is False
+        assert "pinned" in result["error"].lower()
+        assert "hermes curator unpin my-skill" in result["error"]
+        content = (tmp_path / "my-skill" / "SKILL.md").read_text()
+        assert "Do the thing." in content  # unchanged
+
+    def test_patch_supporting_file_refuses_pinned(self, tmp_path):
+        """Pin covers supporting files too, not just SKILL.md."""
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            _write_file("my-skill", "references/api.md", "original")
+            with self._pin("my-skill"):
+                result = _patch_skill(
+                    "my-skill", "original", "modified",
+                    file_path="references/api.md",
+                )
+        assert result["success"] is False
+        assert "pinned" in result["error"].lower()
+        assert (tmp_path / "my-skill" / "references" / "api.md").read_text() == "original"
+
+    def test_delete_refuses_pinned(self, tmp_path):
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            with self._pin("my-skill"):
+                result = _delete_skill("my-skill")
+        assert result["success"] is False
+        assert "pinned" in result["error"].lower()
+        # Skill still exists
+        assert (tmp_path / "my-skill" / "SKILL.md").exists()
+
+    def test_write_file_refuses_pinned(self, tmp_path):
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            with self._pin("my-skill"):
+                result = _write_file("my-skill", "references/api.md", "content")
+        assert result["success"] is False
+        assert "pinned" in result["error"].lower()
+        assert not (tmp_path / "my-skill" / "references" / "api.md").exists()
+
+    def test_remove_file_refuses_pinned(self, tmp_path):
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            _write_file("my-skill", "references/api.md", "content")
+            with self._pin("my-skill"):
+                result = _remove_file("my-skill", "references/api.md")
+        assert result["success"] is False
+        assert "pinned" in result["error"].lower()
+        # File still there
+        assert (tmp_path / "my-skill" / "references" / "api.md").exists()
+
+    def test_unpinned_skills_still_editable(self, tmp_path):
+        """Sanity check: the guard doesn't fire for unpinned skills.
+
+        Only the specifically-pinned skill is refused; a sibling skill must
+        still be freely editable.
+        """
+        with _skill_dir(tmp_path):
+            _create_skill("pinned-one", VALID_SKILL_CONTENT)
+            _create_skill("free-one", VALID_SKILL_CONTENT)
+            with self._pin("pinned-one"):
+                blocked = _edit_skill("pinned-one", VALID_SKILL_CONTENT_2)
+                allowed = _edit_skill("free-one", VALID_SKILL_CONTENT_2)
+        assert blocked["success"] is False
+        assert allowed["success"] is True
+
+    def test_broken_sidecar_fails_open(self, tmp_path):
+        """If skill_usage.get_record raises, we allow the write through.
+
+        Rationale: a corrupted telemetry file shouldn't lock the agent out
+        of skills it would otherwise be allowed to touch.
+        """
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            with patch("tools.skill_usage.get_record",
+                       side_effect=RuntimeError("sidecar broken")):
+                result = _edit_skill("my-skill", VALID_SKILL_CONTENT_2)
+        assert result["success"] is True

tools/skill_manager_tool.py

@@ -131,6 +131,33 @@ def _containing_skills_root(skill_path: Path) -> Path:
     return SKILLS_DIR
 
 
+def _pinned_guard(name: str) -> Optional[str]:
+    """Return a refusal message if *name* is pinned, else None.
+
+    Pinned skills are off-limits to the agent's skill_manage tool.  The only
+    way to modify one is for the user to unpin it via
+    ``hermes curator unpin <name>`` (or edit it directly by hand).  This
+    mirrors the curator's own pinned-skip behavior but extends the guard
+    to tool-driven writes as well, giving users a hard fence against
+    accidental agent edits.
+
+    Best-effort: if the sidecar is unreadable we let the write through
+    rather than block on a broken telemetry file.
+    """
+    try:
+        from tools import skill_usage
+        rec = skill_usage.get_record(name)
+        if rec.get("pinned"):
+            return (
+                f"Skill '{name}' is pinned and cannot be modified by "
+                f"skill_manage. Ask the user to run "
+                f"`hermes curator unpin {name}` if they want the change."
+            )
+    except Exception:
+        logger.debug("pinned-guard lookup failed for %s", name, exc_info=True)
+    return None
+
+
 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
 
@@ -409,6 +436,10 @@ 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."}
 
+    pinned_err = _pinned_guard(name)
+    if pinned_err:
+        return {"success": False, "error": pinned_err}
+
     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
@@ -449,6 +480,10 @@ def _patch_skill(
     if not existing:
         return {"success": False, "error": f"Skill '{name}' not found."}
 
+    pinned_err = _pinned_guard(name)
+    if pinned_err:
+        return {"success": False, "error": pinned_err}
+
     skill_dir = existing["path"]
 
     if file_path:
@@ -528,6 +563,10 @@ def _delete_skill(name: str) -> Dict[str, Any]:
     if not existing:
         return {"success": False, "error": f"Skill '{name}' not found."}
 
+    pinned_err = _pinned_guard(name)
+    if pinned_err:
+        return {"success": False, "error": pinned_err}
+
     skill_dir = existing["path"]
     skills_root = _containing_skills_root(skill_dir)
     shutil.rmtree(skill_dir)
@@ -571,6 +610,10 @@ 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'."}
 
+    pinned_err = _pinned_guard(name)
+    if pinned_err:
+        return {"success": False, "error": pinned_err}
+
     target, err = _resolve_skill_target(existing["path"], file_path)
     if err:
         return {"success": False, "error": err}
@@ -605,6 +648,10 @@ def _remove_file(name: str, file_path: str) -> Dict[str, Any]:
     if not existing:
         return {"success": False, "error": f"Skill '{name}' not found."}
 
+    pinned_err = _pinned_guard(name)
+    if pinned_err:
+        return {"success": False, "error": pinned_err}
+
     skill_dir = existing["path"]
 
     target, err = _resolve_skill_target(skill_dir, file_path)
@@ -737,7 +784,10 @@ def skill_manage(
         "After difficult/iterative tasks, offer to save as a skill. "
         "Skip for simple one-offs. Confirm with user before creating/deleting.\n\n"
         "Good skills: trigger conditions, numbered steps with exact commands, "
-        "pitfalls section, verification steps. Use skill_view() to see format examples."
+        "pitfalls section, verification steps. Use skill_view() to see format examples.\n\n"
+        "Pinned skills are off-limits — all write actions refuse with a message "
+        "pointing the user to `hermes curator unpin <name>`. Don't try to route "
+        "around this by renaming or recreating."
     ),
     "parameters": {
         "type": "object",