fix(skills): harden _rmtree_writable for read-only directories

The salvaged helper used os.chmod(S_IWRITE) which only sets owner-write
and clears the execute bit — insufficient to recurse into and remove a
read-only *directory* on POSIX (the Nix-store case the issue describes).
Grant full owner rwx on both the entry and its parent before retrying.

Reworks the limbo-preservation test: the hardened helper now succeeds on
read-only Nix-store dirs (new test asserts restore succeeds), and the
manifest-preservation guard is exercised via a mocked unrecoverable
rmtree failure instead of relying on read-only-defeats-the-helper.

tests/tools/test_skills_sync.py

@@ -845,3 +845,60 @@ class TestResetBundledSkill:
             post_manifest = _read_manifest()
             assert "google-workspace" in post_manifest
         assert (skills_dir / "productivity" / "google-workspace" / "SKILL.md").exists()
+
+    def test_reset_restore_recovers_readonly_nix_store_copy(self, tmp_path):
+        """#34860/#34972: a read-only user copy (Nix-store permissions) must be
+        removable by the hardened rmtree so restore succeeds cleanly."""
+        import os, 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"
+        dest.mkdir(parents=True)
+        (dest / "SKILL.md").write_text("# user version\n")
+        # Simulate Nix-store permissions: read-only file inside a read-only dir.
+        os.chmod(dest / "SKILL.md", stat.S_IREAD)
+        os.chmod(dest, stat.S_IREAD | stat.S_IEXEC)
+        manifest_file.write_text("google-workspace:STALEHASH000000000000000000000000\n")
+
+        with self._patches(bundled, skills_dir, manifest_file):
+            result = reset_bundled_skill("google-workspace", restore=True)
+
+        # Hardened rmtree made the tree writable and removed it → restore succeeded.
+        assert result["ok"] is True
+        assert result["action"] == "restored"
+        # Skill was re-copied from the bundled source.
+        assert (skills_dir / "productivity" / "google-workspace" / "SKILL.md").exists()
+
+    def test_reset_restore_preserves_manifest_on_rmtree_failure(self, tmp_path):
+        """#34972: when rmtree genuinely cannot remove the copy (e.g. a dir we
+        don't own), the manifest entry must NOT be deleted — otherwise the skill
+        enters a limbo state. Manifest deletion must happen only AFTER a
+        successful rmtree."""
+        from unittest.mock import patch as _patch
+        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")
+
+        with self._patches(bundled, skills_dir, manifest_file):
+            # Force the removal to fail unconditionally, mimicking an
+            # unrecoverable permission error the chmod retry can't fix.
+            with _patch(
+                "tools.skills_sync._rmtree_writable",
+                side_effect=OSError("Operation not permitted"),
+            ):
+                result = reset_bundled_skill("google-workspace", restore=True)
+
+        # Restore failed, but manifest must be preserved.
+        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

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,37 @@ 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.  See #34860, #34972.
+
+    Deleting a read-only *directory* on POSIX requires owner write+execute on
+    the directory itself so rmtree can recurse and unlink its children — and
+    on the parent so the entry can be removed. ``stat.S_IWRITE`` alone is
+    insufficient (it clears the execute bit), so grant the full owner rwx and
+    also unlock the parent before retrying.
+    """
+    import stat
+
+    def _on_error(func, fpath, exc_info):
+        try:
+            parent = os.path.dirname(fpath)
+            if parent:
+                os.chmod(parent, stat.S_IRWXU | os.stat(parent).st_mode)
+        except OSError:
+            pass
+        try:
+            os.chmod(fpath, stat.S_IRWXU | os.stat(fpath).st_mode)
+        except OSError:
+            # Entry already gone or unstatable — let the retry surface it.
+            os.chmod(fpath, stat.S_IRWXU)
+        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 +640,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 +650,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)