Bundled skill sync backup cleanup and safety fixes

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #35251 fix(skills): transactional skills reset --restore + read-only Nix tree cleanup (#34972)

  merged to main

  files: tests/tools/test_skills_sync.py, tools/skills_sync.py

  Show the diff

  • tests/tools/test_skills_sync.py

    @@ -845,3 +845,85 @@ def test_reset_no_op_when_already_clean(self, tmp_path):
                 post_manifest = _read_manifest()
                 assert "google-workspace" in post_manifest
             assert (skills_dir / "productivity" / "google-workspace" / "SKILL.md").exists()
    +
    +    def test_reset_restore_succeeds_on_readonly_nix_tree(self, tmp_path):
    +        """#34972: --restore must succeed even when the user copy is a fully
    +        read-only tree (r-xr-xr-x dirs + files), as produced by copying a
    +        Nix-store source. The manifest is re-baselined and bundled re-copied."""
    +        import os
    +        import stat
    +
    +        bundled = self._setup_bundled(tmp_path)
    +        skills_dir = tmp_path / "user_skills"
    +        manifest_file = skills_dir / ".bundled_manifest"
    +
    +        dest = skills_dir / "productivity" / "google-workspace"
    +        sub = dest / "references"
    +        sub.mkdir(parents=True)
    +        (dest / "SKILL.md").write_text("# user version\n")
    +        (sub / "ref.md").write_text("# nested ref\n")
    +        manifest_file.write_text(
    +            "google-workspace:STALEHASH000000000000000000000000\n"
    +        )
    +
    +        # Read-only files AND directories — the real Nix-store case.
    +        ro_dir = (
    +            stat.S_IRUSR | stat.S_IXUSR | stat.S_IRGRP | stat.S_IXGRP
    +            | stat.S_IROTH | stat.S_IXOTH
    +        )
    +        os.chmod(sub / "ref.md", stat.S_IREAD)
    +        os.chmod(dest / "SKILL.md", stat.S_IREAD)
    +        os.chmod(sub, ro_dir)
    +        os.chmod(dest, ro_dir)
    +
    +        try:
    +            with self._patches(bundled, skills_dir, manifest_file):
    +                result = reset_bundled_skill("google-workspace", restore=True)
    +
    +            assert result["ok"] is True
    +            assert result["action"] == "restored"
    +            # Bundled version was re-copied over the (deleted) user copy.
    +            assert "upstream" in (dest / "SKILL.md").read_text()
    +            # The read-only nested user dir/file was fully removed, not left behind.
    +            assert not (sub / "ref.md").exists()
    +            # sync ran and re-copied the skill (not stuck in limbo).
    +            assert "google-workspace" in result["synced"]["copied"]
    +        finally:
    +            # Restore perms so tmp_path teardown can remove anything left.
    +            for p in (sub, dest):
    +                if p.exists():
    +                    os.chmod(p, stat.S_IRWXU)
    +
    +    def test_reset_restore_preserves_manifest_on_rmtree_failure(self, tmp_path):
    +        """#34972: when the user copy genuinely cannot be removed, the manifest
    +        entry must NOT be deleted — otherwise the skill enters a limbo state
    +        where future syncs silently skip it forever."""
    +        bundled = self._setup_bundled(tmp_path)
    +        skills_dir = tmp_path / "user_skills"
    +        manifest_file = skills_dir / ".bundled_manifest"
    +
    +        dest = skills_dir / "productivity" / "google-workspace"
    +        dest.mkdir(parents=True)
    +        (dest / "SKILL.md").write_text("# user version\n")
    +        manifest_file.write_text(
    +            "google-workspace:STALEHASH000000000000000000000000\n"
    +        )
    +
    +        # Simulate an unremovable tree (e.g. a busy mountpoint or a path even
    +        # chmod can't rescue) by making the removal helper raise.
    +        def _boom(_path):
    +            raise PermissionError(13, "Permission denied")
    +
    +        with self._patches(bundled, skills_dir, manifest_file), patch(
    +            "tools.skills_sync._rmtree_writable", side_effect=_boom
    +        ):
    +            result = reset_bundled_skill("google-workspace", restore=True)
    +
    +        # Restore failed, and the manifest must be left untouched.
    +        assert result["ok"] is False
    +        assert result["action"] == "not_reset"
    +        assert "Manifest entry preserved" in result["message"]
    +        manifest_after = manifest_file.read_text()
    +        assert "google-workspace" in manifest_after
    +        # User copy is still on disk (we changed nothing).
    +        assert (dest / "SKILL.md").exists()

  • tools/skills_sync.py

    @@ -517,7 +517,10 @@ def sync_skills(quiet: bool = False) -> dict:
                             if not quiet:
                                 print(f"  ↑ {skill_name} (updated)")
                             # Remove backup after successful copy
    -                        shutil.rmtree(backup, ignore_errors=True)
    +                        try:
    +                            _rmtree_writable(backup)
    +                        except (OSError, IOError):
    +                            logger.debug("Could not remove backup %s", backup, exc_info=True)
                         except (OSError, IOError):
                             # Restore from backup
                             if backup.exists() and not dest.exists():
    @@ -563,6 +566,30 @@ def sync_skills(quiet: bool = False) -> dict:
         }
     
     
    +def _rmtree_writable(path: Path) -> None:
    +    """Remove a directory tree, making read-only entries writable first.
    +
    +    Handles immutable package sources (Nix store, deb/rpm installs) that
    +    preserve read-only permissions on copied files *and* directories
    +    (``r-xr-xr-x``).  Removing a child requires write permission on its
    +    parent directory, so the retry handler makes the failing path **and its
    +    parent** writable before re-attempting.  See #34860, #34972.
    +    """
    +    import stat
    +
    +    def _on_error(func, fpath, exc_info):
    +        # Unlinking a child requires the parent dir to be writable, so chmod
    +        # the parent as well as the failing path, then retry.
    +        for target in (os.path.dirname(fpath), fpath):
    +            try:
    +                os.chmod(target, stat.S_IRWXU)
    +            except OSError:
    +                pass
    +        func(fpath)
    +
    +    shutil.rmtree(path, onerror=_on_error)
    +
    +
     def reset_bundled_skill(name: str, restore: bool = False) -> dict:
         """
         Reset a bundled skill's manifest tracking so future syncs work normally.
    @@ -606,12 +633,9 @@ def reset_bundled_skill(name: str, restore: bool = False) -> dict:
                 "synced": None,
             }
     
    -    # Step 1: drop the manifest entry so next sync treats it as new
    -    if in_manifest:
    -        del manifest[name]
    -        _write_manifest(manifest)
    -
    -    # Step 2 (optional): delete the user's copy so next sync re-copies bundled
    +    # Step 1 (optional): delete the user's copy so next sync re-copies bundled.
    +    # Must happen BEFORE manifest deletion so that a failed rmtree does not
    +    # leave the skill in a manifest-less limbo state (see #34972).
         deleted_user_copy = False
         if restore:
             if not is_bundled:
    @@ -619,28 +643,32 @@ def reset_bundled_skill(name: str, restore: bool = False) -> dict:
                     "ok": False,
                     "action": "bundled_missing",
                     "message": (
    -                    f"'{name}' has no bundled source — manifest entry cleared "
    +                    f"'{name}' has no bundled source — manifest entry preserved "
                         f"but cannot restore from bundled (skill was removed upstream)."
                     ),
                     "synced": None,
                 }
    -        # The destination mirrors the bundled path relative to bundled_dir.
             dest = _compute_relative_dest(bundled_by_name[name], bundled_dir)
             if dest.exists():
                 try:
    -                shutil.rmtree(dest)
    +                _rmtree_writable(dest)
                     deleted_user_copy = True
                 except (OSError, IOError) as e:
                     return {
                         "ok": False,
    -                    "action": "manifest_cleared",
    +                    "action": "not_reset",
                         "message": (
    -                        f"Cleared manifest entry for '{name}' but could not "
    -                        f"delete user copy at {dest}: {e}"
    +                        f"Could not delete user copy at {dest}: {e}. "
    +                        f"Manifest entry preserved — nothing was changed."
                         ),
                         "synced": None,
                     }
     
    +    # Step 2: drop the manifest entry so next sync treats it as new
    +    if in_manifest:
    +        del manifest[name]
    +        _write_manifest(manifest)
    +
         # Step 3: run sync to re-baseline (or re-copy if we deleted)
         synced = sync_skills(quiet=True)