# NousResearch/hermes-agent · Bundled skill sync backup cleanup and safety fixes

## Open now

- Nothing open on GitHub.

These issues and PRs address crash-safety, stale backup directories, read-only file handling, and transaction ordering in the bundled skill sync and reset process. The work centers on making backup creation and cleanup more robust across edge cases like Nix read-only trees.

## TL;DR

- 4 member(s): 0 open issue(s), 1 closed issue(s), 0 open PR(s), 1 merged PR(s), 2 PR(s) closed without merging.
- A fix was merged to main in PR #35251. Verify it addresses your case.
- **Recommended:** read PR #35251 (and any competing PRs below) before filing. Confirm the merged change covers your symptom.

## Maintainer resolution verdict

- Internal-tracker verdict over this cluster's issue members: `unresolved`. This is distinct from the fix status above. Weigh it yourself.

## How this cluster was grouped

- Link evidence (`STRUCTURAL_SPANNING`): every member is connected by structural evidence: shared changed files, or a closing/superseded reference.
- Pairwise link density `0.50`: the share of member pairs that carry a link, counting a structural link in full and a relevance-only link at 0.36. `1.00` means every pair is linked.

## Closed / duplicate issues

- #34860: \[Bug\]: bundled skill sync can leave stale .bak directories

## Prior & competing PRs

- **#35131: fix(skills): fix transaction ordering in reset_bundled_skill and handle read-only files in rmtree**. closed without merge · fix tier `suggested`
  - maintainer direction: category `implemented_elsewhere`; implication `superseded`; refs `83a7d0b6016495a5d67f341a5252642ab8128f14`
  - _suggested-tier fix detail withheld; re-request with `?tier=all` to include it._
- **#35251: fix(skills): transactional skills reset --restore + read-only Nix tree cleanup (#34972)**. merged to main · fix tier `verified`
  - maintainer direction: category `merged`
  - files: `tests/tools/test_skills_sync.py`, `tools/skills_sync.py`
  - fix hunks:
  - `tests/tools/test_skills_sync.py`

    ```diff
    @@ -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`

    ```diff
    @@ -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():
    ```

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

    ```diff
    @@ -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:
    ```

    ```diff
    @@ -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)
     
    ```
- **#35182: fix(skills): handle read-only files when cleaning up skill sync backups**. closed without merge
  - maintainer direction: category `superseded_by_pr`; implication `superseded`; refs `#35251`, `#34972`

## Fix status

- Merged to main in PR #35251 (cited above). No release data is tracked. Verify downstream.
- Fix last verified (merged) `2026-05-30`.
- Subsystem: `skills`