fix(skills): pin protects against deletion only, not edits (#20220)

Previously, pinning a skill blocked every skill_manage write action
(edit, patch, delete, write_file, remove_file). The 'hard fence'
design conflated two concerns:

  1. Pin as deletion protection — don't let the curator archive
     or the agent delete a stable skill.
  2. Pin as content freeze — don't let the agent rewrite it mid-conversation.

In practice (1) is what users pin for: they want a skill to survive
curator passes. (2) created friction — agents finding a new pitfall
in a pinned skill had to ask the user to unpin, then the agent
patches, then the user re-pins. The dance discouraged skill
maintenance and pinned skills went stale.

This narrows the _pinned_guard to skill_manage(action='delete') only.
Patches, edits, and supporting-file writes go through on pinned
skills so the agent can keep improving them. The curator's own
pinned-skip behavior (agent/curator.py:271 for auto-archive,
line 349 for the LLM review prompt) is unchanged — curator still
never touches pinned skills.

Changes:
- tools/skill_manager_tool.py: remove _pinned_guard calls from
  _edit_skill, _patch_skill, _write_file, _remove_file; keep on
  _delete_skill. Updated _pinned_guard docstring and error message.
- tools/skill_manager_tool.py: updated skill_manage model-facing tool
  description to reflect the new semantic.
- website/docs/user-guide/features/curator.md: updated pinning
  section.
- tests/tools/test_skill_manager_tool.py: flipped refuses-pinned
  tests for edit/patch/write_file/remove_file into allowed-when-pinned;
  kept test_delete_refuses_pinned (strengthened assertion to check the
  'cannot be deleted' wording).

Closes #18354

tests/tools/test_skill_manager_tool.py

@@ -838,12 +838,13 @@ class TestExternalSkillMutations:
 
 
 # ---------------------------------------------------------------------------
-# Pinned-skill guard — skill_manage refuses all writes to pinned skills.
+# Pinned-skill guard — skill_manage refuses only `delete` on pinned skills.
-# The user unpins via `hermes curator unpin <name>`.
+# Patches and edits go through so pinned skills can still evolve as pitfalls
+# come up. The user unpins via `hermes curator unpin <name>` to delete.
 # ---------------------------------------------------------------------------
 
 class TestPinnedGuard:
-    """Every mutation action must refuse when the skill is pinned."""
+    """Delete is refused on pinned skills; patch/edit/write_file/remove_file are allowed."""
 
     @staticmethod
     def _pin(name: str):
@@ -852,31 +853,28 @@ class TestPinnedGuard:
             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):
+    def test_edit_allowed_when_pinned(self, tmp_path):
+        """Pin does NOT block edit — agent can still improve pinned skills."""
         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 result["success"] is True, result
-        assert "pinned" in result["error"].lower()
+        # Content updated
-        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
+        assert "A test skill" not in content
 
-    def test_patch_refuses_pinned(self, tmp_path):
+    def test_patch_allowed_when_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 result["success"] is True, result
-        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
+        assert "Do the new thing." in content
 
-    def test_patch_supporting_file_refuses_pinned(self, tmp_path):
+    def test_patch_supporting_file_allowed_when_pinned(self, tmp_path):
-        """Pin covers supporting files too, not just SKILL.md."""
+        """Supporting-file patches also go through on pinned skills."""
         with _skill_dir(tmp_path):
             _create_skill("my-skill", VALID_SKILL_CONTENT)
             _write_file("my-skill", "references/api.md", "original")
@@ -885,57 +883,56 @@ class TestPinnedGuard:
                     "my-skill", "original", "modified",
                     file_path="references/api.md",
                 )
-        assert result["success"] is False
+        assert result["success"] is True, result
-        assert "pinned" in result["error"].lower()
+        assert (tmp_path / "my-skill" / "references" / "api.md").read_text() == "modified"
-        assert (tmp_path / "my-skill" / "references" / "api.md").read_text() == "original"
 
     def test_delete_refuses_pinned(self, tmp_path):
+        """Delete is the one action pin still blocks — it's the irrecoverable one."""
         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()
+        assert "cannot be deleted" in result["error"]
+        assert "hermes curator unpin my-skill" in result["error"]
         # Skill still exists
         assert (tmp_path / "my-skill" / "SKILL.md").exists()
 
-    def test_write_file_refuses_pinned(self, tmp_path):
+    def test_write_file_allowed_when_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 result["success"] is True, result
-        assert "pinned" in result["error"].lower()
+        assert (tmp_path / "my-skill" / "references" / "api.md").read_text() == "content"
-        assert not (tmp_path / "my-skill" / "references" / "api.md").exists()
 
-    def test_remove_file_refuses_pinned(self, tmp_path):
+    def test_remove_file_allowed_when_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 result["success"] is True, result
-        assert "pinned" in result["error"].lower()
+        assert not (tmp_path / "my-skill" / "references" / "api.md").exists()
-        # 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.
+        """Sanity check: the guard doesn't fire for unpinned skills on delete.
 
-        Only the specifically-pinned skill is refused; a sibling skill must
+        Only the specifically-pinned skill is refused from delete; a sibling
-        still be freely editable.
+        skill must still be freely deletable.
         """
         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)
+                blocked = _delete_skill("pinned-one")
-                allowed = _edit_skill("free-one", VALID_SKILL_CONTENT_2)
+                allowed = _delete_skill("free-one")
         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.
+        """If skill_usage.get_record raises, we allow delete through.
 
         Rationale: a corrupted telemetry file shouldn't lock the agent out
         of skills it would otherwise be allowed to touch.
@@ -944,5 +941,5 @@ class TestPinnedGuard:
             _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)
+                result = _delete_skill("my-skill")
         assert result["success"] is True

tools/skill_manager_tool.py

@@ -137,14 +137,12 @@ def _containing_skills_root(skill_path: Path) -> Path:
 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
+    Pin protects a skill from **deletion** — both the curator's auto-archive
-    way to modify one is for the user to unpin it via
+    passes and the agent's ``skill_manage(action="delete")`` tool call. The
-    ``hermes curator unpin <name>`` (or edit it directly by hand).  This
+    agent can still patch/edit pinned skills; pin only guards against
-    mirrors the curator's own pinned-skip behavior but extends the guard
+    irrecoverable loss, not against content evolution.
-    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
+    Best-effort: if the sidecar is unreadable we let the delete through
     rather than block on a broken telemetry file.
     """
     try:
@@ -152,9 +150,11 @@ def _pinned_guard(name: str) -> Optional[str]:
         rec = skill_usage.get_record(name)
         if rec.get("pinned"):
             return (
-                f"Skill '{name}' is pinned and cannot be modified by "
+                f"Skill '{name}' is pinned and cannot be deleted by "
                 f"skill_manage. Ask the user to run "
-                f"`hermes curator unpin {name}` if they want the change."
+                f"`hermes curator unpin {name}` if they want to delete it. "
+                f"Patches and edits are allowed on pinned skills; only "
+                f"deletion is blocked."
             )
     except Exception:
         logger.debug("pinned-guard lookup failed for %s", name, exc_info=True)
@@ -439,10 +439,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."}
 
-    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
@@ -483,10 +479,6 @@ 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:
@@ -645,10 +637,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'."}
 
-    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}
@@ -683,10 +671,6 @@ 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)
@@ -835,9 +819,10 @@ SKILL_MANAGE_SCHEMA = {
         "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.\n\n"
-        "Pinned skills are off-limits — all write actions refuse with a message "
+        "Pinned skills are protected from deletion only — skill_manage(action='delete') "
-        "pointing the user to `hermes curator unpin <name>`. Don't try to route "
+        "will refuse with a message pointing the user to `hermes curator unpin <name>`. "
-        "around this by renaming or recreating."
+        "Patches and edits go through on pinned skills so you can still improve them as "
+        "pitfalls come up; pin only guards against irrecoverable loss."
     ),
     "parameters": {
         "type": "object",

website/docs/user-guide/features/curator.md

@@ -157,10 +157,10 @@ If you want to protect a specific skill from ever being touched — for example
 
 ## Pinning a skill
 
-Pinning is a hard fence against both automated and agent-driven changes. Once a skill is pinned:
+Pinning protects a skill from deletion — both the curator's automated archive passes and the agent's `skill_manage(action="delete")` tool call. Once a skill is pinned:
 
 - The **curator** skips it during auto-transitions (`active → stale → archived`), and its LLM review pass is instructed to leave it alone.
-- The **agent's `skill_manage` tool** refuses every write action on it. Calls to `edit`, `patch`, `delete`, `write_file`, and `remove_file` return a refusal that tells the model to ask the user to run `hermes curator unpin <name>`. This prevents the agent from silently rewriting a skill mid-conversation.
+- The **agent's `skill_manage` tool** refuses `delete` on it, pointing the user at `hermes curator unpin <name>`. Patches and edits still go through, so the agent can improve a pinned skill's content as pitfalls come up without a pin/unpin/re-pin dance.
 
 Pin and unpin with:
 
@@ -173,7 +173,7 @@ The flag is stored as `"pinned": true` on the skill's entry in `~/.hermes/skills
 
 Only **agent-created** skills can be pinned — bundled and hub-installed skills are never subject to curator mutation in the first place, and `hermes curator pin` will refuse with an explanatory message if you try.
 
-If you need to update a pinned skill yourself, edit `~/.hermes/skills/<name>/SKILL.md` directly with your editor. The pin only guards the agent's tool path, not your own filesystem access.
+If you want a stronger guarantee than "no deletion" — for instance, freezing a skill's content entirely while the agent still reads it — edit `~/.hermes/skills/<name>/SKILL.md` directly with your editor. The pin guards tool-driven deletion, not your own filesystem access.
 
 ## Usage telemetry