openclaw/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 9 Packages Projects Releases Wiki Activity

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

Browse Source 
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).
This commit is contained in:
teknium1
2026-05-30 01:45:06 -07:00
committed by Teknium
parent 83a7d0b601
commit 8ae0802d59
2 changed files with 80 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files

77
tests/tools/test_skills_sync.py
View File

@ -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()

19
tools/skills_sync.py
View File

@ -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)