diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index ab900ca61..9fc8957f1 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -714,3 +714,114 @@ class TestExternalSkillMutations: 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 `. +# --------------------------------------------------------------------------- + +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 diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 3fdefb2cc..cc8b0fed2 100644 --- a/tools/skill_manager_tool.py +++ b/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 `` (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 @@ SKILL_MANAGE_SCHEMA = { "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 `. Don't try to route " + "around this by renaming or recreating." ), "parameters": { "type": "object",