Skill update backup file handling safety fixes

Fixes on record

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

• #45837 fix(skills): make bundled-update backup handling crash-safe

  merged to main

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

  Show the diff

  • scripts/release.py

    @@ -81,6 +81,7 @@
         "[redacted:email]": "synapsesx",
         "[redacted:email]": "itsflownium",
         "[redacted:email]": "dirtyren",
    +    "[redacted:email]": "MaxFreedomPollard",
         "[redacted:email]": "achaljhawar",
         "[redacted:email]": "claytonchew",
         "[redacted:email]": "hbentel",

  • tests/tools/test_skills_sync.py

    @@ -1,5 +1,6 @@
     """Tests for tools/skills_sync.py — manifest-based skill seeding and updating."""
     
    +import shutil
     import json
     from pathlib import Path
     from unittest.mock import patch
    @@ -1067,3 +1068,123 @@ def test_remove_keeps_user_modified(self, tmp_path):
                 assert "EDITED" in (skills_dir / "beta" / "SKILL.md").read_text()
                 # non-bundled local skill never considered
                 assert (skills_dir / "mine" / "SKILL.md").exists()
    +
    +
    +class TestUpdateBackupRecovery:
    +    """Regression tests for backup handling in the bundled-update path.
    +
    +    Covers three failure modes around ``dest.with_suffix(".bak")``:
    +    a stale backup poisoning the next update's move/restore, an orphaned
    +    backup (crash between move and copytree) being misread as a user
    +    deletion, and a partially-written dest blocking restore-on-failure.
    +    """
    +
    +    def _setup(self, tmp_path, bundled_text="# Old v2 (updated)"):
    +        """Bundled dir with one flat skill, plus user dirs."""
    +        bundled = tmp_path / "bundled_skills"
    +        (bundled / "old-skill").mkdir(parents=True)
    +        (bundled / "old-skill" / "SKILL.md").write_text(bundled_text)
    +        skills_dir = tmp_path / "user_skills"
    +        skills_dir.mkdir()
    +        manifest_file = skills_dir / ".bundled_manifest"
    +        return bundled, skills_dir, manifest_file
    +
    +    def _patches(self, bundled, skills_dir, manifest_file):
    +        from contextlib import ExitStack
    +        stack = ExitStack()
    +        stack.enter_context(patch("tools.skills_sync._get_bundled_dir", return_value=bundled))
    +        stack.enter_context(patch("tools.skills_sync._get_optional_dir", return_value=bundled.parent / "optional-skills"))
    +        stack.enter_context(patch("tools.skills_sync.SKILLS_DIR", skills_dir))
    +        stack.enter_context(patch("tools.skills_sync.MANIFEST_FILE", manifest_file))
    +        return stack
    +
    +    def _seed_synced_copy(self, skills_dir, manifest_file, text="# Old v1"):
    +        """User copy of old-skill whose hash matches the manifest origin."""
    +        dest = skills_dir / "old-skill"
    +        dest.mkdir(parents=True)
    +        (dest / "SKILL.md").write_text(text)
    +        with patch("tools.skills_sync.MANIFEST_FILE", manifest_file):
    +            _write_manifest({"old-skill": _dir_hash(dest)})
    +        return dest
    +
    +    def test_stale_backup_does_not_poison_failed_update(self, tmp_path):
    +        """A leftover .bak must not nest the live copy or corrupt restore.
    +
    +        With a stale ``old-skill.bak`` present, ``shutil.move(dest, backup)``
    +        moves the live copy *inside* the stale dir. If copytree then fails,
    +        restore drags the stale junk (with the live copy nested in it) back
    +        to dest — corrupting the skill and wedging it as "user-modified".
    +        """
    +        bundled, skills_dir, manifest_file = self._setup(tmp_path)
    +        dest = self._seed_synced_copy(skills_dir, manifest_file)
    +
    +        stale = skills_dir / "old-skill.bak"
    +        stale.mkdir()
    +        (stale / "SKILL.md").write_text("# stale junk from an earlier failure")
    +
    +        def _boom(src, dst, **kwargs):
    +            raise OSError("simulated copy failure")
    +
    +        with self._patches(bundled, skills_dir, manifest_file), \
    +                patch("tools.skills_sync.shutil.copytree", side_effect=_boom):
    +            sync_skills(quiet=True)
    +
    +        # The live copy must survive the failed update untouched...
    +        assert (dest / "SKILL.md").read_text() == "# Old v1"
    +        # ...not be nested inside recycled stale-backup content.
    +        assert not (dest / "old-skill").exists()
    +        # And no backup directory may linger.
    +        assert not (skills_dir / "old-skill.bak").exists()
    +
    +    def test_orphaned_backup_is_recovered_not_treated_as_deleted(self, tmp_path):
    +        """Crash between move and copytree must not lose the skill.
    +
    +        After such a crash, dest is gone and the user's only copy sits in
    +        ``old-skill.bak``. The sync loop's "in manifest but not on disk"
    +        branch reads that as a deliberate user deletion and skips — the
    +        skill silently vanishes. It must instead be recovered and updated.
    +        """
    +        bundled, skills_dir, manifest_file = self._setup(tmp_path)
    +        dest = self._seed_synced_copy(skills_dir, manifest_file)
    +        # Simulate the crash: dest was moved aside, new copy never arrived.
    +        shutil.move(str(dest), str(skills_dir / "old-skill.bak"))
    +
    +        with self._patches(bundled, skills_dir, manifest_file):
    +            result = sync_skills(quiet=True)
    +
    +        # Recovered and then updated to the new bundled version in one run.
    +        assert (dest / "SKILL.md").exists()
    +        assert (dest / "SKILL.md").read_text() == "# Old v2 (updated)"
    +        assert "old-skill" in result["updated"]
    +        assert not (skills_dir / "old-skill.bak").exists()
    +
    +    def test_partial_copy_failure_restores_original(self, tmp_path):
    +        """A half-written dest must not block restore-on-failure.
    +
    +        If copytree dies after creating dest, the ``not dest.exists()``
    +        guard skips the restore: the user keeps a broken partial skill,
    +        the .bak lingers, and the partial hash wedges the skill as
    +        "user-modified" on every later sync.
    +        """
    +        bundled, skills_dir, manifest_file = self._setup(tmp_path)
    +        dest = self._seed_synced_copy(skills_dir, manifest_file)
    +
    +        def _partial_then_fail(src, dst, **kwargs):
    +            Path(dst).mkdir(parents=True, exist_ok=True)
    +            (Path(dst) / "PARTIAL").write_text("half-written")
    +            raise OSError("simulated failure mid-copy")
    +
    +        with self._patches(bundled, skills_dir, manifest_file), \
    +                patch("tools.skills_sync.shutil.copytree", side_effect=_partial_then_fail):
    +            sync_skills(quiet=True)
    +
    +        # Original content restored, partial debris and backup gone.
    +        assert (dest / "SKILL.md").read_text() == "# Old v1"
    +        assert not (dest / "PARTIAL").exists()
    +        assert not (skills_dir / "old-skill.bak").exists()
    +
    +        # And the skill is not wedged: a later normal sync updates cleanly.
    +        with self._patches(bundled, skills_dir, manifest_file):
    +            result2 = sync_skills(quiet=True)
    +        assert "old-skill" in result2["updated"]
    +        assert result2["user_modified"] == []

  • tools/skills_sync.py

    @@ -506,6 +506,24 @@ def sync_skills(quiet: bool = False) -> dict:
             dest = _compute_relative_dest(skill_src, bundled_dir)
             bundled_hash = _dir_hash(skill_src)
     
    +        # Recover an orphaned backup before classifying. If a previous
    +        # update was interrupted between moving dest aside and copying the
    +        # new version in, the user's only copy sits in ``dest.bak`` while
    +        # dest is gone — without this, the "in manifest but not on disk"
    +        # branch below misreads the skill as user-deleted and it silently
    +        # vanishes from discovery.
    +        _orphan = dest.with_suffix(".bak")
    +        if _orphan.exists() and not dest.exists():
    +            try:
    +                dest.parent.mkdir(parents=True, exist_ok=True)
    +                shutil.move(str(_orphan), str(dest))
    +                logger.info("Recovered orphaned skill backup: %s", _orphan)
    +            except (OSError, IOError):
    +                logger.warning(
    +                    "Could not recover orphaned skill backup %s", _orphan,
    +                    exc_info=True,
    +                )
    +
             if skill_name not in manifest:
                 # ── New skill — never offered before ──
                 try:
    @@ -569,6 +587,12 @@ def sync_skills(quiet: bool = False) -> dict:
                     try:
                         # Move old copy to a backup so we can restore on failure
                         backup = dest.with_suffix(".bak")
    +                    # A stale backup left by an earlier failure would make
    +                    # shutil.move() nest dest *inside* it (or fail outright)
    +                    # and would poison the restore path below. The current
    +                    # dest is the authoritative copy — clear the leftover.
    +                    if backup.exists():
    +                        _rmtree_writable(backup)
                         shutil.move(str(dest), str(backup))
                         try:
                             shutil.copytree(skill_src, dest)
    @@ -582,9 +606,20 @@ def sync_skills(quiet: bool = False) -> dict:
                             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():
    -                            shutil.move(str(backup), str(dest))
    +                        # Restore from backup. A partially-written dest must
    +                        # not shadow the user's copy or block the restore —
    +                        # clear it first, then move the backup home.
    +                        if backup.exists():
    +                            if dest.exists():
    +                                try:
    +                                    _rmtree_writable(dest)
    +                                except (OSError, IOError):
    +                                    logger.warning(
    +                                        "Could not clear partial copy %s during restore",
    +                                        dest, exc_info=True,
    +                                    )
    +                            if not dest.exists():
    +                                shutil.move(str(backup), str(dest))
                             raise
                     except (OSError, IOError) as e:
                         if not quiet: