From 8ae0802d59b26b5fdf104c902ca82e434132dd9a Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 30 May 2026 01:45:06 -0700 Subject: [PATCH] fix(skills): make _rmtree_writable handle read-only directories, not just files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 77 ++++++++++++++++++++++++++++----- tools/skills_sync.py | 19 +++++--- 2 files changed, 80 insertions(+), 16 deletions(-) diff --git a/tests/tools/test_skills_sync.py b/tests/tools/test_skills_sync.py index 1ef5e82d9..1a647ff36 100644 --- a/tests/tools/test_skills_sync.py +++ b/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 - entry must NOT be deleted — otherwise the skill enters a limbo state.""" - import os, stat + """#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" @@ -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 - os.chmod(dest, stat.S_IREAD | stat.S_IRGRP | stat.S_IROTH) - manifest_file.write_text("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 - os.chmod(dest, stat.S_IRWXU) + # User copy is still on disk (we changed nothing). + assert (dest / "SKILL.md").exists() diff --git a/tools/skills_sync.py b/tools/skills_sync.py index 96b6ed308..11d031cde 100644 --- a/tools/skills_sync.py +++ b/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 - import stat - os.chmod(fpath, stat.S_IWRITE) + # 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)