fix(skills): a failed rollback restore keeps the skill and the snapshots

Rollback removed the live skill directory before restoring its
snapshot. When copytree then failed (disk full, locked file, path too
long on Windows) the except only added a note, and the finally deleted
the snapshot directory too, so nothing survived: the skill was gone
with a success-shaped error payload.

The broken state is now renamed aside first and deleted only after the
snapshot is restored. If the restore still fails, the broken state is
renamed back, so the worst outcome is the half applied batch instead of
no skill at all. When rollback reports any failure the snapshots are
kept on disk and their location is logged, instead of being deleted by
the finally.

Follow-up to #97692, same batch executor.

tests/tools/test_skill_manage_batch.py

@@ -176,6 +176,42 @@ class TestSkillManageBatch(unittest.TestCase):
         self.assertNotIn("Step A.", content)
         self.assertFalse(os.path.exists(os.path.join(self.home, "skills", "beta")))
 
+    def test_failed_restore_never_destroys_the_skill(self):
+        """Rollback used to rmtree the live skill directory BEFORE
+        copytree restored the snapshot. When copytree failed (disk full,
+        locked file) the except only appended a note and the finally then
+        deleted the snapshot too: nothing survived. The broken state must
+        be moved aside and only deleted once the restore succeeded."""
+        from unittest.mock import patch as _patch
+
+        import shutil as _shutil
+
+        self._call("probe", [{"action": "create", "content": SK.format(n="probe")}])
+        state = {"n": 0}
+        real_copytree = _shutil.copytree
+
+        def flaky_copytree(src, dst, *a, **k):
+            state["n"] += 1
+            if state["n"] == 2:  # call 1 snapshots, call 2 is the restore
+                raise OSError("disk full")
+            return real_copytree(src, dst, *a, **k)
+
+        with _patch("shutil.copytree", side_effect=flaky_copytree):
+            r = self._call("probe", [
+                {"action": "patch",
+                 "old_string": "Step 1.", "new_string": "Step ONE."},
+                {"action": "write_file",
+                 "file_path": "bad/nope.md", "file_content": "x"},
+            ])
+        self.assertFalse(r["success"], r)
+        self.assertIn("ROLLBACK FAILED", r["error"])
+        # The skill directory was NOT destroyed by the failed rollback:
+        # the half applied state survives instead of nothing at all.
+        skill_md = os.path.join(self.home, "skills", "probe", "SKILL.md")
+        self.assertTrue(os.path.exists(skill_md))
+        content = open(skill_md).read()
+        self.assertIn("Step ONE.", content)
+
     def test_single_op_path_unchanged(self):
         self._call("probe", [{"action": "create", "content": SK.format(n="probe")}])
         raw = self.smt.skill_manage(

tools/skill_manager_tool.py

@@ -1699,6 +1699,8 @@ def _skill_manage_batch(
                 return tool_error(f"Could not snapshot '{nm}' for atomic batch: {exc}", success=False)
         snapshots[nm] = (pre_dir, snap)
 
+    rollback_failed = False
+
     def _rollback() -> str:
         notes = []
         for nm, (pre_dir, snap) in snapshots.items():
@@ -1707,13 +1709,34 @@ def _skill_manage_batch(
                 post_dir = Path(post["path"]) if post else None
                 if snap is not None:
                     if post_dir is not None and post_dir.is_dir():
-                        shutil.rmtree(post_dir)
-                    shutil.copytree(snap, pre_dir)
+                        # Never destroy the only other copy before the
+                        # restore lands. Deleting first turned a failed
+                        # copytree (disk full, locked file) into total
+                        # skill loss once the finally below removed the
+                        # snapshot too. Move the broken state aside, and
+                        # delete it only after the snapshot is back.
+                        aside = post_dir.with_name(post_dir.name + ".rollback-broken")
+                        shutil.rmtree(aside, ignore_errors=True)
+                        post_dir.rename(aside)
+                        try:
+                            shutil.copytree(snap, pre_dir)
+                        except Exception:
+                            # Restore failed: put the broken state back so
+                            # the skill survives (half applied) rather than
+                            # leaving nothing.
+                            shutil.rmtree(pre_dir, ignore_errors=True)
+                            aside.rename(pre_dir)
+                            raise
+                        shutil.rmtree(aside, ignore_errors=True)
+                    else:
+                        shutil.copytree(snap, pre_dir)
                 elif post_dir is not None and post_dir.is_dir():
                     # Batch created this skill: remove the partial result.
                     shutil.rmtree(post_dir)
             except Exception as exc:  # noqa: BLE001
                 notes.append(f"ROLLBACK FAILED for '{nm}' ({exc})")
+        nonlocal rollback_failed
+        rollback_failed = bool(notes)
         return "; ".join(notes) if notes else "all touched skills rolled back"
 
     # --- execute ops through the normal single-op path (gate bypassed:
@@ -1765,7 +1788,16 @@ def _skill_manage_batch(
                             "success": True})
     finally:
         _skill_gate_bypass.reset(token)
-        shutil.rmtree(snap_root, ignore_errors=True)
+        if rollback_failed:
+            # Keep the snapshots so the operator can still recover by
+            # hand. Deleting them here is what turned one failed restore
+            # into permanent skill loss.
+            logger.warning(
+                "skill_manage batch rollback failed, snapshots kept at %s",
+                snap_root,
+            )
+        else:
+            shutil.rmtree(snap_root, ignore_errors=True)
 
     return json.dumps(
         {"success": True, "operations_applied": len(results),