diff --git a/hermes_cli/mcp_catalog.py b/hermes_cli/mcp_catalog.py index 5e7b687cc6..3645fa3c21 100644 --- a/hermes_cli/mcp_catalog.py +++ b/hermes_cli/mcp_catalog.py @@ -20,6 +20,7 @@ from hermes_cli._subprocess_compat import noninteractive_git_env from hermes_cli.colors import Colors, color from hermes_cli.config import load_config, save_config, get_env_value, save_env_value from hermes_cli.cli_output import prompt as _prompt_input +from utils import rmtree_readonly _MANIFEST_VERSION = 1 @@ -413,7 +414,7 @@ def _do_git_install(entry: CatalogEntry) -> Path: if dest.exists(): # Fresh checkout each install — the manifest ref is the source of truth. _say(f" Removing existing install at {dest}", Colors.DIM) - shutil.rmtree(dest) + rmtree_readonly(dest) _say(f" Cloning {install.url} ({install.ref}) → {dest}", Colors.CYAN) # `git clone --branch` only accepts branches/tags, NOT commit SHAs; detect SHA-shaped refs @@ -433,7 +434,7 @@ def _do_git_install(entry: CatalogEntry) -> Path: if not is_sha_ref and _git("clone", "--depth", "1", "--branch", install.ref, install.url, str(dest)) != 0: # Branch/tag form failed (e.g. ref deleted upstream): fall through to full-clone path. if dest.exists(): - shutil.rmtree(dest) + rmtree_readonly(dest) is_sha_ref = True if is_sha_ref: if _git("clone", install.url, str(dest)) != 0: @@ -749,6 +750,6 @@ def uninstall_entry(name: str, *, purge_install_dir: bool = True) -> bool: if purge_install_dir: clone = _install_root() / name if clone.exists(): - shutil.rmtree(clone) + rmtree_readonly(clone) removed = True return removed diff --git a/hermes_cli/plugins_cmd.py b/hermes_cli/plugins_cmd.py index 3fdf1c2387..e97052674f 100644 --- a/hermes_cli/plugins_cmd.py +++ b/hermes_cli/plugins_cmd.py @@ -21,7 +21,7 @@ from hermes_cli.cli_output import line_input from hermes_cli.config import cfg_get from hermes_cli.plugin_capabilities import _child_dict from hermes_cli.secret_prompt import masked_secret_prompt -from utils import atomic_write_text +from utils import atomic_write_text, rmtree_readonly logger = logging.getLogger(__name__) @@ -709,7 +709,7 @@ def _swap_in_plugin(tmp_target: Path, target: Path, backup: Path, old_metadata: _write_install_metadata(new_metadata) except Exception: if target.exists(): - shutil.rmtree(target) + rmtree_readonly(target) if replaced_existing and backup.exists(): os.replace(backup, target) if old_metadata: @@ -984,7 +984,7 @@ def _remove_plugin_core(target: Path) -> None: """Remove one plugin and its metadata without splitting their state.""" metadata = _read_install_metadata() if target.name not in metadata: - shutil.rmtree(target) + rmtree_readonly(target) return updated = {k: v for k, v in metadata.items() if k != target.name} staging = Path(tempfile.mkdtemp(prefix=f".{target.name}.remove-", dir=target.parent)) @@ -1000,9 +1000,9 @@ def _remove_plugin_core(target: Path) -> None: f"Plugin metadata update failed and '{target.name}' could not be " f"restored automatically; recovery copy remains at {backup}." ) from restore_exc - shutil.rmtree(staging, ignore_errors=True) + rmtree_readonly(staging, ignore_errors=True) raise - shutil.rmtree(staging) + rmtree_readonly(staging) def cmd_remove(name: str) -> None: diff --git a/tests/hermes_cli/test_mcp_catalog.py b/tests/hermes_cli/test_mcp_catalog.py index 8426588175..ffbdad9fa2 100644 --- a/tests/hermes_cli/test_mcp_catalog.py +++ b/tests/hermes_cli/test_mcp_catalog.py @@ -656,6 +656,22 @@ class TestUninstall: assert uninstall_entry("nonexistent") is False + def test_uninstall_removes_read_only_git_clone(self, monkeypatch): + """Loose objects are read-only in a clone: the purge must clear that, not abort (#117176).""" + import hermes_cli.mcp_catalog as mc + + monkeypatch.setattr(mc, "remove_server", lambda name: False) + clone = mc._install_root() / "demo" + obj_dir = clone / ".git" / "objects" / "4b" + obj_dir.mkdir(parents=True) + obj = obj_dir / "825dc642cb6eb9a060e54bf8d69288fbee4904" + obj.write_text("blob", encoding="utf-8") + obj.chmod(0o444) + obj_dir.chmod(0o555) + + assert mc.uninstall_entry("demo") is True + assert not clone.exists() + # --------------------------------------------------------------------------- # Picker (non-TTY paths only — interactive curses is integration-tested) diff --git a/tests/hermes_cli/test_plugins_cmd.py b/tests/hermes_cli/test_plugins_cmd.py index fb4f3f09bf..598a72966a 100644 --- a/tests/hermes_cli/test_plugins_cmd.py +++ b/tests/hermes_cli/test_plugins_cmd.py @@ -361,7 +361,7 @@ class TestCmdInstall: @patch("hermes_cli.plugins_cmd._display_after_install") @patch("hermes_cli.plugins_cmd.shutil.move") - @patch("hermes_cli.plugins_cmd.shutil.rmtree") + @patch("hermes_cli.plugins_cmd.rmtree_readonly") @patch("hermes_cli.plugins_cmd._plugins_dir") @patch("hermes_cli.plugins_cmd._read_manifest") @patch("hermes_cli.plugins_cmd.subprocess.run") @@ -449,7 +449,7 @@ class TestCmdRemove: @patch("hermes_cli.plugins_cmd._sanitize_plugin_name") @patch("hermes_cli.plugins_cmd._plugins_dir") - @patch("hermes_cli.plugins_cmd.shutil.rmtree") + @patch("hermes_cli.plugins_cmd.rmtree_readonly") def test_remove_deletes_plugin(self, mock_rmtree, mock_plugins_dir, mock_sanitize): from hermes_cli.plugins_cmd import cmd_remove @@ -479,6 +479,22 @@ class TestCmdRemove: assert exc_info.value.code == 1 + def test_remove_plugin_core_deletes_read_only_git_tree(self, tmp_path): + """Git leaves loose objects read-only: removal must clear that, not abort (#117179).""" + from hermes_cli.plugins_cmd import _remove_plugin_core + + target = tmp_path / "plugins" / "demo" + obj_dir = target / ".git" / "objects" / "4b" + obj_dir.mkdir(parents=True) + obj = obj_dir / "825dc642cb6eb9a060e54bf8d69288fbee4904" + obj.write_text("blob", encoding="utf-8") + obj.chmod(0o444) + obj_dir.chmod(0o555) + + _remove_plugin_core(target) + + assert not target.exists() + # ── cmd_list tests ───────────────────────────────────────────────────────── diff --git a/tests/test_utils_rmtree_readonly.py b/tests/test_utils_rmtree_readonly.py new file mode 100644 index 0000000000..81fc7cec55 --- /dev/null +++ b/tests/test_utils_rmtree_readonly.py @@ -0,0 +1,88 @@ +"""``utils.rmtree_readonly`` removes trees that ``shutil.rmtree`` refuses. + +Git marks loose object files read-only on Windows (``WinError 5``), and package +installs arrive as read-only trees on POSIX, so every cleanup path that deletes a +clone needs the retry. Regression coverage for #117170, #117176 and #117179. +""" + +from __future__ import annotations + +import os +import stat +from pathlib import Path + +import pytest + +import utils +from utils import rmtree_readonly + + +def _read_only_object_dir(root: Path) -> Path: + """A clone-shaped tree whose loose object and its directory are read-only.""" + obj_dir = root / ".git" / "objects" / "4b" + obj_dir.mkdir(parents=True) + obj = obj_dir / "825dc642cb6eb9a060e54bf8d69288fbee4904" + obj.write_text("blob", encoding="utf-8") + obj.chmod(0o444) + obj_dir.chmod(0o555) # POSIX unlink needs a writable parent + return obj_dir + + +def test_removes_tree_with_read_only_object(tmp_path): + root = tmp_path / "plugins" / "demo" + obj_dir = _read_only_object_dir(root) + assert not (obj_dir / "825dc642cb6eb9a060e54bf8d69288fbee4904").stat().st_mode & stat.S_IWUSR + + rmtree_readonly(root) + + assert not root.exists() + + +@pytest.mark.windows_only +def test_removes_read_only_file_in_writable_directory(tmp_path): + """The Git-for-Windows shape: the file is read-only, its directory is writable.""" + root = tmp_path / "plugins" / "demo" + obj_dir = root / ".git" / "objects" / "4b" + obj_dir.mkdir(parents=True) + (obj_dir / "825dc642cb6eb9a060e54bf8d69288fbee4904").write_text("blob", encoding="utf-8") + (obj_dir / "825dc642cb6eb9a060e54bf8d69288fbee4904").chmod(stat.S_IREAD) + + rmtree_readonly(root) + + assert not root.exists() + + +def _stub_rmtree(monkeypatch, exc: OSError) -> list: + """Replace ``shutil.rmtree`` so the wrapper sees *exc* for every attempt.""" + attempts: list = [] + + def _fake(path, **kwargs): + attempts.append((path, kwargs)) + raise exc + + monkeypatch.setattr(utils.shutil, "rmtree", _fake) + return attempts + + +def test_ignore_errors_swallows_a_persistent_permission_failure(tmp_path, monkeypatch): + _stub_rmtree(monkeypatch, PermissionError(13, "Access is denied", str(tmp_path))) + + rmtree_readonly(tmp_path, ignore_errors=True) + + +def test_permission_failure_still_raises_without_ignore_errors(tmp_path, monkeypatch): + _stub_rmtree(monkeypatch, PermissionError(13, "Access is denied", str(tmp_path))) + + with pytest.raises(PermissionError): + rmtree_readonly(tmp_path) + + +def test_non_permission_failures_propagate(tmp_path, monkeypatch): + """Only ``PermissionError`` is retried — everything else keeps rmtree semantics.""" + attempts = _stub_rmtree(monkeypatch, OSError(39, "Directory not empty", str(tmp_path))) + + with pytest.raises(OSError) as excinfo: + rmtree_readonly(tmp_path) + + assert excinfo.value.errno == 39 + assert len(attempts) == 1 # no second attempt for a non-permission failure diff --git a/tools/checkpoint_manager.py b/tools/checkpoint_manager.py index 9f3e9178d8..ef9cfc9318 100644 --- a/tools/checkpoint_manager.py +++ b/tools/checkpoint_manager.py @@ -27,7 +27,7 @@ from typing import Dict, Iterator, List, NamedTuple, Optional, Set, Tuple from hermes_constants import get_hermes_home from hermes_cli._subprocess_compat import windows_hide_flags from hermes_cli.gitlock import clear_stale_tmp_packs -from utils import env_int +from utils import env_int, rmtree_readonly logger = logging.getLogger(__name__) @@ -1050,7 +1050,7 @@ def _rmtree_counted(child: Path, result: Dict[str, int], key: str, fail_fmt: str """rmtree ``child``, crediting bytes + ``result[key]``; failures count as ``errors`` when tracked.""" try: size = _dir_size_bytes(child) - shutil.rmtree(child) + rmtree_readonly(child) result["bytes_freed"] += size result[key] += 1 except OSError as exc: @@ -1271,7 +1271,7 @@ def clear_all(checkpoint_base: Optional[Path] = None) -> Dict[str, int]: return out size = _dir_size_bytes(base) try: - shutil.rmtree(base) + rmtree_readonly(base) out.update(bytes_freed=size, deleted=True) except OSError as exc: logger.warning("Could not clear checkpoint base %s: %s", base, exc) diff --git a/utils.py b/utils.py index 3786bc8aa4..64b8c5436f 100644 --- a/utils.py +++ b/utils.py @@ -225,6 +225,41 @@ def fsync_directory(path: Union[str, Path]) -> None: os.close(fd) +def rmtree_readonly(path: Union[str, Path], *, ignore_errors: bool = False) -> None: + """``shutil.rmtree`` that can also delete read-only trees. + + ``shutil.rmtree`` stops at the first entry it cannot unlink. Git marks + loose object files read-only on Windows (``WinError 5``), and package + installs (Nix store, deb/rpm) are copied ``r--r--r--`` into ``0555`` + directories on POSIX, where unlinking needs a writable *parent*. Clear the + write bit on the failing path and on its parent, then retry the exact + operation that failed. Only ``PermissionError`` is retried: every other + failure keeps ``shutil.rmtree``'s semantics (and ``ignore_errors``). + """ + + def _on_error(func, fpath, exc_info): + # ``onerror`` (3.11) passes ``exc_info``, ``onexc`` (3.12+) the exception. + exc = exc_info[1] if isinstance(exc_info, tuple) else exc_info + if not isinstance(exc, PermissionError): + raise exc + for candidate in (os.path.dirname(fpath), fpath): + if candidate: + with suppress(OSError): + os.chmod(candidate, os.stat(candidate).st_mode | stat.S_IWUSR | stat.S_IXUSR) + func(fpath) + + try: + try: + shutil.rmtree(path, onexc=_on_error) + except TypeError: # ``onexc`` is 3.12+; 3.11 only knows ``onerror`` + shutil.rmtree(path, onerror=_on_error) + except OSError: + # ``ignore_errors`` still gets the read-only recovery; it only swallows + # whatever is left after the retry. + if not ignore_errors: + raise + + def _atomic_write(path: Path, write, *, prefix: str, encoding: str = "utf-8", mode: "int | None" = None, preserve_owner: bool = True, binary: bool = False, fsync_dir: bool = False) -> None: """Temp file + fsync + :func:`atomic_replace`, then re-apply owner/mode.