Merge pull request #3753 from rudycelekli/fix/cow-net-new-skill-replacement-rollback-20261013

fix(skills): preserve installed skills during replacement failures

agent/skills/service.py

@@ -11,7 +11,9 @@ import os
 import re
 import shutil
 import tempfile
+import uuid
 from typing import List, Optional
+from common.directory_replace import replace_directory
 from common.log import logger
 from agent.skills.archive import extract_archive
 from agent.skills.manager import SkillManager, apply_install_fields
@@ -503,10 +505,11 @@ class SkillService:
 
         skill_dir = self._safe_skill_dir(name)
 
-        tmp_dir = skill_dir + ".tmp"
-        if os.path.exists(tmp_dir):
-            shutil.rmtree(tmp_dir)
-        os.makedirs(tmp_dir, exist_ok=True)
+        parent = os.path.dirname(skill_dir)
+        os.makedirs(parent, exist_ok=True)
+        # Not mkdtemp: this directory becomes the skill dir and must keep umask permissions.
+        tmp_dir = os.path.join(parent, f".{os.path.basename(skill_dir)}.download-{uuid.uuid4().hex[:8]}")
+        os.makedirs(tmp_dir)
 
         try:
             for file_info in files:
@@ -517,13 +520,9 @@ class SkillService:
                     continue
                 dest = self._safe_file_path(tmp_dir, rel_path)
                 self._download_file(url, dest)
-        except Exception:
+            replace_directory(tmp_dir, skill_dir, move_source=True)
+        finally:
             shutil.rmtree(tmp_dir, ignore_errors=True)
-            raise
-
-        if os.path.exists(skill_dir):
-            shutil.rmtree(skill_dir)
-        os.rename(tmp_dir, skill_dir)
 
         logger.info(f"[SkillService] add: skill '{name}' installed via url ({len(files)} files)")
 
@@ -568,9 +567,7 @@ class SkillService:
                 if os.path.isdir(single):
                     extract_dir = single
 
-            if os.path.exists(skill_dir):
-                shutil.rmtree(skill_dir)
-            shutil.copytree(extract_dir, skill_dir)
+            replace_directory(extract_dir, skill_dir)
 
         logger.info(f"[SkillService] add: skill '{name}' installed via package ({url})")
 

cli/commands/skill.py

@@ -73,6 +73,13 @@ def _target_skills_dir(agent_id: str = None) -> str:
     return _staging_dir.get() or get_skills_dir(agent_id)
 
 
+def _replace_skill_dir(source: str, target: str) -> None:
+    _ensure_project_on_path()
+    from common.directory_replace import replace_directory
+
+    replace_directory(source, target)
+
+
 def _parse_github_url(url: str):
     """Parse a full GitHub URL into (owner, repo, branch, subpath).
 
@@ -328,9 +335,7 @@ def _batch_install_skills(discovered, spec, skills_dir, source, result: InstallR
             continue
         target_dir = os.path.join(skills_dir, safe_name)
         _check_skill_copy_paths(sdir, target_dir)
-        if os.path.exists(target_dir):
-            shutil.rmtree(target_dir)
-        shutil.copytree(sdir, target_dir)
+        _replace_skill_dir(sdir, target_dir)
         _register_installed_skill(safe_name, source=source, display_name=display_name if single else "", agent_id=agent_id)
         result.installed.append(safe_name)
         result.messages.append(f"  + {safe_name}")
@@ -366,9 +371,7 @@ def _install_local(path: str, result: InstallResult, agent_id: str = None):
         _check_skill_name(skill_name)
         target_dir = os.path.join(skills_dir, skill_name)
         _check_skill_copy_paths(path, target_dir)
-        if os.path.exists(target_dir):
-            shutil.rmtree(target_dir)
-        shutil.copytree(path, target_dir)
+        _replace_skill_dir(path, target_dir)
         _register_installed_skill(skill_name, source="local", agent_id=agent_id)
         result.installed.append(skill_name)
         result.messages.append(f"Installed '{skill_name}' from local path.")
@@ -555,18 +558,14 @@ def _install_targz_bytes(content: bytes, name: str, skills_dir: str, result: Ins
             if not _SAFE_NAME_RE.match(safe_name):
                 safe_name = name
             target = os.path.join(skills_dir, safe_name)
-            if os.path.exists(target):
-                shutil.rmtree(target)
-            shutil.copytree(sdir, target)
+            _replace_skill_dir(sdir, target)
             _register_installed_skill(safe_name, source=source_label, agent_id=agent_id)
             result.installed.append(safe_name)
             result.messages.append(f"Installed '{safe_name}' from URL.")
             return
 
         target = os.path.join(skills_dir, name)
-        if os.path.exists(target):
-            shutil.rmtree(target)
-        shutil.copytree(pkg_root, target)
+        _replace_skill_dir(pkg_root, target)
         _register_installed_skill(name, source=source_label, agent_id=agent_id)
         result.installed.append(name)
         result.messages.append(f"Installed '{name}' from URL.")
@@ -1212,9 +1211,7 @@ def commit_staged(staging_dir: str, names=None, agent_id: str = None) -> list:
         if not _SAFE_NAME_RE.match(entry):
             continue
         target = os.path.join(skills_dir, entry)
-        if os.path.exists(target):
-            shutil.rmtree(target)
-        shutil.copytree(src, target)
+        _replace_skill_dir(src, target)
         meta = config.get(entry) if isinstance(config.get(entry), dict) else {}
         _register_installed_skill(
             entry,
@@ -1536,9 +1533,7 @@ def _install_github(spec, result: InstallResult, subpath=None, skill_name=None,
             api_dest = os.path.join(api_tmp, skill_name)
             os.makedirs(api_dest)
             _download_github_dir(owner, repo, branch, subpath.strip("/"), api_dest)
-            if os.path.exists(target_dir):
-                shutil.rmtree(target_dir)
-            shutil.copytree(api_dest, target_dir)
+            _replace_skill_dir(api_dest, target_dir)
         _register_installed_skill(skill_name, source=source, agent_id=agent_id)
         result.installed.append(skill_name)
         result.messages.append(f"Installed '{skill_name}' from GitHub.")
@@ -1562,9 +1557,7 @@ def _install_from_repo_root(repo_root, spec, subpath, skill_name, skills_dir, so
             _check_skill_name(skill_name)
 
             target_dir = os.path.join(skills_dir, skill_name)
-            if os.path.exists(target_dir):
-                shutil.rmtree(target_dir)
-            shutil.copytree(source_dir, target_dir)
+            _replace_skill_dir(source_dir, target_dir)
             _register_installed_skill(skill_name, source=source, agent_id=agent_id)
             result.installed.append(skill_name)
             result.messages.append(f"Installed '{skill_name}' from {source}.")
@@ -1586,9 +1579,7 @@ def _install_from_repo_root(repo_root, spec, subpath, skill_name, skills_dir, so
                 skill_name = spec.split("/")[-1]
                 _check_skill_name(skill_name)
             target_dir = os.path.join(skills_dir, skill_name)
-            if os.path.exists(target_dir):
-                shutil.rmtree(target_dir)
-            shutil.copytree(repo_root, target_dir)
+            _replace_skill_dir(repo_root, target_dir)
             _register_installed_skill(skill_name, source=source, agent_id=agent_id)
             result.installed.append(skill_name)
             result.messages.append(f"Installed '{skill_name}' from {source}.")
@@ -1681,9 +1672,7 @@ def _install_zip_bytes(content, name, skills_dir, result: InstallResult = None,
             if not _SAFE_NAME_RE.match(safe_name):
                 safe_name = name
             target = os.path.join(skills_dir, safe_name)
-            if os.path.exists(target):
-                shutil.rmtree(target)
-            shutil.copytree(sdir, target)
+            _replace_skill_dir(sdir, target)
             _register_installed_skill(safe_name, source=source_label, display_name=display_name, agent_id=agent_id)
             if result is not None:
                 result.installed.append(safe_name)
@@ -1691,9 +1680,7 @@ def _install_zip_bytes(content, name, skills_dir, result: InstallResult = None,
             return
 
         target = os.path.join(skills_dir, name)
-        if os.path.exists(target):
-            shutil.rmtree(target)
-        shutil.copytree(pkg_root, target)
+        _replace_skill_dir(pkg_root, target)
 
 
 

common/directory_replace.py

@@ -0,0 +1,56 @@
+"""Replace a directory without discarding it before the new copy is ready."""
+
+import os
+import shutil
+import tempfile
+
+from common.log import logger
+
+def replace_directory(source: str, target: str, move_source: bool = False) -> None:
+    source = os.path.realpath(source)
+    target = os.path.abspath(target)
+    real_target = os.path.realpath(target)
+    try:
+        shared = os.path.commonpath([os.path.normcase(source), os.path.normcase(real_target)])
+    except ValueError:
+        shared = None
+    if shared in (os.path.normcase(source), os.path.normcase(real_target)):
+        raise ValueError("source and target directories overlap")
+    _replace(source, target, move_source)
+
+
+def _replace(source: str, target: str, move_source: bool) -> None:
+    # rmtree previously refused a target symlink. Keep that boundary so a
+    # planted skill-directory alias cannot replace an unrelated directory.
+    if os.path.islink(target):
+        raise OSError("cannot replace a directory symlink")
+    if os.path.exists(target) and not os.path.isdir(target):
+        raise NotADirectoryError(target)
+    parent = os.path.dirname(target)
+    os.makedirs(parent, exist_ok=True)
+    stage_root = tempfile.mkdtemp(prefix=f".{os.path.basename(target)}.replace-", dir=parent)
+    staged = os.path.join(stage_root, "new")
+    backup = os.path.join(stage_root, "previous")
+    retain_backup = False
+    try:
+        if move_source:
+            os.rename(source, staged)
+        else:
+            shutil.copytree(source, staged)
+        if os.path.exists(target):
+            os.rename(target, backup)
+        try:
+            os.rename(staged, target)
+        except BaseException:
+            if os.path.exists(backup):
+                try:
+                    os.rename(backup, target)
+                except BaseException:
+                    retain_backup = True
+                    logger.error(f"[DirectoryReplace] Could not restore directory; backup retained at {backup}")
+            # Re-raise the original promotion failure even if restoring the
+            # previous directory also failed; its backup remains recoverable.
+            raise
+    finally:
+        if not retain_backup:
+            shutil.rmtree(stage_root, ignore_errors=True)

tests/test_directory_replace.py

@@ -0,0 +1,93 @@
+"""Native rollback, ownership and staging controls for directory replacement."""
+
+import os
+import errno
+from pathlib import Path
+
+import pytest
+
+from common.directory_replace import replace_directory
+
+
+def _directory(path, contents):
+    path.mkdir()
+    (path / "notes.txt").write_text(contents, encoding="utf-8")
+    return path
+
+
+@pytest.mark.parametrize("move", [False, True])
+def test_complete_replacement_retains_only_the_new_target(tmp_path, move):
+    source = _directory(tmp_path / "source", "new notes")
+    target = _directory(tmp_path / "target", "old notes")
+    replace_directory(str(source), str(target), move_source=move)
+    assert (target / "notes.txt").read_text() == "new notes"
+    assert source.exists() is not move
+    assert list(tmp_path.glob(".target.replace-*")) == []
+
+
+def test_failed_restoration_keeps_the_previous_directory_for_recovery(tmp_path, monkeypatch):
+    source = _directory(tmp_path / "source", "new notes")
+    target = _directory(tmp_path / "target", "old notes")
+    rename = os.rename
+
+    def refuse_target(src, dst):
+        if Path(dst) == target:
+            raise PermissionError("target temporarily unavailable")
+        return rename(src, dst)
+
+    monkeypatch.setattr(os, "rename", refuse_target)
+    with pytest.raises(PermissionError, match="temporarily unavailable"):
+        replace_directory(str(source), str(target))
+    preserved = list(tmp_path.glob(".target.replace-*/previous/notes.txt"))
+    assert len(preserved) == 1
+    assert preserved[0].read_text() == "old notes"
+    assert (source / "notes.txt").read_text() == "new notes"
+
+
+def test_failed_restoration_preserves_the_original_promotion_exception(tmp_path, monkeypatch):
+    source = _directory(tmp_path / "source", "new notes")
+    target = _directory(tmp_path / "target", "old notes")
+    rename = os.rename
+    promotion_error = PermissionError("promotion denied")
+    rollback_error = OSError(errno.ENOSPC, "restoration denied")
+
+    def fail_publication_and_restoration(src, dst):
+        if Path(dst) == target:
+            raise rollback_error if Path(src).name == "previous" else promotion_error
+        return rename(src, dst)
+
+    monkeypatch.setattr(os, "rename", fail_publication_and_restoration)
+    with pytest.raises(PermissionError) as failed:
+        replace_directory(str(source), str(target))
+    assert failed.value is promotion_error
+    preserved = list(tmp_path.glob(".target.replace-*/previous/notes.txt"))
+    assert len(preserved) == 1
+    assert preserved[0].read_text() == "old notes"
+    assert (source / "notes.txt").read_text() == "new notes"
+
+
+def test_target_alias_cannot_replace_a_directory_elsewhere(tmp_path):
+    source = _directory(tmp_path / "source", "new notes")
+    outside = _directory(tmp_path / "elsewhere", "unrelated notes")
+    alias = tmp_path / "target"
+    try:
+        alias.symlink_to(outside, target_is_directory=True)
+    except OSError:
+        pytest.skip("directory symlinks unavailable")
+    with pytest.raises(OSError, match="symlink"):
+        replace_directory(str(source), str(alias))
+    assert alias.is_symlink()
+    assert (outside / "notes.txt").read_text() == "unrelated notes"
+
+
+@pytest.mark.parametrize("direction", ["same", "source_inside", "target_inside"])
+def test_overlapping_paths_leave_all_input_files_unchanged(tmp_path, direction):
+    root = _directory(tmp_path / "root", "original notes")
+    child = _directory(root / "child", "child notes")
+    source, target = {
+        "same": (root, root), "source_inside": (child, root), "target_inside": (root, child),
+    }[direction]
+    original = {p.relative_to(root): p.read_bytes() for p in root.rglob("*") if p.is_file()}
+    with pytest.raises(ValueError, match="overlap"):
+        replace_directory(str(source), str(target))
+    assert {p.relative_to(root): p.read_bytes() for p in root.rglob("*") if p.is_file()} == original

tests/test_security_ssrf_path_traversal.py

@@ -245,13 +245,13 @@ class TestSkillServiceFilePathTraversal(unittest.TestCase):
     def test_relative_file_path_allowed(self):
         """A plain nested path stays inside the skill directory."""
         written = self._add_url_with_path("scripts/run.py")
-        expected = os.path.realpath(
-            os.path.join(self.skills_root, "innocent.tmp", "scripts/run.py")
-        )
-        self.assertEqual([expected], [os.path.realpath(p) for p in written])
-        self.assertTrue(
-            os.path.exists(os.path.join(self.skills_root, "innocent", "scripts", "run.py"))
+        self.assertEqual(1, len(written))
+        self.assertEqual(
+            os.path.realpath(self.skills_root),
+            os.path.commonpath([os.path.realpath(self.skills_root), os.path.realpath(written[0])]),
         )
+        with open(os.path.join(self.skills_root, "innocent", "scripts", "run.py")) as installed:
+            self.assertEqual("pwned", installed.read())
 
     def test_dotdot_file_path_blocked(self):
         """'../../escaped.py' must be rejected before anything is downloaded."""

tests/test_skill_replacement_rollback.py

@@ -0,0 +1,134 @@
+"""A failed replacement cannot remove an already installed working skill."""
+
+import errno
+import io
+import os
+import shutil
+import zipfile
+from pathlib import Path
+
+import pytest
+
+from agent.skills.manager import SkillManager
+from agent.skills.service import SkillService
+
+
+OLD = "---\nname: sample\ndescription: Existing skill\n---\n\n# Old instructions\n"
+NEW = "---\nname: sample\ndescription: Updated skill\n---\n\n# New instructions\n"
+
+
+@pytest.fixture
+def installed(tmp_path, monkeypatch):
+    builtin = tmp_path / "builtin"
+    builtin.mkdir()
+    custom = tmp_path / "skills"
+    skill = custom / "sample"
+    skill.mkdir(parents=True)
+    (skill / "SKILL.md").write_text(OLD, encoding="utf-8")
+    (skill / "resource.txt").write_text("original resource", encoding="utf-8")
+    manager = SkillManager(builtin_dir=str(builtin), custom_dir=str(custom))
+    service = SkillService(manager)
+    archive = io.BytesIO()
+    with zipfile.ZipFile(archive, "w") as package:
+        package.writestr("sample/SKILL.md", NEW)
+        package.writestr("sample/resource.txt", "updated resource")
+
+    def download(url, destination):
+        Path(destination).parent.mkdir(parents=True, exist_ok=True)
+        if url.endswith(".zip"):
+            Path(destination).write_bytes(archive.getvalue())
+        else:
+            Path(destination).write_text(NEW, encoding="utf-8")
+
+    monkeypatch.setattr(service, "_download_file", download)
+    return service, skill, builtin, custom
+
+
+def _payload(kind):
+    return {
+        "name": "sample", "type": kind,
+        "files": [{"url": "https://fixture.invalid/skill.zip"}] if kind == "package"
+        else [{"url": "https://fixture.invalid/SKILL.md", "path": "SKILL.md"}],
+    }
+
+
+def _assert_old_skill_reloads(skill, builtin, custom):
+    assert (skill / "SKILL.md").read_text(encoding="utf-8") == OLD
+    assert (skill / "resource.txt").read_text(encoding="utf-8") == "original resource"
+    fresh = SkillManager(builtin_dir=str(builtin), custom_dir=str(custom))
+    assert fresh.get_skill("sample").skill.description == "Existing skill"
+
+
+def test_copy_failure_preserves_the_existing_installed_package(installed, monkeypatch):
+    service, skill, builtin, custom = installed
+
+    def disk_full(source, destination, *args, **kwargs):
+        Path(destination).mkdir(parents=True)
+        (Path(destination) / "partial.txt").write_text("partial copy")
+        raise OSError(errno.ENOSPC, "No space left on device")
+
+    monkeypatch.setattr(shutil, "copytree", disk_full)
+    with pytest.raises(OSError, match="No space left"):
+        service.add(_payload("package"))
+    _assert_old_skill_reloads(skill, builtin, custom)
+
+
+def test_url_promotion_failure_restores_the_previous_skill(installed, monkeypatch):
+    service, skill, builtin, custom = installed
+    rename = os.rename
+
+    def fail_new_skill(source, destination, *args, **kwargs):
+        marker = Path(source) / "SKILL.md"
+        if Path(destination) == skill and marker.is_file() and marker.read_text(encoding="utf-8") == NEW:
+            raise PermissionError("replacement unavailable")
+        return rename(source, destination, *args, **kwargs)
+
+    monkeypatch.setattr(os, "rename", fail_new_skill)
+    with pytest.raises(PermissionError, match="replacement unavailable"):
+        service.add(_payload("url"))
+    _assert_old_skill_reloads(skill, builtin, custom)
+
+
+@pytest.mark.parametrize("kind", ["url", "package"])
+def test_successful_update_replaces_the_skill_and_reloads_metadata(installed, kind):
+    service, skill, builtin, custom = installed
+    service.add(_payload(kind))
+    assert (skill / "SKILL.md").read_text(encoding="utf-8") == NEW
+    fresh = SkillManager(builtin_dir=str(builtin), custom_dir=str(custom))
+    assert fresh.get_skill("sample").skill.description == "Updated skill"
+    assert [p.name for p in custom.iterdir() if p.is_dir()] == ["sample"]
+
+
+@pytest.mark.skipif(os.name == "nt", reason="POSIX permission bits")
+def test_url_install_keeps_umask_permissions(installed):
+    service, skill, _, _ = installed
+    service.add(_payload("url"))
+    umask = os.umask(0)
+    os.umask(umask)
+    assert skill.stat().st_mode & 0o777 == 0o777 & ~umask
+
+
+@pytest.mark.parametrize("kind", ["local", "staged"])
+def test_cli_copy_failure_keeps_the_previous_registered_skill(installed, tmp_path, monkeypatch, kind):
+    from cli.commands import skill as skill_cli
+
+    service, skill, builtin, custom = installed
+    incoming = tmp_path / "incoming" / "sample"
+    incoming.mkdir(parents=True)
+    (incoming / "SKILL.md").write_text(NEW, encoding="utf-8")
+    monkeypatch.setattr(skill_cli, "get_skills_dir", lambda agent_id=None: str(custom))
+    original_config = (custom / "skills_config.json").read_bytes()
+
+    def disk_full(source, destination, *args, **kwargs):
+        Path(destination).mkdir(parents=True)
+        (Path(destination) / "partial.txt").write_text("partial copy")
+        raise OSError(errno.ENOSPC, "No space left on device")
+
+    monkeypatch.setattr(shutil, "copytree", disk_full)
+    with pytest.raises(OSError, match="No space left"):
+        if kind == "local":
+            skill_cli.install_skill(str(incoming))
+        else:
+            skill_cli.commit_staged(str(incoming.parent))
+    assert (custom / "skills_config.json").read_bytes() == original_config
+    _assert_old_skill_reloads(skill, builtin, custom)