Security fixes for automated skill review role handling

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

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

  merged to main

  files: tests/tools/test_skill_manager_tool.py, tools/skill_manager_tool.py

  Show the diff

  • tests/tools/test_skill_manager_tool.py

    @@ -912,6 +912,67 @@ def test_background_review_refuses_to_patch_external_skill(self, tmp_path):
             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 = [redacted:secret-assignment]
    +            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 = [redacted:secret-assignment]
    +            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
    +
     
     
     # ---------------------------------------------------------------------------

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