fix(skills): background review fork respects pinned skills (#53226)

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
This commit is contained in:
Teknium 2026-06-26 12:49:33 -07:00 committed by GitHub
parent f509f6e598
commit 525e1e775d
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 82 additions and 0 deletions

View file

@ -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
# ---------------------------------------------------------------------------

View file

@ -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):