diff --git a/agent/background_review.py b/agent/background_review.py index c2ea87bd94e2..a0dbd4a99e28 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -209,7 +209,10 @@ _SKILL_REVIEW_PROMPT = ( "conversation for skills the user loaded via /skill-name or you " "read via skill_view. If any of them covers the territory of the " "new learning, PATCH that one first. It is the skill that was in " - "play, so it's the right one to extend.\n" + "play, so it's the right one to extend — but only if it is " + "curator-managed. Bundled, hub, pinned, and user-owned skills are " + "off-limits to you no matter how relevant (see Protected skills " + "below); for those, fall through to the next option.\n" " 2. UPDATE AN EXISTING UMBRELLA (via skills_list + skill_view). " "If no loaded skill fits but an existing class-level skill does, " "patch it. Add a subsection, a pitfall, or broaden a trigger.\n" @@ -251,10 +254,18 @@ _SKILL_REVIEW_PROMPT = ( "Protected skills (DO NOT edit these):\n" " • Bundled skills (shipped with Hermes, e.g. 'hermes-agent').\n" " • Hub-installed skills (installed via 'hermes skills install').\n" - "Pinned skills (marked via 'hermes curator pin') CAN be improved — " - "pin only blocks deletion/archive/consolidation by the curator, not " - "content updates. Patch them when a pitfall or missing step turns up, " - "same as any other agent-created skill.\n" + " • Skills in skills.external_dirs (externally owned).\n" + " • PINNED skills (marked via 'hermes curator pin'). You are an " + "autonomous no-user-present actor, so pin blocks your writes too — " + "content updates included. Only the user, in a foreground session, " + "can change a pinned skill.\n" + " • USER-OWNED skills — anything not curator-managed. A skill the " + "user hand-wrote, installed by URL, or asked a foreground agent to " + "create is theirs, not yours; your writes to it WILL be refused. " + "This includes skills that were loaded or consulted this session: " + "being in play does not make one yours to edit. If such a skill is " + "wrong or outdated, say so in your reply and recommend " + "'hermes curator adopt ' — do not try to patch it.\n" "If the only skills that need updating are protected, say\n" "'Nothing to save.' and stop.\n\n" "Do NOT capture (these become persistent self-imposed constraints " @@ -309,7 +320,9 @@ _COMBINED_REVIEW_PROMPT = ( " 1. UPDATE A CURRENTLY-LOADED SKILL. Check what skills were " "loaded via /skill-name or skill_view in the conversation. If one " "of them covers the learning, PATCH it first. It was in play; " - "it's the right place.\n" + "it's the right place — provided it is curator-managed. Protected " + "and user-owned skills are off-limits however relevant; fall " + "through when one of those is the best fit.\n" " 2. UPDATE AN EXISTING UMBRELLA (skills_list + skill_view to " "find the right one). Patch it.\n" " 3. ADD A SUPPORT FILE under an existing umbrella via " @@ -337,10 +350,15 @@ _COMBINED_REVIEW_PROMPT = ( "Protected skills (DO NOT edit these):\n" " • Bundled skills (shipped with Hermes, e.g. 'hermes-agent').\n" " • Hub-installed skills (installed via 'hermes skills install').\n" - "Pinned skills (marked via 'hermes curator pin') CAN be improved — " - "pin only blocks deletion/archive/consolidation by the curator, not " - "content updates. Patch them when a pitfall or missing step turns up, " - "same as any other agent-created skill.\n" + " • Skills in skills.external_dirs (externally owned).\n" + " • PINNED skills (marked via 'hermes curator pin'). Pin blocks " + "autonomous writes entirely — content updates included — because no " + "user is present to consent. Only a foreground session can change one.\n" + " • USER-OWNED skills — anything not curator-managed (hand-written, " + "URL-installed, or created by a foreground agent at the user's " + "request). Your writes to these WILL be refused, including to skills " + "loaded or consulted this session. If one is wrong, say so in your " + "reply and recommend 'hermes curator adopt ' instead.\n" "If the only skills that need updating are protected, say\n" "'Nothing to save.' and stop.\n\n" "Do NOT capture as skills (these become persistent self-imposed " diff --git a/hermes_cli/curator.py b/hermes_cli/curator.py index 9fe75a62470b..26d419ffefa0 100644 --- a/hermes_cli/curator.py +++ b/hermes_cli/curator.py @@ -313,6 +313,34 @@ def _cmd_unpin(args) -> int: return 0 +def _cmd_list_unmanaged(args) -> int: + """List curation-eligible skills that carry no provenance marker. + + The same population `status` summarizes, itemized. Useful before deciding + what to hand over with `adopt`. + """ + from tools import skill_usage + + rows = skill_usage.unmanaged_report() + if not rows: + print("curator: no unmanaged skills — every eligible skill is managed") + return 0 + + print(f"unmanaged skills ({len(rows)}):") + for r in sorted(rows, key=lambda x: x["name"]): + why = "created_by:null" if r.get("has_provenance_key") else "no marker" + last = _fmt_ts(r.get("last_activity_at")) + print( + f" {r['name']:44s} " + f"activity={r.get('activity_count', 0):4d} " + f"last_activity={last:14s} " + f"({why})" + ) + print("\nadopt one with `hermes curator adopt `, " + "or all with `hermes curator adopt --all-unmanaged`") + return 0 + + def _cmd_adopt(args) -> int: """Hand unmanaged skills to the curator by explicit user declaration. @@ -716,6 +744,11 @@ def register_cli(parent: argparse.ArgumentParser) -> None: p_unpin.add_argument("skill", help="Skill name") p_unpin.set_defaults(func=_cmd_unpin) + subs.add_parser( + "list-unmanaged", + help="List curation-eligible skills with no provenance marker", + ).set_defaults(func=_cmd_list_unmanaged) + p_adopt = subs.add_parser( "adopt", help="Hand unmanaged skills to the curator (provenance is a user declaration)", diff --git a/tests/hermes_cli/test_curator_status.py b/tests/hermes_cli/test_curator_status.py index 9d28c2da825e..18f8c0878204 100644 --- a/tests/hermes_cli/test_curator_status.py +++ b/tests/hermes_cli/test_curator_status.py @@ -342,3 +342,47 @@ def test_adopt_subcommand_is_registered(): assert named.skill == ["alpha", "beta"] assert named.all_unmanaged is False + +def test_list_unmanaged_subcommand_is_registered(): + import argparse + + import hermes_cli.curator as curator_cli + + parser = argparse.ArgumentParser() + curator_cli.register_cli(parser) + args = parser.parse_args(["list-unmanaged"]) + assert args.func is curator_cli._cmd_list_unmanaged + + +def test_list_unmanaged_itemizes_and_explains(curator_status_env): + """`status` gives the count; this gives the names plus WHY each is + unmanaged, so the user can decide what to adopt.""" + env = curator_status_env + env["make_skill"]("legacy-one") + env["make_skill"]("managed-one") + env["skill_usage"].mark_agent_created("managed-one") + + buf = io.StringIO() + with redirect_stdout(buf): + rc = env["curator_cli"]._cmd_list_unmanaged(Namespace()) + out = buf.getvalue() + + assert rc == 0 + assert "legacy-one" in out + assert "managed-one" not in out + assert "no marker" in out or "created_by:null" in out + assert "curator adopt" in out + + +def test_list_unmanaged_reports_clean_state(curator_status_env): + env = curator_status_env + env["make_skill"]("managed-one") + env["skill_usage"].mark_agent_created("managed-one") + + buf = io.StringIO() + with redirect_stdout(buf): + rc = env["curator_cli"]._cmd_list_unmanaged(Namespace()) + + assert rc == 0 + assert "no unmanaged skills" in buf.getvalue() + diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 397133ed93cf..8c9c83884399 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -1027,6 +1027,12 @@ class TestExternalSkillMutations: with patch( "tools.skill_usage.get_record", side_effect=lambda n: {"pinned": False}, + ), patch( + # Ownership runs before the pin guard; mark the skill + # curator-managed so this test still isolates the PIN guard + # (since #67140 an unmarked skill fails closed on ownership). + "tools.skill_usage.load_usage", + return_value={"my-skill": {"created_by": "agent"}}, ): raw = skill_manage( action="patch", @@ -1073,7 +1079,10 @@ class TestExternalSkillMutations: result = json.loads(raw) assert result["success"] is False - assert "manually authored" in result["error"].lower() + # Refusal must name the ownership reason and point at the supported way + # in (`hermes curator adopt`), not just say "no". + assert "not curator-managed" in result["error"].lower() + assert "curator adopt" in result["error"] def test_background_review_allows_agent_created_skill(self, tmp_path): """Agent-created skills (created_by='agent') are NOT blocked by the @@ -1111,10 +1120,168 @@ class TestExternalSkillMutations: reset_current_write_origin(token) result = json.loads(raw) - # Should not be blocked by the manual-skill guard (may be blocked by + # Should not be blocked by the ownership guard (may be blocked by # the consolidation-delete guard if absorbed_into is empty, but the - # manual-skill guard must not fire). - assert "manually authored" not in result.get("error", "").lower() + # ownership guard must not fire). + assert "not curator-managed" not in result.get("error", "").lower() + + +class TestBackgroundOwnershipPolicyConsistency: + """The autonomous write policy must not depend on its own side effects. + + Issue #67140: the ownership guard keyed on ``isinstance(usage_rec, dict)``, + so a local skill with NO usage record passed. The successful write then + called ``bump_patch()``, creating a ``created_by: null`` record — and the + identical write was refused from then on. "Allowed exactly once" is a race + with our own bookkeeping, not a policy. + """ + + @staticmethod + def _bg_patch(tmp_path, name, old, new): + from tools.skill_manager_tool import mark_background_review_skill_read + from tools.skill_provenance import ( + BACKGROUND_REVIEW, + reset_current_write_origin, + set_current_write_origin, + ) + + token = set_current_write_origin(BACKGROUND_REVIEW) + try: + mark_background_review_skill_read(tmp_path / name / "SKILL.md") + return json.loads(skill_manage( + action="patch", name=name, old_string=old, new_string=new, + )) + finally: + reset_current_write_origin(token) + + def test_missing_record_fails_closed_like_explicit_null(self, tmp_path, monkeypatch): + """Both unmanaged record shapes must produce the SAME verdict.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + with _skill_dir(tmp_path): + _create_skill("no-record", VALID_SKILL_CONTENT) + with patch("tools.skill_usage.load_usage", return_value={}): + missing = self._bg_patch( + tmp_path, "no-record", "Do the thing.", "Do the new thing.", + ) + with patch( + "tools.skill_usage.load_usage", + return_value={"no-record": {"created_by": None}}, + ): + null_rec = self._bg_patch( + tmp_path, "no-record", "Do the thing.", "Do the new thing.", + ) + + assert missing["success"] is False + assert null_rec["success"] is False + assert ("not curator-managed" in missing["error"].lower() + and "not curator-managed" in null_rec["error"].lower()) + + def test_repeated_identical_write_gets_the_same_answer(self, tmp_path, monkeypatch): + """The real #67140 shape: no stubbing of load_usage, so the first write's + telemetry side effect is live. Both attempts must agree.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + (tmp_path / ".hermes" / "skills").mkdir(parents=True, exist_ok=True) + with _skill_dir(tmp_path): + _create_skill("flip-skill", VALID_SKILL_CONTENT) + first = self._bg_patch( + tmp_path, "flip-skill", "Do the thing.", "Do the new thing.", + ) + second = self._bg_patch( + tmp_path, "flip-skill", "Do the thing.", "Do the new thing.", + ) + + assert first["success"] == second["success"], ( + "autonomous write policy flipped between two identical attempts: " + f"first={first.get('success')} second={second.get('success')}" + ) + assert first["success"] is False + + def test_refusal_points_at_the_supported_way_in(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + with _skill_dir(tmp_path): + _create_skill("no-record", VALID_SKILL_CONTENT) + with patch("tools.skill_usage.load_usage", return_value={}): + res = self._bg_patch( + tmp_path, "no-record", "Do the thing.", "Do the new thing.", + ) + assert "hermes curator adopt no-record" in res["error"] + + def test_foreground_write_to_unmanaged_skill_still_allowed(self, tmp_path, monkeypatch): + """Fail-closed applies to AUTONOMOUS writes only. A user-directed + foreground edit to their own skill must keep working.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + with _skill_dir(tmp_path): + _create_skill("no-record", VALID_SKILL_CONTENT) + with patch("tools.skill_usage.load_usage", return_value={}): + res = json.loads(skill_manage( + action="patch", name="no-record", + old_string="Do the thing.", new_string="Do the new thing.", + )) + assert res["success"] is True + + def test_adopted_skill_becomes_writable_by_autonomous_curation(self, tmp_path, monkeypatch): + """Adoption is the documented path from refused to allowed.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + with _skill_dir(tmp_path): + _create_skill("adopt-me", VALID_SKILL_CONTENT) + with patch("tools.skill_usage.load_usage", return_value={}): + before = self._bg_patch( + tmp_path, "adopt-me", "Do the thing.", "Do the new thing.", + ) + with patch( + "tools.skill_usage.load_usage", + return_value={"adopt-me": {"created_by": "agent"}}, + ), patch( + "tools.skill_usage.get_record", + side_effect=lambda n: {"created_by": "agent", "pinned": False}, + ): + after = self._bg_patch( + tmp_path, "adopt-me", "Do the thing.", "Do the new thing.", + ) + + assert before["success"] is False + assert after["success"] is True, after + + +class TestReviewPromptMatchesEnforcement: + """The session-review prompt must only ask for writes enforcement permits. + + Issue #67140 claims 1 and 3: the prompt told the reviewer to patch any + skill consulted in the conversation and stated pinned skills could be + improved, while the shared background guard refuses both. A prompt that + requests refused writes burns review turns on guaranteed failures. + """ + + @staticmethod + def _prompts(): + from agent import background_review as br + + out = [] + for attr in ("_SKILL_REVIEW_PROMPT", "_COMBINED_REVIEW_PROMPT"): + text = getattr(br, attr, "") + if text: + out.append((attr, text)) + assert out, "no review prompts found to check" + return out + + def test_prompts_do_not_claim_pinned_skills_are_patchable(self): + for attr, text in self._prompts(): + assert "CAN be improved" not in text, ( + f"{attr} still tells the reviewer pinned skills are patchable, " + "but _background_review_write_guard refuses pinned writes" + ) + + def test_prompts_list_pinned_and_user_owned_as_protected(self): + for attr, text in self._prompts(): + assert "PINNED skills" in text, f"{attr} omits the pin restriction" + assert "USER-OWNED skills" in text, f"{attr} omits the ownership restriction" + + def test_prompts_point_at_adopt_instead_of_patching(self): + for attr, text in self._prompts(): + assert "curator adopt" in text, ( + f"{attr} does not tell the reviewer what to recommend when the " + "right skill is user-owned" + ) # --------------------------------------------------------------------------- @@ -1309,6 +1476,15 @@ def _curator_pass(tmp_path, *, monkeypatch): (``get_hermes_home()``) resolves into the same tree the skill manager searches, and flips ``is_background_review()`` → True so the consolidation guard fires. + + Also stubs the ownership check to report every skill as curator-managed. + The ownership guard runs BEFORE the consolidation / read-before-write + guards these tests target, and since #67140 a skill with no usage record + fails closed — so without this, every test in this class would be refused + by ownership and never reach the guard under test. The real curator only + ever operates on managed sediment, so "managed" is the correct premise + here; tests that specifically exercise the ownership guard set their own + records instead. """ hermes_home = tmp_path / ".hermes" skills_root = hermes_home / "skills" @@ -1317,6 +1493,7 @@ def _curator_pass(tmp_path, *, monkeypatch): with patch("tools.skill_manager_tool.SKILLS_DIR", skills_root), \ patch("tools.skills_tool.SKILLS_DIR", skills_root), \ patch("agent.skill_utils.get_all_skills_dirs", return_value=[skills_root]), \ + patch("tools.skill_usage._is_curator_managed_record", return_value=True), \ patch("tools.skill_provenance.is_background_review", return_value=True): yield skills_root diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 88d7e47e17ae..c3899cb2ccee 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -380,21 +380,34 @@ def _background_review_write_guard( f"skill '{name}'." ), } - # Manually authored skills (created_by != "agent") are off-limits - # to autonomous curation. This prevents the LLM consolidation pass - # from archiving skills the user placed manually (e.g. via URL - # install or direct SKILL.md authoring), which lack the - # `created_by: "agent"` marker. + # Skills that are not curator-managed are off-limits to autonomous + # curation. This prevents the LLM consolidation pass from mutating + # skills the user owns (manually authored, URL-installed, or created by + # a foreground `skill_manage(create)` at the user's request), which lack + # the `created_by: "agent"` marker. + # + # A MISSING record and an explicit `created_by: null` must resolve + # IDENTICALLY (issue #67140). Keying on `isinstance(usage_rec, dict)` + # made the policy depend on the guard's own side effect: a local skill + # with no telemetry record passed, the successful write called + # bump_patch() which created a `created_by: null` record, and the very + # same write was refused from then on. "Allowed exactly once" is not a + # policy — it is a race with our own bookkeeping. Fail closed for both + # shapes; `hermes curator adopt ` is the supported way in. usage_data = skill_usage.load_usage() usage_rec = usage_data.get(name) - if isinstance(usage_rec, dict) and not skill_usage._is_curator_managed_record(usage_rec): + if not skill_usage._is_curator_managed_record(usage_rec): + if isinstance(usage_rec, dict): + _detail = f"created_by={usage_rec.get('created_by')!r}" + else: + _detail = "no usage record" return { "success": False, "error": ( f"Refusing background curator {action} for skill " - f"'{name}': the skill records show it is not agent-created " - f"(created_by={usage_rec.get('created_by')!r}). Manually authored " - f"skills are off-limits to autonomous curation." + f"'{name}': the skill is not curator-managed ({_detail}). " + "User-owned skills are off-limits to autonomous curation. " + f"Run `hermes curator adopt {name}` to opt it in." ), } except Exception: diff --git a/tools/skill_usage.py b/tools/skill_usage.py index f5d5c865dd60..7f519a5d99bd 100644 --- a/tools/skill_usage.py +++ b/tools/skill_usage.py @@ -479,12 +479,38 @@ def is_curation_eligible(skill_name: str, skill_path: Optional[Path] = None) -> def _is_curator_managed_record(record: Any) -> bool: - """Return True when a usage record opts a skill into curator management.""" + """Return True when a usage record opts a skill into curator management. + + NAMING (issue #67140): the on-disk field is ``created_by``, which reads + like provenance but is consumed as a **curator-management opt-in policy + flag**. The two are not the same question: + + * provenance = "who authored this file" — historical fact, and for records + written before the marker existed it is simply unrecoverable. + * management = "may autonomous curation mutate/archive this" — a policy + decision the user can change at any time via ``hermes curator adopt``. + + ``created_by: "agent"`` therefore means "curator-managed", NOT "proof the + agent wrote it". The field name is retained because it is already on disk + in every user's ``.usage.json``; renaming it would strand those records. + Read it as policy, and prefer ``is_curator_managed()`` at call sites so the + intent is unambiguous. + """ if not isinstance(record, dict): return False return record.get("created_by") == "agent" or record.get("agent_created") is True +def is_curator_managed(skill_name: str) -> bool: + """Whether *skill_name* is opted into curator management. + + Policy-intent alias for the ``created_by``-marker check, so call sites read + as the question they are actually asking (see ``_is_curator_managed_record`` + for why the stored field name says "created_by"). + """ + return _is_curator_managed_record(load_usage().get(skill_name)) + + def list_unmanaged_skill_names() -> List[str]: """Enumerate curation-ELIGIBLE skills that carry no provenance marker. diff --git a/website/docs/user-guide/features/curator.md b/website/docs/user-guide/features/curator.md index 6e7e8ac9f42c..2497aca61b3c 100644 --- a/website/docs/user-guide/features/curator.md +++ b/website/docs/user-guide/features/curator.md @@ -105,6 +105,7 @@ hermes curator pin # never auto-transition this skill hermes curator unpin hermes curator adopt # hand an unmanaged skill to the curator hermes curator adopt --all-unmanaged # hand over every unmanaged skill +hermes curator list-unmanaged # itemize skills with no provenance marker hermes curator restore # move an archived skill back to active hermes curator list-archived # list skills currently in ~/.hermes/skills/.archive/ hermes curator archive # manually archive a single skill now @@ -202,7 +203,8 @@ A large library can therefore look fully curated while most of it is untouchable. `adopt` closes that gap by **declaration**: ```bash -hermes curator adopt [ ...] # hand specific skills over +hermes curator list-unmanaged # itemize them, with reasons +hermes curator adopt [ ...] # hand specific skills over hermes curator adopt --all-unmanaged --dry-run # preview the full list hermes curator adopt --all-unmanaged # hand over everything (prompts) hermes curator adopt --all-unmanaged --yes # skip the prompt @@ -214,6 +216,21 @@ existing `last_activity_at`, so handing over a library you already stopped using does not buy it a fresh 90-day window. Expect adopted long-idle skills to go `stale` (or `archived`) on the next pass; that is the point. +Adoption is also what unblocks autonomous *improvement*. The background review +fork refuses to patch a skill that isn't curator-managed, so if it notices one +of your skills is outdated it will say so and recommend adoption rather than +edit it. Foreground (user-directed) edits are never affected — you and the +agent can always edit your own skills on request. + +:::note `created_by` is a policy flag, not a provenance claim +The stored field is named `created_by`, but it is consumed as "may autonomous +curation touch this?" — not "who wrote this file". Those are different +questions, and for records predating the marker the authorship answer is simply +unrecoverable. The name is kept because it is already on disk in every +`.usage.json`; read it as policy. `hermes curator adopt` changes the policy, and +says nothing about who authored the file. +::: + :::note Provenance is declared, never inferred Adoption is deliberately manual. Telemetry cannot establish authorship: a skill with thousands of patches proves the agent **maintains** it, not that the agent