fix(skills): make skill_manage patch failures recoverable instead of a dead end

`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

@@ -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.
 

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":