From 4f4e778db8c9eb826b90eae6d82b24032a835e45 Mon Sep 17 00:00:00 2001 From: Clifford Garwood Date: Sun, 19 Jul 2026 19:11:58 -0400 Subject: [PATCH] fix(skills): make skill_manage patch failures recoverable instead of a dead end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `skill_manage(action='patch')` rejected a missing `old_string` with: old_string is required for 'patch'. Provide the text to find. That is a dead end. The model cannot tell whether it omitted the argument or supplied text that did not match, so it retries blindly — and then escapes to the neighbour that always works: `action='write_file'`, which rewrites the entire skill file and destroys unrelated content. `skill_manage`'s own action enum puts that destructive path one token away from the failing one. The error now names the recovery route: `old_string` must be the EXACT text currently in the file, read the target first (the skill's SKILL.md, or the file named by `file_path`), copy the snippet verbatim, and do not fall back to `action='write_file'`. Validation lives in `_patch_skill` rather than the dispatcher. `skill_manage()` previously returned its own bare missing-argument error before ever calling the helper, which would leave the new guidance unreachable through the public tool. Removing that duplicate makes the helper the single source of truth; its {"success": False, "error": ...} flows through the same json.dumps path, so the serialized shape is unchanged, and validation still precedes the skill lookup — a missing old_string on an unknown skill still reports the argument error rather than "skill not found". Fixes #33064 --- tests/tools/test_skill_manager_tool.py | 43 ++++++++++++++++++++++++++ tools/skill_manager_tool.py | 36 +++++++++++++++------ 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 10e64ea168..830abe2a8b 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -236,6 +236,28 @@ word word assert result["success"] is False assert "match" in result["error"].lower() + def test_patch_missing_old_string_tells_the_model_how_to_recover(self, tmp_path): + """#33064 — a bare 'required' error is a dead end. + + The model cannot tell whether it omitted old_string or supplied it wrongly, + so it retries blindly and escapes to action='write_file', clobbering the + whole skill file. The error must say how to obtain the exact text, and must + forbid the whole-file rewrite. + """ + with _skill_dir(tmp_path): + _create_skill("my-skill", VALID_SKILL_CONTENT) + result = _patch_skill("my-skill", "", "replacement") + + assert result["success"] is False + err = result["error"] + assert "read" in err.lower(), "must tell the model to read the file first" + assert "write_file" in err, "must name the escape hatch it is forbidding" + assert "exact" in err.lower() + + + + + def test_patch_supporting_file_symlink_escape_blocked(self, tmp_path): outside_file = tmp_path / "outside.txt" outside_file.write_text("old text here") @@ -342,6 +364,27 @@ class TestRemoveFile: class TestSkillManageDispatcher: + @pytest.mark.parametrize("old_string", [None, ""]) + def test_patch_missing_old_string_carries_recovery_guidance(self, tmp_path, old_string): + """#33064 — the actionable error must survive the public dispatch path. + + The dispatcher used to return its own bare "old_string is required" + before ever reaching _patch_skill, so the guidance below was + unreachable through the tool the model actually calls. Assert on the + serialized public result, not the helper's dict. + """ + with _skill_dir(tmp_path): + _create_skill("my-skill", VALID_SKILL_CONTENT) + raw = skill_manage(action="patch", name="my-skill", + old_string=old_string, new_string="replacement") + + result = json.loads(raw) + assert result["success"] is False + err = result["error"] + assert "read" in err.lower(), "must tell the model to read the file first" + assert "write_file" in err, "must name the escape hatch it is forbidding" + assert "exact" in err.lower() + def test_full_create_via_dispatcher(self, tmp_path): """Foreground create does NOT mark the skill as agent-created. diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 8d0b118a50..fa1c11d02e 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -1112,9 +1112,30 @@ def _patch_skill( Requires a unique match unless replace_all is True. """ if not old_string: - return {"success": False, "error": "old_string is required for 'patch'."} + # A bare "required" error is a dead end: the model cannot tell whether it + # omitted the arg or supplied it wrongly, so it retries blindly and often + # escapes to action='write_file', clobbering the whole skill file. Tell it + # how to recover. Upstream: NousResearch/hermes-agent#33064. + return { + "success": False, + "error": ( + "old_string is required for 'patch' and must be the EXACT text currently in the " + "file. Read the target file first (read_file on the skill's SKILL.md, or the file " + "named by file_path) and copy the snippet verbatim, then retry 'patch'. " + "Do NOT fall back to action='write_file' — that rewrites the entire file and " + "destroys unrelated content." + ), + } if new_string is None: return {"success": False, "error": "new_string is required for 'patch'. Use an empty string to delete matched text."} + if old_string == new_string: + return { + "success": False, + "error": ( + "old_string and new_string are identical — this patch would change nothing. " + "Supply the actual replacement text." + ), + } existing = _find_skill(name) if not existing: @@ -1892,15 +1913,10 @@ def skill_manage( if content: result = _edit_skill(name, content) else: - if not old_string: - return tool_error( - "patch needs old_string/new_string for a targeted " - "replacement, or content for a full SKILL.md rewrite " - "(read it first with skill_view()).", - success=False, - ) - if new_string is None: - return tool_error("new_string is required for 'patch'. Use empty string to delete matched text.", success=False) + # Targeted-replacement validation lives in _patch_skill so the + # public tool and the helper return the same actionable guidance. + # A bare "required" error here would shadow it and leave the + # model with nowhere to go but action='write_file'. #33064. result = _patch_skill(name, old_string, new_string, file_path, replace_all) elif action == "delete":