diff --git a/src/specify_cli/_utils.py b/src/specify_cli/_utils.py index 30c59f553a..f7a9724fcc 100644 --- a/src/specify_cli/_utils.py +++ b/src/specify_cli/_utils.py @@ -149,13 +149,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: @@ -238,6 +238,39 @@ 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 & 0o7777) | 0o300) + except OSError: + pass + + +def copy_file_preserving_exec(src: str, dst: str) -> None: + """Copy file content without propagating source mtime or read-only bits. + + shutil.copyfile transfers only content, leaving destination permissions at + the umask default (typically 0o644). This wrapper additionally re-applies + any execute bits present on the source so that installed shell scripts + remain executable while still being owner-writable. + """ + shutil.copyfile(src, dst) + if os.name != "nt": + exec_bits = os.stat(src).st_mode & 0o111 + dst_mode = os.stat(dst).st_mode & 0o7777 + os.chmod(dst, dst_mode | 0o200 | exec_bits) + + 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 997b9ee679..198d5c3813 100644 --- a/src/specify_cli/commands/init.py +++ b/src/specify_cli/commands/init.py @@ -468,7 +468,7 @@ def init( 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 ecc26fa877..049e84b48e 100644 --- a/src/specify_cli/extensions.py +++ b/src/specify_cli/extensions.py @@ -28,7 +28,7 @@ from ._init_options import is_ai_skills_enabled from ._invocation_style import is_slash_skills_agent -from ._utils import dump_frontmatter +from ._utils import copy_file_preserving_exec, dump_frontmatter, ensure_writable_tree from .catalogs import CatalogEntry as BaseCatalogEntry from .catalogs import CatalogStackBase @@ -1378,7 +1378,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=copy_file_preserving_exec) + ensure_writable_tree(dest_dir) # Register commands with AI agents registered_commands = {} diff --git a/src/specify_cli/integrations/base.py b/src/specify_cli/integrations/base.py index fbab1a2b37..e886399d6d 100644 --- a/src/specify_cli/integrations/base.py +++ b/src/specify_cli/integrations/base.py @@ -441,7 +441,7 @@ def copy_command_to_directory( """ dest_dir.mkdir(parents=True, exist_ok=True) dst = dest_dir / filename - shutil.copy2(src, dst) + shutil.copyfile(src, dst) return dst @staticmethod @@ -517,9 +517,9 @@ def install_scripts( if not src_script.is_file(): continue dst_script = scripts_dest / src_script.name - shutil.copy2(src_script, dst_script) + shutil.copyfile(src_script, dst_script) if dst_script.suffix == ".sh": - dst_script.chmod(dst_script.stat().st_mode | 0o111) + dst_script.chmod((dst_script.stat().st_mode & 0o7777) | 0o111) self.record_file_in_manifest(dst_script, project_root, manifest) created.append(dst_script) diff --git a/src/specify_cli/integrations/copilot/__init__.py b/src/specify_cli/integrations/copilot/__init__.py index 71b6d6919d..229b1b2a50 100644 --- a/src/specify_cli/integrations/copilot/__init__.py +++ b/src/specify_cli/integrations/copilot/__init__.py @@ -391,7 +391,7 @@ def _setup_default( # remove the user's settings file on uninstall. self._merge_vscode_settings(settings_src, dst_settings) else: - shutil.copy2(settings_src, dst_settings) + shutil.copyfile(settings_src, dst_settings) self.record_file_in_manifest(dst_settings, project_root, manifest) created.append(dst_settings) diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index f8b9bac698..803f13aee8 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -29,6 +29,7 @@ from ..extensions import REINSTALL_COMMAND, ExtensionRegistry, normalize_priority from .._init_options import is_ai_skills_enabled +from .._utils import copy_file_preserving_exec, ensure_writable_tree from ..integrations.base import IntegrationBase from .._utils import dump_frontmatter @@ -1535,7 +1536,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=copy_file_preserving_exec) + 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. diff --git a/tests/integrations/test_base.py b/tests/integrations/test_base.py index 47f9d09059..33f4f48774 100644 --- a/tests/integrations/test_base.py +++ b/tests/integrations/test_base.py @@ -1,7 +1,13 @@ """Tests for IntegrationOption, IntegrationBase, MarkdownIntegration, and primitives.""" +import os + import pytest +_SKIP_WINDOWS = pytest.mark.skipif( + os.name == "nt", reason="POSIX mode bits are not stable on Windows" +) + from specify_cli.integrations.base import ( IntegrationBase, IntegrationOption, @@ -149,6 +155,15 @@ def test_copy_command_to_directory(self, tmp_path): assert result == dest_dir / "speckit.plan.md" assert result.read_text(encoding="utf-8") == "content" + @_SKIP_WINDOWS + def test_copy_command_to_directory_readonly_source_is_writable(self, tmp_path): + src = tmp_path / "source.md" + src.write_text("content", encoding="utf-8") + src.chmod(0o444) + dest_dir = tmp_path / "output" + result = IntegrationBase.copy_command_to_directory(src, dest_dir, "speckit.plan.md") + assert result.stat().st_mode & 0o200, "destination must be owner-writable" + def test_record_file_in_manifest(self, tmp_path): f = tmp_path / "f.txt" f.write_text("hello", encoding="utf-8") @@ -164,6 +179,28 @@ def test_write_file_and_record(self, tmp_path): assert dest.read_text(encoding="utf-8") == "content" assert "sub/f.txt" in m.files + @_SKIP_WINDOWS + def test_install_scripts_readonly_source_files_are_writable(self, tmp_path, monkeypatch): + scripts_src = tmp_path / "scripts_src" + scripts_src.mkdir() + helper = scripts_src / "helper.sh" + helper.write_text("#!/bin/sh\necho hi\n") + helper.chmod(0o444) + data = scripts_src / "data.txt" + data.write_text("payload") + data.chmod(0o444) + + project = tmp_path / "project" + project.mkdir() + i = StubIntegration() + m = IntegrationManifest("stub", project) + monkeypatch.setattr(i, "integration_scripts_dir", lambda: scripts_src) + created = i.install_scripts(project, m) + + assert len(created) == 2 + for dst in created: + assert dst.stat().st_mode & 0o200, f"{dst.name} must be owner-writable" + def test_setup_copies_shared_templates(self, tmp_path): i = StubIntegration() m = IntegrationManifest("stub", tmp_path) diff --git a/tests/integrations/test_integration_copilot.py b/tests/integrations/test_integration_copilot.py index 6b7cc7c13f..21a4c12c08 100644 --- a/tests/integrations/test_integration_copilot.py +++ b/tests/integrations/test_integration_copilot.py @@ -3,6 +3,7 @@ import json import os +import pytest import yaml from specify_cli.integrations import get_integration @@ -67,6 +68,21 @@ def test_setup_creates_vscode_settings_new(self, tmp_path): assert settings in created assert any("settings.json" in k for k in m.files) + @pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are not stable on Windows") + def test_setup_vscode_settings_readonly_source_is_writable(self, tmp_path, monkeypatch): + from specify_cli.integrations.copilot import CopilotIntegration + import json as _json + copilot = CopilotIntegration() + # Simulate a bundled settings file with read-only store permissions. + ro_src = tmp_path / "ro_settings.json" + ro_src.write_text(_json.dumps({"foo": "bar"})) + ro_src.chmod(0o444) + monkeypatch.setattr(copilot, "_vscode_settings_path", lambda: ro_src) + m = IntegrationManifest("copilot", tmp_path) + copilot.setup(tmp_path, m) + settings = tmp_path / ".vscode" / "settings.json" + assert settings.stat().st_mode & 0o200, ".vscode/settings.json must be owner-writable" + def test_setup_merges_existing_vscode_settings(self, tmp_path): from specify_cli.integrations.copilot import CopilotIntegration copilot = CopilotIntegration() diff --git a/tests/test_extensions.py b/tests/test_extensions.py index e063571b14..016ab8fb67 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 @@ -984,6 +985,91 @@ 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) + + @pytest.mark.skipif(os.name == "nt", reason="POSIX mode bits are not stable on Windows") + def test_install_from_directory_preserves_executable_bits( + self, extension_dir, project_dir + ): + """Shell scripts inside an extension must remain executable after install.""" + scripts_dir = extension_dir / "scripts" / "bash" + scripts_dir.mkdir(parents=True) + script = scripts_dir / "run.sh" + script.write_text("#!/bin/sh\necho ok\n") + script.chmod(0o755) + + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + installed_script = ( + project_dir / ".specify" / "extensions" / "test-ext" / "scripts" / "bash" / "run.sh" + ) + assert installed_script.exists() + mode = stat.S_IMODE(installed_script.stat().st_mode) + assert mode & 0o111, f"Installed script lost execute bits: {oct(mode)}" + assert mode & 0o200, f"Installed script is not owner-writable: {oct(mode)}" + 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 de6054d99c..0ec631ec19 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -13,6 +13,8 @@ import pytest import io import json +import os +import stat import tempfile import shutil import warnings @@ -592,6 +594,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..01ba3e6218 --- /dev/null +++ b/tests/test_utils.py @@ -0,0 +1,190 @@ +"""Unit tests for specify_cli._utils – ensure_writable_tree(), copy_file_preserving_exec().""" + +from __future__ import annotations + +import os +import stat +from pathlib import Path + +import pytest + +from specify_cli._utils import copy_file_preserving_exec, 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)}" + + +# -- copy_file_preserving_exec tests ------------------------------------------ + + +@_SKIP_WINDOWS +def test_copy_file_preserving_exec_executable_source(tmp_path: Path) -> None: + """Execute bits present on the source must appear on the destination.""" + src = tmp_path / "script.sh" + src.write_text("#!/bin/sh\necho hi\n") + src.chmod(0o755) + + dst = tmp_path / "out" / "script.sh" + dst.parent.mkdir() + copy_file_preserving_exec(str(src), str(dst)) + + mode = stat.S_IMODE(dst.stat().st_mode) + assert mode & 0o111, f"Execute bits missing on destination: {oct(mode)}" + assert mode & 0o200, f"Owner write bit missing on destination: {oct(mode)}" + + +@_SKIP_WINDOWS +def test_copy_file_preserving_exec_non_executable_source(tmp_path: Path) -> None: + """Execute bits absent on the source must not appear on the destination.""" + src = tmp_path / "data.json" + src.write_text('{"key": "value"}') + src.chmod(0o644) + + dst = tmp_path / "out" / "data.json" + dst.parent.mkdir() + copy_file_preserving_exec(str(src), str(dst)) + + mode = stat.S_IMODE(dst.stat().st_mode) + assert not (mode & 0o111), f"Unexpected execute bits on destination: {oct(mode)}" + assert mode & 0o200, f"Owner write bit missing on destination: {oct(mode)}" + + +@_SKIP_WINDOWS +def test_copy_file_preserving_exec_readonly_source_is_writable(tmp_path: Path) -> None: + """Source read-only bits must not propagate to the destination.""" + src = tmp_path / "locked.txt" + src.write_text("content") + src.chmod(0o444) + + dst = tmp_path / "out" / "locked.txt" + dst.parent.mkdir() + copy_file_preserving_exec(str(src), str(dst)) + + mode = stat.S_IMODE(dst.stat().st_mode) + assert mode & 0o200, f"Destination is not owner-writable: {oct(mode)}" + + +@_SKIP_WINDOWS +def test_copy_file_preserving_exec_copies_content(tmp_path: Path) -> None: + """File content must be faithfully copied.""" + payload = "hello world\n" + src = tmp_path / "src.txt" + src.write_text(payload) + + dst = tmp_path / "dst.txt" + copy_file_preserving_exec(str(src), str(dst)) + + assert dst.read_text() == payload