fix(extensions): make shipped scripts executable after extension add#3723
fix(extensions): make shipped scripts executable after extension add#3723ogil109 wants to merge 1 commit into
extension add#3723Conversation
| from .. import ensure_executable_scripts | ||
| ensure_executable_scripts(project_root) |
mnriem
left a comment
There was a problem hiding this comment.
Please address Copilot feedback
c555e4d to
54e4b12
Compare
… (per review) Addresses review feedback on github#3723. The first commit restored execute bits only in `extension_add`, but `extension update` and bundle installs place files through the same manager methods and would still ship non-executable scripts. Move the `ensure_executable_scripts()` call out of `extension_add` and into `ExtensionManager.install_from_directory` — the single sink every install route funnels through (`install_from_zip` delegates to it; add / update / bundle all reach it). This covers every route, present and future, by construction rather than by patching each call site. Audited the existing callers before adding it: `init`'s end-of-init call also covers extensions installed during init (now a harmless idempotent re-scan) and is still needed for core `.specify/scripts`; integration-install and migrate do no manager install and cover only core scripts. So nothing becomes dead and nothing is removed. Adds a manager-level regression test asserting a mode-0644 script ships executable via both `install_from_directory` and `install_from_zip(force=True)` (the latter also exercising the remove-then-reinstall shape of `extension update`). The end-to-end `extension add --dev` test from the first commit stays.
54e4b12 to
fc3d6b5
Compare
|
Thanks for the review — both points led to a better change. Summary of where it landed. What it does. Guarantees that a shipped POSIX script inside an installed extension How it works. All extension file placement funnels through one method, Why this shape.
Scope guarantees: no-op on Windows, best-effort per file (a permission failure never Tests. A manager-level regression test asserts a mode- |
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/<id>/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 github#3722.
049f0ea to
67680d4
Compare
Description
specify extension add(andextension update, and bundle installs) leave shipped POSIXscripts non-executable, so a documented
.specify/extensions/<id>/scripts/bash/foo.shinvocation fails with
Permission denied— e.g. a CI step that runs an extension's gate.Archives are unpacked with
zipfile.extractalland directory installs are copied; neitherrestores a stripped Unix mode.
Fix at the shared sink, not per-command. Every extension install route funnels through
ExtensionManager.install_from_directory(install_from_zipdelegates to it;extension add,extension update, and bundle installs all reach it). Restoring perms there coversevery route — present and future — by construction, rather than patching each call site.
extension add --from <zip>install_from_zip→install_from_directoryextension add <dir> --devinstall_from_directoryextension updateinstall_from_zipinstall_from_zip/install_from_directoryReuses
ensure_executable_scripts()as-is. That helper already exists and alreadymakes
.specifyscripts executable (init,migrate, and integration-install all callit); the extension install routes were simply the ones that never did. The sink calls it
plainly, re-establishing the same idempotent "scripts are executable" invariant those
other flows restore.
Design note: the sink calls the helper plainly rather than scoping it to just the installed extension. A scan-scope argument would only spare re-walking a handful of already-correct files — negligible beside the copy/extract the install just performed — while widening a simple, four-caller interface for one caller and pushing a "which roots to scan" decision up to callers the helper should own. The whole-project call is the deliberate choice: simplest interface, sensible default, correct for every caller.
No dead code introduced. Existing callers were audited:
init's end-of-init call stillcovers core
.specify/scriptsand is untouched; integration-install / migrate do nomanager install and cover only core scripts. Nothing was removed.
No-op on Windows (helper returns early); best-effort per file; does not change which
files are executable or their mode.
Fixes #3722.
Testing
uv run specify --helpuv sync && uv run pytest—tests/test_extensions.pyandtests/test_presets.pygreen (765 passed), including the new tests.archive via the real CLI: before →
-rw-r--r--+Permission denied; after →-rwxr-xr-x, runs.New
TestExtensionManager::test_install_restores_execute_bit_on_shipped_scriptsasserts amode-
0644script comes out executable via bothinstall_from_directoryandinstall_from_zip(force=True)(the latter also exercising the remove-then-reinstall shapeof
extension update). Plus the end-to-endextension add --devtest. Both fail withoutthe change; skipped on Windows.
AI Disclosure
Fix and tests written with Claude Code (Claude Opus). Found verifying a community extension
of mine, root-caused, and — since it became a cross-cutting change across install routes —
worked the design as a small spec (route inventory + sink-vs-per-route decision + dead-code
audit) before landing it. Verified end-to-end against a CLI built from current
main.