From 67680d4b8a90f03d4a6fd6515f1a319b76dba0a0 Mon Sep 17 00:00:00 2001 From: Oscar Gil Date: Sat, 25 Jul 2026 00:52:35 +0200 Subject: [PATCH] fix(extensions): make shipped scripts executable after install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extension archives are unpacked with zipfile.extractall and directory installs are copied; neither restores a stripped Unix mode. A bundled *.sh therefore lands non-executable, so a documented `.specify/extensions//scripts/bash/foo.sh` invocation fails with "Permission denied" — e.g. a CI step that runs an extension's gate. It only worked incidentally, after a later `specify init`. Restore permissions at the shared sink. Every extension install route funnels through ExtensionManager.install_from_directory (install_from_zip delegates to it; extension add, extension update, and bundle installs all reach it), so calling the existing ensure_executable_scripts() there covers every route — present and future — by construction rather than by patching each command. The helper already makes .specify scripts executable (init, migrate, and integration-install all call it); it is called plainly, re-establishing the same idempotent "scripts are executable" invariant those flows restore. Deliberately the whole-project call rather than a scoped one: a scan-scope argument would only spare re-walking already-correct files — negligible beside the copy/extract just performed — while widening a simple, widely-used interface for a single caller. Existing callers were audited: init's end-of-init call still covers core .specify/scripts and is untouched; integration-install and migrate do no manager install. Nothing is removed. No-op on Windows; best-effort per file; does not change which files are executable or their mode. Tests: a manager-level regression test asserts a mode-0644 script comes out executable via both install_from_directory and install_from_zip(force=True) (the latter also covering the remove-then-reinstall shape of extension update), plus an end-to-end `extension add --dev` test. Both fail without the change; skipped on Windows. Fixes #3722. --- src/specify_cli/extensions/__init__.py | 18 ++++++ tests/test_extensions.py | 90 ++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 00a6d92b41..595bba7fb5 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2021,6 +2021,24 @@ def _restore_stranded_config_file( except OSError: pass # Best-effort; install already committed to the registry. + # Restore execute bits on shipped POSIX scripts. copytree here (and the + # zipfile.extractall in install_from_zip, which delegates to this method) does + # not restore a stripped Unix mode, so a bundled *.sh would land non-executable + # and a documented `.specify/extensions//scripts/...` invocation would fail + # with "Permission denied". This is the single sink every install route funnels + # through (extension add / update / bundle), so fixing it here covers them all. + # No-op on Windows (helper returns early). + # + # Deliberately the whole-project call, not a scoped one. The helper's contract + # is "make .specify scripts executable" — the same idempotent invariant that + # init and migrate restore — so re-establishing it after an install is exactly + # its job. A scoped variant would only spare re-walking already-correct files, + # a handful of stats that are negligible beside the copy/extract this method just + # did, and it would cost a per-caller scan-scope argument on an otherwise simple, + # widely-used interface. The simpler call wins. + from .. import ensure_executable_scripts + ensure_executable_scripts(self.project_root) + return manifest def install_from_zip( diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 61826aad5b..1b254de81e 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -1103,6 +1103,52 @@ 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 execute bits are not meaningful on Windows" + ) + def test_install_restores_execute_bit_on_shipped_scripts( + self, extension_dir, project_dir + ): + """Every install route restores execute bits on shipped POSIX scripts. + + ``install_from_directory`` is the single sink all routes funnel through + (``install_from_zip`` delegates to it; ``extension add``, ``extension + update``, and bundle installs all call these two methods). copytree / + zipfile.extractall do not restore a stripped Unix mode, so a shipped + ``*.sh`` would land non-executable and a documented + ``.specify/extensions//scripts/...`` invocation would fail with + "Permission denied". Guards that every route restores the bit; fails + without the ensure_executable_scripts() call in install_from_directory. + """ + import zipfile + import tempfile + + scripts = extension_dir / "scripts" + scripts.mkdir() + script = scripts / "gate.sh" + script.write_text("#!/usr/bin/env bash\necho hi\n") + script.chmod(0o644) # non-executable, as extractall/copytree may leave it + + installed = ( + project_dir / ".specify" / "extensions" / "test-ext" / "scripts" / "gate.sh" + ) + manager = ExtensionManager(project_dir) + + # Route 1 — install_from_directory (extension add --dev, bundle dir install) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + assert os.access(installed, os.X_OK), "directory install left script non-executable" + + # Route 2 — install_from_zip, force=True: the ZIP path (extension add --from, + # bundle zip) and the remove-then-reinstall shape of `extension update`. + with tempfile.TemporaryDirectory() as tmpdir: + zip_path = Path(tmpdir) / "test-ext.zip" + with zipfile.ZipFile(zip_path, "w") as zf: + for f in extension_dir.rglob("*"): + if f.is_file(): + zf.write(f, f.relative_to(extension_dir)) + manager.install_from_zip(zip_path, "0.1.0", force=True) + assert os.access(installed, os.X_OK), "zip/update install left script non-executable" + def test_install_from_directory_explicitly_recovers_active_skills_dir( self, extension_dir, project_dir, monkeypatch ): @@ -6317,6 +6363,50 @@ def test_add_dev_links_copilot_agent_when_supported( else: assert not agent_file.is_symlink() + @pytest.mark.skipif( + os.name == "nt", reason="POSIX execute bits are not meaningful on Windows" + ) + def test_add_makes_shipped_scripts_executable(self, extension_dir, project_dir): + """extension add must restore execute bits on bundled POSIX scripts. + + Archives are unpacked with zipfile.extractall and --dev installs copy the + tree; neither restores a stripped Unix mode, so a shipped *.sh can land + non-executable and a documented `.specify/extensions//scripts/...` + invocation then fails with "Permission denied". init / migrate / + integration-install already call ensure_executable_scripts(); this guards + that `extension add` does too. + """ + import stat + + scripts_dir = extension_dir / "scripts" + scripts_dir.mkdir() + script = scripts_dir / "gate.sh" + script.write_text("#!/usr/bin/env bash\necho hi\n") + script.chmod(0o644) # non-executable, as an unpacked/copied script may be + assert not os.access(script, os.X_OK) + + from typer.testing import CliRunner + from unittest.mock import patch + from specify_cli import app + + runner = CliRunner() + with patch.object(Path, "cwd", return_value=project_dir): + result = runner.invoke( + app, + ["extension", "add", str(extension_dir), "--dev"], + catch_exceptions=True, + ) + + assert result.exit_code == 0, result.output + installed = ( + project_dir / ".specify" / "extensions" / "test-ext" / "scripts" / "gate.sh" + ) + assert installed.exists(), result.output + assert os.access(installed, os.X_OK), ( + f"installed script not executable: mode=" + f"{stat.S_IMODE(installed.stat().st_mode):o}" + ) + def test_add_dev_writes_codex_skills_as_files(self, extension_dir, project_dir): """Codex dev skills should be written as files so Codex can load them.""" from typer.testing import CliRunner