From f70bdf94fa1be2fbc8984d821e7b583a4188d00d Mon Sep 17 00:00:00 2001 From: rayhem Date: Sat, 6 Jun 2026 12:24:36 -0400 Subject: [PATCH 1/2] fix: ensure installed files are owner-writable regardless of source permissions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shutil.copy2 and copytree propagate permission bits from the source, leaving destination files at 0o444 and dirs at 0o555 when copying from any read-only source (Nix store, read-only mounts, etc.). Subsequent writes to .specify/ then fail with PermissionError. Replace copy2 with copyfile (content-only, no permission bits) at all four install-path call sites. Add ensure_writable_tree() to fix directory permissions after copytree calls — copytree always stamps dest dirs via copystat() regardless of copy_function. Co-Authored-By: Claude Sonnet 4.6 --- src/specify_cli/_utils.py | 22 ++++++++++++++++++++-- src/specify_cli/commands/init.py | 2 +- src/specify_cli/extensions.py | 4 +++- src/specify_cli/presets.py | 4 +++- 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/_utils.py b/src/specify_cli/_utils.py index beae253593..d009f25866 100644 --- a/src/specify_cli/_utils.py +++ b/src/specify_cli/_utils.py @@ -183,13 +183,13 @@ def atomic_write_json(target_file: Path, payload: dict[str, Any]) -> None: else: log("Skipped merge (preserved existing settings)", "yellow") else: - shutil.copy2(sub_item, dest_file) + shutil.copyfile(sub_item, dest_file) log("Copied (no existing settings.json):", "blue") except Exception as e: log(f"Warning: Could not merge settings: {e}", "yellow") if not dest_file.exists(): - shutil.copy2(sub_item, dest_file) + shutil.copyfile(sub_item, dest_file) def merge_json_files(existing_path: Path, new_content: Any, verbose: bool = False) -> dict[str, Any] | None: @@ -272,6 +272,24 @@ def deep_merge_polite(base: dict[str, Any], update: dict[str, Any]) -> dict[str, return merged +def ensure_writable_tree(path: Path) -> None: + """Add owner write+execute to every directory under path. + + shutil.copytree always calls copystat() on directories, propagating + read-only bits from any read-only source. Call this after copytree to + guarantee destination directories accept writes regardless of source + permissions. + """ + if os.name == "nt": + return + for dirpath, _, _ in os.walk(path): + dp = Path(dirpath) + try: + dp.chmod(dp.stat().st_mode | 0o300) + except OSError: + pass + + def _display_project_path(project_root: Path, path: str | Path) -> str: """Return a stable POSIX-style display path for paths under a project.""" path_obj = Path(path) diff --git a/src/specify_cli/commands/init.py b/src/specify_cli/commands/init.py index 68f5bed31f..4a606ab970 100644 --- a/src/specify_cli/commands/init.py +++ b/src/specify_cli/commands/init.py @@ -430,7 +430,7 @@ def init( import shutil as _shutil dest_wf = project_path / ".specify" / "workflows" / "speckit" dest_wf.mkdir(parents=True, exist_ok=True) - _shutil.copy2( + _shutil.copyfile( bundled_wf / "workflow.yml", dest_wf / "workflow.yml", ) diff --git a/src/specify_cli/extensions.py b/src/specify_cli/extensions.py index bddf637cbc..a9ce528709 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -27,6 +27,7 @@ from .catalogs import CatalogEntry as BaseCatalogEntry, CatalogStackBase from ._init_options import is_ai_skills_enabled +from ._utils import ensure_writable_tree _FALLBACK_CORE_COMMAND_NAMES = frozenset({ "analyze", @@ -1280,7 +1281,8 @@ def install_from_directory( shutil.rmtree(dest_dir) ignore_fn = self._load_extensionignore(source_dir) - shutil.copytree(source_dir, dest_dir, ignore=ignore_fn) + shutil.copytree(source_dir, dest_dir, ignore=ignore_fn, copy_function=shutil.copyfile) + ensure_writable_tree(dest_dir) # Register commands with AI agents registered_commands = {} diff --git a/src/specify_cli/presets.py b/src/specify_cli/presets.py index 47b4240d85..4c93651c79 100644 --- a/src/specify_cli/presets.py +++ b/src/specify_cli/presets.py @@ -30,6 +30,7 @@ from .extensions import REINSTALL_COMMAND, ExtensionRegistry, normalize_priority from .integrations.base import IntegrationBase from ._init_options import is_ai_skills_enabled +from ._utils import ensure_writable_tree def _substitute_core_template( @@ -1534,7 +1535,8 @@ def install_from_directory( if dest_dir.exists(): shutil.rmtree(dest_dir) - shutil.copytree(source_dir, dest_dir) + shutil.copytree(source_dir, dest_dir, copy_function=shutil.copyfile) + ensure_writable_tree(dest_dir) # Pre-register the preset so that composition resolution can see it # in the priority stack when resolving composed command content. From 8dc734c6240071a8a134288efad95812d5f45b2a Mon Sep 17 00:00:00 2001 From: rayhem Date: Sat, 13 Jun 2026 14:44:06 -0400 Subject: [PATCH 2/2] test: add positive and negative tests for ensure_writable_tree and read-only source installs Unit tests for ensure_writable_tree() in test_utils.py: - read-only dirs gain owner w+x bits - existing writable permissions preserved - nested read-only tree fully fixed - file permissions left untouched - no-op on Windows (os.name == 'nt') - OSError on individual chmod silently swallowed - empty directory handled without error Integration tests in test_extensions.py and test_presets.py: - install_from_directory with read-only source produces writable dest dirs - install_from_directory with read-only source produces writable dest files --- tests/test_extensions.py | 64 ++++++++++++++++++++ tests/test_presets.py | 58 ++++++++++++++++++ tests/test_utils.py | 127 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 249 insertions(+) create mode 100644 tests/test_utils.py diff --git a/tests/test_extensions.py b/tests/test_extensions.py index e46fdf4353..39a885233e 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -13,6 +13,7 @@ import json import os import platform +import stat import tempfile import shutil import tomllib @@ -782,6 +783,69 @@ def test_install_from_directory(self, extension_dir, project_dir): assert (ext_dir / "extension.yml").exists() assert (ext_dir / "commands" / "hello.md").exists() + @pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are not stable on Windows") + def test_install_from_directory_readonly_source_produces_writable_dest( + self, extension_dir, project_dir + ): + """Directories copied from a read-only source must be owner-writable.""" + # Lock the source tree to simulate a Nix store / read-only mount + for dirpath, _, filenames in os.walk(extension_dir): + dp = Path(dirpath) + for fn in filenames: + (dp / fn).chmod(0o444) + dp.chmod(0o555) + + try: + manager = ExtensionManager(project_dir) + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + for dirpath, _, _ in os.walk(ext_dir): + mode = stat.S_IMODE(Path(dirpath).stat().st_mode) + assert mode & 0o200, ( + f"{dirpath} is not owner-writable: {oct(mode)}" + ) + finally: + # Restore writable perms so the temp_dir fixture can clean up + for dirpath, _, filenames in os.walk(extension_dir): + dp = Path(dirpath) + dp.chmod(0o755) + for fn in filenames: + (dp / fn).chmod(0o644) + + @pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are not stable on Windows") + def test_install_from_directory_readonly_source_files_are_writable( + self, extension_dir, project_dir + ): + """Files copied via copyfile should inherit default umask, not source perms.""" + for dirpath, _, filenames in os.walk(extension_dir): + dp = Path(dirpath) + for fn in filenames: + (dp / fn).chmod(0o444) + dp.chmod(0o555) + + try: + manager = ExtensionManager(project_dir) + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False + ) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + for dirpath, _, filenames in os.walk(ext_dir): + for fn in filenames: + fp = Path(dirpath) / fn + # Functional check: the file must be openable for writing + with open(fp, "a") as fh: + fh.write("") + finally: + for dirpath, _, filenames in os.walk(extension_dir): + dp = Path(dirpath) + dp.chmod(0o755) + for fn in filenames: + (dp / fn).chmod(0o644) + def test_install_from_directory_explicitly_recovers_active_skills_dir( self, extension_dir, project_dir, monkeypatch ): diff --git a/tests/test_presets.py b/tests/test_presets.py index 9c2d3a508f..e037e6adab 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -12,6 +12,8 @@ import pytest import json +import os +import stat import tempfile import shutil import warnings @@ -590,6 +592,62 @@ def test_install_from_directory(self, project_dir, pack_dir): assert (installed_dir / "preset.yml").exists() assert (installed_dir / "templates" / "spec-template.md").exists() + @pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are not stable on Windows") + def test_install_from_directory_readonly_source_produces_writable_dest( + self, project_dir, pack_dir + ): + """Directories copied from a read-only source must be owner-writable.""" + for dirpath, _, filenames in os.walk(pack_dir): + dp = Path(dirpath) + for fn in filenames: + (dp / fn).chmod(0o444) + dp.chmod(0o555) + + try: + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + installed_dir = project_dir / ".specify" / "presets" / "test-pack" + for dirpath, _, _ in os.walk(installed_dir): + mode = stat.S_IMODE(Path(dirpath).stat().st_mode) + assert mode & 0o200, ( + f"{dirpath} is not owner-writable: {oct(mode)}" + ) + finally: + for dirpath, _, filenames in os.walk(pack_dir): + dp = Path(dirpath) + dp.chmod(0o755) + for fn in filenames: + (dp / fn).chmod(0o644) + + @pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are not stable on Windows") + def test_install_from_directory_readonly_source_files_are_writable( + self, project_dir, pack_dir + ): + """Files copied via copyfile should inherit default umask, not source perms.""" + for dirpath, _, filenames in os.walk(pack_dir): + dp = Path(dirpath) + for fn in filenames: + (dp / fn).chmod(0o444) + dp.chmod(0o555) + + try: + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + + installed_dir = project_dir / ".specify" / "presets" / "test-pack" + for dirpath, _, filenames in os.walk(installed_dir): + for fn in filenames: + fp = Path(dirpath) / fn + with open(fp, "a") as fh: + fh.write("") + finally: + for dirpath, _, filenames in os.walk(pack_dir): + dp = Path(dirpath) + dp.chmod(0o755) + for fn in filenames: + (dp / fn).chmod(0o644) + def test_install_already_installed(self, project_dir, pack_dir): """Test installing an already-installed pack raises error.""" manager = PresetManager(project_dir) diff --git a/tests/test_utils.py b/tests/test_utils.py new file mode 100644 index 0000000000..65727c8be9 --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,127 @@ +"""Unit tests for specify_cli._utils – ensure_writable_tree().""" + +from __future__ import annotations + +import os +import stat +from pathlib import Path + +import pytest + +from specify_cli._utils import ensure_writable_tree + +_SKIP_WINDOWS = pytest.mark.skipif( + os.name == "nt", reason="POSIX mode bits are not stable on Windows" +) + + +# -- Positive tests ----------------------------------------------------------- + + +@_SKIP_WINDOWS +def test_ensure_writable_tree_adds_owner_write_to_readonly_dirs(tmp_path: Path) -> None: + """Read-only directories should gain owner write+execute bits.""" + child = tmp_path / "a" + child.mkdir() + child.chmod(0o555) + + ensure_writable_tree(tmp_path) + + mode = stat.S_IMODE(child.stat().st_mode) + assert mode & 0o300 == 0o300, f"Expected owner w+x bits set, got {oct(mode)}" + + +@_SKIP_WINDOWS +def test_ensure_writable_tree_preserves_existing_permissions(tmp_path: Path) -> None: + """Directories that are already writable should keep their existing bits.""" + child = tmp_path / "a" + child.mkdir() + child.chmod(0o755) + + ensure_writable_tree(tmp_path) + + mode = stat.S_IMODE(child.stat().st_mode) + assert mode == 0o755, f"Expected 0o755 unchanged, got {oct(mode)}" + + +@_SKIP_WINDOWS +def test_ensure_writable_tree_handles_nested_readonly_dirs(tmp_path: Path) -> None: + """All levels of a deeply nested read-only tree should become writable.""" + # Build bottom-up so we can still mkdir before locking + a = tmp_path / "a" + b = a / "b" + c = b / "c" + c.mkdir(parents=True) + + # Lock from the bottom up + for d in (c, b, a): + d.chmod(0o555) + + ensure_writable_tree(tmp_path) + + for d in (a, b, c): + mode = stat.S_IMODE(d.stat().st_mode) + assert mode & 0o300 == 0o300, f"{d} missing owner w+x: {oct(mode)}" + + +@_SKIP_WINDOWS +def test_ensure_writable_tree_does_not_touch_files(tmp_path: Path) -> None: + """File permissions must be left untouched – only directories are fixed.""" + f = tmp_path / "readonly.txt" + f.write_text("hello") + f.chmod(0o444) + + ensure_writable_tree(tmp_path) + + mode = stat.S_IMODE(f.stat().st_mode) + assert mode == 0o444, f"File mode should be unchanged, got {oct(mode)}" + + +# -- Negative / edge-case tests ---------------------------------------------- + + +def test_ensure_writable_tree_noop_on_windows(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """On Windows (os.name == 'nt'), the function should return immediately.""" + monkeypatch.setattr(os, "name", "nt") + + child = tmp_path / "a" + child.mkdir() + + # Capture the mode before the call (may not be meaningful on real Windows, + # but this test runs on any OS with os.name faked). + before = child.stat().st_mode + + ensure_writable_tree(tmp_path) + + after = child.stat().st_mode + assert before == after, "Permissions should not change when os.name == 'nt'" + + +@_SKIP_WINDOWS +def test_ensure_writable_tree_swallows_oserror(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """OSError on individual chmod calls must be silently swallowed.""" + child = tmp_path / "a" + child.mkdir() + + original_chmod = Path.chmod + + def exploding_chmod(self: Path, mode: int) -> None: + if self == child: + raise OSError("synthetic permission error") + original_chmod(self, mode) + + monkeypatch.setattr(Path, "chmod", exploding_chmod) + + # Must not raise + ensure_writable_tree(tmp_path) + + +@_SKIP_WINDOWS +def test_ensure_writable_tree_empty_directory(tmp_path: Path) -> None: + """An empty directory should itself gain write bits without error.""" + tmp_path.chmod(0o555) + + ensure_writable_tree(tmp_path) + + mode = stat.S_IMODE(tmp_path.stat().st_mode) + assert mode & 0o300 == 0o300, f"Root dir missing owner w+x: {oct(mode)}"