diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index e24e19dea1..96c3a361f0 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -838,12 +838,13 @@ class TestExternalSkillMutations: # --------------------------------------------------------------------------- -# Pinned-skill guard — skill_manage refuses all writes to pinned skills. -# The user unpins via `hermes curator unpin `. +# Pinned-skill guard — skill_manage refuses only `delete` on pinned skills. +# Patches and edits go through so pinned skills can still evolve as pitfalls +# come up. The user unpins via `hermes curator unpin ` 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 "pinned" in result["error"].lower() - assert "hermes curator unpin my-skill" in result["error"] - # Original content preserved + assert result["success"] is True, result + # Content updated 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 "pinned" in result["error"].lower() - assert "hermes curator unpin my-skill" in result["error"] + assert result["success"] is True, result 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): - """Pin covers supporting files too, not just SKILL.md.""" + def test_patch_supporting_file_allowed_when_pinned(self, tmp_path): + """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 "pinned" in result["error"].lower() - assert (tmp_path / "my-skill" / "references" / "api.md").read_text() == "original" + assert result["success"] is True, result + assert (tmp_path / "my-skill" / "references" / "api.md").read_text() == "modified" 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 "pinned" in result["error"].lower() - assert not (tmp_path / "my-skill" / "references" / "api.md").exists() + assert result["success"] is True, result + assert (tmp_path / "my-skill" / "references" / "api.md").read_text() == "content" - 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 "pinned" in result["error"].lower() - # File still there - assert (tmp_path / "my-skill" / "references" / "api.md").exists() + assert result["success"] is True, result + assert not (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 - still be freely editable. + Only the specifically-pinned skill is refused from delete; a sibling + 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) - allowed = _edit_skill("free-one", VALID_SKILL_CONTENT_2) + blocked = _delete_skill("pinned-one") + 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 diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 58c3fe3d2d..ed4cb3f103 100644 --- a/tools/skill_manager_tool.py +++ b/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 - 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. + Pin protects a skill from **deletion** — both the curator's auto-archive + passes and the agent's ``skill_manage(action="delete")`` tool call. The + agent can still patch/edit pinned skills; pin only guards against + irrecoverable loss, not against content evolution. - 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 " - "pointing the user to `hermes curator unpin `. Don't try to route " - "around this by renaming or recreating." + "Pinned skills are protected from deletion only — skill_manage(action='delete') " + "will refuse with a message pointing the user to `hermes curator unpin `. " + "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", diff --git a/website/docs/user-guide/features/curator.md b/website/docs/user-guide/features/curator.md index fccef941dc..e53076b45e 100644 --- a/website/docs/user-guide/features/curator.md +++ b/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 `. 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 `. 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//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//SKILL.md` directly with your editor. The pin guards tool-driven deletion, not your own filesystem access. ## Usage telemetry