From 525e1e775d0eed7508202c836a90a70ae692fb70 Mon Sep 17 00:00:00 2001
From: Teknium <127238744+teknium1@users.noreply.github.com>
Date: Fri, 26 Jun 2026 12:49:33 -0700
Subject: [PATCH] fix(skills): background review fork respects pinned skills
 (#53226)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The autonomous self-improvement review fork could still write to a pinned
skill — only external/bundled/hub-installed/protected-builtin skills were
guarded. The curator skips pinned skills from every auto-transition; the
review fork is the same kind of no-user-present actor and must too.

Adds a pin check to _background_review_write_guard so background-origin
edit/patch/delete/write_file/remove_file on a pinned skill are refused.
Stricter than the foreground _pinned_guard (delete-only) by design: with
no user in the loop there is no one to consent to an edit.

Fixes #25839
---
 tests/tools/test_skill_manager_tool.py | 61 ++++++++++++++++++++++++++
 tools/skill_manager_tool.py            | 21 +++++++++
 2 files changed, 82 insertions(+)

diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py
index 9b4e83bbfd0..5797049433d 100644
--- a/tests/tools/test_skill_manager_tool.py
+++ b/tests/tools/test_skill_manager_tool.py
@@ -912,6 +912,67 @@ class TestExternalSkillMutations:
         assert "OLD_MARKER" in (skill_dir / "SKILL.md").read_text()
         assert "NEW_MARKER" not in (skill_dir / "SKILL.md").read_text()
 
+    def test_background_review_refuses_to_patch_pinned_skill(self, tmp_path):
+        """#25839: the autonomous review fork respects pin like the curator
+        does — a pinned skill is off-limits to background maintenance, even
+        for patch/edit (which a foreground user-directed call is allowed to
+        perform). Without a user in the loop there is no one to consent."""
+        from tools.skill_provenance import (
+            BACKGROUND_REVIEW,
+            reset_current_write_origin,
+            set_current_write_origin,
+        )
+
+        def _fake_get_record(skill_name):
+            return {"pinned": True} if skill_name == "my-skill" else {"pinned": False}
+
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            token = set_current_write_origin(BACKGROUND_REVIEW)
+            try:
+                with patch("tools.skill_usage.get_record", side_effect=_fake_get_record):
+                    raw = skill_manage(
+                        action="patch",
+                        name="my-skill",
+                        old_string="Do the thing.",
+                        new_string="Do the new thing.",
+                    )
+            finally:
+                reset_current_write_origin(token)
+
+        result = json.loads(raw)
+        assert result["success"] is False
+        assert "pinned" in result["error"].lower()
+
+    def test_background_review_unpinned_skill_not_blocked_by_pin_guard(self, tmp_path):
+        """The pin guard must not over-block: an unpinned agent-owned skill is
+        still writable by the review fork."""
+        from tools.skill_provenance import (
+            BACKGROUND_REVIEW,
+            reset_current_write_origin,
+            set_current_write_origin,
+        )
+
+        with _skill_dir(tmp_path):
+            _create_skill("my-skill", VALID_SKILL_CONTENT)
+            token = set_current_write_origin(BACKGROUND_REVIEW)
+            try:
+                with patch(
+                    "tools.skill_usage.get_record",
+                    side_effect=lambda n: {"pinned": False},
+                ):
+                    raw = skill_manage(
+                        action="patch",
+                        name="my-skill",
+                        old_string="Do the thing.",
+                        new_string="Do the new thing.",
+                    )
+            finally:
+                reset_current_write_origin(token)
+
+        result = json.loads(raw)
+        assert result["success"] is True
+
 
 
 # ---------------------------------------------------------------------------
diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py
index 3a6f315b224..5d1e8fd117a 100644
--- a/tools/skill_manager_tool.py
+++ b/tools/skill_manager_tool.py
@@ -254,6 +254,27 @@ def _background_review_write_guard(
     except Exception:
         return None
 
+    # Pin must be respected by autonomous maintenance. The curator already
+    # skips pinned skills from every auto-transition; the background review
+    # fork is the same kind of autonomous, no-user-present actor, so it must
+    # not write to a pinned skill either (issue #25839). This is stricter than
+    # the foreground ``_pinned_guard`` (which only blocks deletion) precisely
+    # because there is no user in the loop to consent to an edit here.
+    try:
+        from tools import skill_usage
+        if skill_usage.get_record(name).get("pinned"):
+            return {
+                "success": False,
+                "error": (
+                    f"Refusing background curator {action} for pinned skill "
+                    f"'{name}': pinned skills are off-limits to autonomous "
+                    "maintenance. Ask the user to run "
+                    f"`hermes curator unpin {name}` if they want it changed."
+                ),
+            }
+    except Exception:
+        logger.debug("pinned skill guard lookup failed for %s", name, exc_info=True)
+
     try:
         from agent.skill_utils import is_external_skill_path
         if is_external_skill_path(skill_dir):