From 9b909115fc129c073a00ac4445531564cb373fbb Mon Sep 17 00:00:00 2001 From: webtecnica Date: Sat, 18 Jul 2026 16:37:59 -0300 Subject: [PATCH] fix(skills): fail closed on unknown curator ownership - require positive agent ownership proof before any background-review mutation - fail closed when usage record is missing, malformed, or unreadable - preserve foreground user-directed mutations and valid agent-owned curator flows - cover patch, edit, delete, write_file, and remove_file end to end Closes #67073 --- tests/tools/test_skill_manager_tool.py | 110 +++++++++++++++++++++++-- tools/skill_manager_tool.py | 10 ++- 2 files changed, 111 insertions(+), 9 deletions(-) diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 8c9c8388439..c4e9a8ddae3 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -1084,6 +1084,91 @@ class TestExternalSkillMutations: assert "not curator-managed" in result["error"].lower() assert "curator adopt" in result["error"] + @pytest.mark.parametrize( + ("action", "kwargs"), + [ + ("patch", {"old_string": "Do the thing.", "new_string": "Changed."}), + ("edit", {"content": VALID_SKILL_CONTENT_2}), + ("delete", {}), + ( + "write_file", + {"file_path": "references/new.md", "file_content": "new"}, + ), + ("remove_file", {"file_path": "references/existing.md"}), + ], + ) + def test_background_review_fails_closed_without_agent_ownership_record( + self, tmp_path, action, kwargs + ): + """Every autonomous mutation requires positive agent ownership proof.""" + from tools.skill_provenance import ( + BACKGROUND_REVIEW, + reset_current_write_origin, + set_current_write_origin, + ) + + with _skill_dir(tmp_path): + _create_skill("manual-skill", VALID_SKILL_CONTENT) + support = tmp_path / "manual-skill" / "references" / "existing.md" + support.parent.mkdir(parents=True) + support.write_text("keep", encoding="utf-8") + before = { + path.relative_to(tmp_path): path.read_bytes() + for path in tmp_path.rglob("*") + if path.is_file() + } + + token = set_current_write_origin(BACKGROUND_REVIEW) + try: + with patch("tools.skill_usage.load_usage", return_value={}): + raw = skill_manage(action=action, name="manual-skill", **kwargs) + finally: + reset_current_write_origin(token) + + after = { + path.relative_to(tmp_path): path.read_bytes() + for path in tmp_path.rglob("*") + if path.is_file() + } + + result = json.loads(raw) + assert result["success"] is False + # Wording landed as "not curator-managed" (#67140) rather than + # "ownership"; the contract asserted here is the refusal + zero writes. + assert "not curator-managed" in result["error"].lower() + assert before == after + + def test_background_review_fails_closed_when_ownership_lookup_errors(self, tmp_path): + from tools.skill_provenance import ( + BACKGROUND_REVIEW, + reset_current_write_origin, + set_current_write_origin, + ) + + with _skill_dir(tmp_path): + _create_skill("manual-skill", VALID_SKILL_CONTENT) + token = set_current_write_origin(BACKGROUND_REVIEW) + try: + with patch( + "tools.skill_usage.load_usage", + side_effect=ValueError("corrupt usage data"), + ): + raw = skill_manage( + action="patch", + name="manual-skill", + old_string="Do the thing.", + new_string="Changed.", + ) + finally: + reset_current_write_origin(token) + + result = json.loads(raw) + assert result["success"] is False + assert "ownership" in result["error"].lower() + assert "Do the thing." in ( + tmp_path / "manual-skill" / "SKILL.md" + ).read_text(encoding="utf-8") + def test_background_review_allows_agent_created_skill(self, tmp_path): """Agent-created skills (created_by='agent') are NOT blocked by the manual-skill guard — they remain eligible for autonomous curation.""" @@ -1514,6 +1599,15 @@ def _skill_content(name: str) -> str: "Step 1: Do the thing.\n" ) +def _create_curator_skill(name: str, content: str): + """Create a skill and record the agent ownership a real curator create has.""" + from tools.skill_usage import mark_agent_created + + result = _create_skill(name, content) + assert result["success"] is True, result + mark_agent_created(name) + return result + class TestCuratorConsolidationDeleteGuard: """The curator's LLM consolidation pass must fail CLOSED on unverified @@ -1528,7 +1622,7 @@ class TestCuratorConsolidationDeleteGuard: def test_bare_prune_during_curator_pass_refused(self, tmp_path, monkeypatch): with _curator_pass(tmp_path, monkeypatch=monkeypatch) as skills_root: - _create_skill("active-skill", VALID_SKILL_CONTENT) + _create_curator_skill("active-skill", VALID_SKILL_CONTENT) result = _delete_skill("active-skill", absorbed_into="") assert result["success"] is False assert result.get("_fail_closed") is True @@ -1537,7 +1631,7 @@ class TestCuratorConsolidationDeleteGuard: def test_omitted_absorbed_into_during_curator_pass_refused(self, tmp_path, monkeypatch): with _curator_pass(tmp_path, monkeypatch=monkeypatch) as skills_root: - _create_skill("active-skill", VALID_SKILL_CONTENT) + _create_curator_skill("active-skill", VALID_SKILL_CONTENT) result = _delete_skill("active-skill") # absorbed_into omitted assert result["success"] is False assert result.get("_fail_closed") is True @@ -1545,7 +1639,7 @@ class TestCuratorConsolidationDeleteGuard: def test_whitespace_absorbed_into_during_curator_pass_refused(self, tmp_path, monkeypatch): with _curator_pass(tmp_path, monkeypatch=monkeypatch) as skills_root: - _create_skill("active-skill", VALID_SKILL_CONTENT) + _create_curator_skill("active-skill", VALID_SKILL_CONTENT) result = _delete_skill("active-skill", absorbed_into=" ") assert result["success"] is False assert result.get("_fail_closed") is True @@ -1553,8 +1647,8 @@ class TestCuratorConsolidationDeleteGuard: def test_verified_consolidation_archives_recoverably(self, tmp_path, monkeypatch): with _curator_pass(tmp_path, monkeypatch=monkeypatch) as skills_root: - _create_skill("umbrella", _skill_content("umbrella")) - _create_skill("narrow", _skill_content("narrow")) + _create_curator_skill("umbrella", _skill_content("umbrella")) + _create_curator_skill("narrow", _skill_content("narrow")) result = _delete_skill("narrow", absorbed_into="umbrella") assert result["success"] is True, result assert result.get("_archived") is True @@ -1569,7 +1663,7 @@ class TestCuratorConsolidationDeleteGuard: # The pre-existing target-existence check fires before the recoverable # archive — a hallucinated umbrella is refused and the skill stays put. with _curator_pass(tmp_path, monkeypatch=monkeypatch) as skills_root: - _create_skill("narrow", VALID_SKILL_CONTENT) + _create_curator_skill("narrow", VALID_SKILL_CONTENT) result = _delete_skill("narrow", absorbed_into="ghost-umbrella") assert result["success"] is False assert "does not exist" in result["error"] @@ -1608,7 +1702,7 @@ class TestCuratorConsolidationDeleteGuard: _reset_background_review_read_marks() with _curator_pass(tmp_path, monkeypatch=monkeypatch): - _create_skill("reviewed", _skill_content("reviewed")) + _create_curator_skill("reviewed", _skill_content("reviewed")) blocked = json.loads(skill_manage( action="patch", @@ -1638,7 +1732,7 @@ class TestCuratorConsolidationDeleteGuard: _reset_background_review_read_marks() with _curator_pass(tmp_path, monkeypatch=monkeypatch): - _create_skill("reviewed", _skill_content("reviewed")) + _create_curator_skill("reviewed", _skill_content("reviewed")) ref = tmp_path / ".hermes" / "skills" / "reviewed" / "references" ref.mkdir() (ref / "workflow.md").write_text("old workflow\n", encoding="utf-8") diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index c3899cb2cce..2cd20bde2ea 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -411,7 +411,15 @@ def _background_review_write_guard( ), } except Exception: - logger.debug("owned skill guard lookup failed for %s", name, exc_info=True) + logger.warning("owned skill guard lookup failed for %s", name, exc_info=True) + return { + "success": False, + "error": ( + f"Refusing background curator {action} for skill '{name}': " + "agent ownership could not be verified because the provenance " + "record is unavailable or unreadable." + ), + } return None