fix(skills): make _rmtree_writable handle read-only directories, not just files

The cherry-picked fix's onerror handler chmod'd only the failing path, but
unlinking a child requires write permission on its PARENT directory. On a true
Nix-store copy (r-xr-xr-x dirs + files) rmtree still failed. Now chmod the
parent dir as well before retrying.

Also rewrites the regression test: the original asserted the helper FAILS on a
read-only dir (documenting the limitation), which is the wrong success criterion.
Split into two tests — restore succeeds on a full read-only tree (real Nix case),
and manifest is preserved when removal genuinely cannot proceed (monkeypatched).

tests/tools/test_skills_sync.py

@@ -846,10 +846,59 @@ class TestResetBundledSkill:
             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()
+            # Manifest now tracks the skill again (re-baselined, not in limbo).
+            manifest_after = _read_manifest()
+            assert "google-workspace" in manifest_after
+        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 rmtree fails (e.g. read-only Nix-store files), the manifest
+        """#34972: when the user copy genuinely cannot be removed, the manifest
-        entry must NOT be deleted — otherwise the skill enters a limbo state."""
+        entry must NOT be deleted — otherwise the skill enters a limbo state
-        import os, stat
+        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"
@@ -857,19 +906,25 @@ class TestResetBundledSkill:
         dest = skills_dir / "productivity" / "google-workspace"
         dest.mkdir(parents=True)
         (dest / "SKILL.md").write_text("# user version\n")
-        # Make directory read-only to simulate Nix-store permissions
+        manifest_file.write_text(
-        os.chmod(dest, stat.S_IREAD | stat.S_IRGRP | stat.S_IROTH)
+            "google-workspace:STALEHASH000000000000000000000000\n"
-        manifest_file.write_text("google-workspace:STALEHASH000000000000000000000000\n")
+        )
 
-        with self._patches(bundled, skills_dir, manifest_file):
+        # 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, but manifest must be preserved
+        # 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 still has the old entry (not deleted)
         manifest_after = manifest_file.read_text()
         assert "google-workspace" in manifest_after
-        # Cleanup: restore permissions for tmp_path removal
+        # User copy is still on disk (we changed nothing).
-        os.chmod(dest, stat.S_IRWXU)
+        assert (dest / "SKILL.md").exists()

tools/skills_sync.py

@@ -567,15 +567,24 @@ def sync_skills(quiet: bool = False) -> dict:
 
 
 def _rmtree_writable(path: Path) -> None:
-    """Remove a directory tree, making read-only files writable first.
+    """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.  See #34860, #34972.
+    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):
-        # Make the file/directory writable and retry
+        # Unlinking a child requires the parent dir to be writable, so chmod
-        import stat
+        # the parent as well as the failing path, then retry.
-        os.chmod(fpath, stat.S_IWRITE)
+        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)