fix: ensure installed files are owner-writable regardless of source permissions#2884
Open
rayhem wants to merge 5 commits into
Open
fix: ensure installed files are owner-writable regardless of source permissions#2884rayhem wants to merge 5 commits into
rayhem wants to merge 5 commits into
Conversation
…ermissions 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 <noreply@anthropic.com>
mnriem
requested changes
Jun 8, 2026
mnriem
left a comment
mnriem
left a comment
Collaborator
There was a problem hiding this comment.
Please add some positive and negative tests
…ad-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
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a packaging/installation pitfall where shutil.copy2()/copytree() preserve source permission bits, causing files and directories installed from read-only sources (e.g., Nix store) to become non-writable in the project and later fail with PermissionError. It switches key copy operations to copyfile() (to avoid propagating perms/mtime) and introduces a helper to make copied directory trees owner-writable.
Changes:
- Add
ensure_writable_tree()and apply it aftercopytree()installs to force destination directories to be owner-writable. - Replace
copy2usage withcopyfilein several install/copy paths to avoid inheriting read-only perms and source mtimes. - Add regression tests covering read-only source trees for extensions/presets and unit tests for
ensure_writable_tree().
Show a summary per file
| File | Description |
|---|---|
src/specify_cli/_utils.py |
Adds ensure_writable_tree() and switches settings copy fallback from copy2 to copyfile. |
src/specify_cli/presets.py |
Uses copytree(..., copy_function=copyfile) and applies ensure_writable_tree() after preset installs. |
src/specify_cli/extensions.py |
Uses copytree(..., copy_function=copyfile) and applies ensure_writable_tree() after extension installs. |
src/specify_cli/commands/init.py |
Switches bundled workflow copy from copy2 to copyfile to avoid inheriting perms/mtime. |
tests/test_utils.py |
New unit tests for ensure_writable_tree() behavior (POSIX-focused). |
tests/test_presets.py |
Adds regression tests ensuring preset installs from read-only sources yield writable destinations. |
tests/test_extensions.py |
Adds regression tests ensuring extension installs from read-only sources yield writable destinations. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 1
Comment on lines
+233
to
+237
| 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. | ||
| """ |
Two install paths missed in f70bdf9: copy_command_to_directory(), install_scripts() in IntegrationBase, and the vscode-settings.json copy in CopilotIntegration all used copy2, propagating 0o444/0o555 from bundled package data (Nix store) to destination files. Switch to copyfile at all three sites. Add permission regression tests for each. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| for dirpath, _, _ in os.walk(path): | ||
| dp = Path(dirpath) | ||
| try: | ||
| dp.chmod(dp.stat().st_mode | 0o300) |
| 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) |
Comment on lines
1360
to
+1362
| 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) |
Comment on lines
+1538
to
+1539
| shutil.copytree(source_dir, dest_dir, copy_function=shutil.copyfile) | ||
| ensure_writable_tree(dest_dir) |
Collaborator
|
Please address Copilot feedback and rebase on upstream/main |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
shutil.copy2andshutil.copytree(which usescopy2by default) propagate permission bits from source to destination. When the source lives on a read-only filesystem — the Nix store, any read-only mount, or a permission-restricted directory — installed files land at0o444and directories at0o555. Any subsequent write to.specify/(re-runningspecify init, upgrading, editing installed config) then fails withPermissionError.An install operation should produce owner-writable destinations. The installed file's mtime should also reflect when it was installed, not when the bundled asset was built — so the
copy2→copyfilechange is correct on both counts.I encountered this problem attempting to package spec-kit on NixOS. The repository gets added read-only to the Nix store.
specifyassumes it can write to the files it creates, but the copy utilities preserve the read-only permissions resulting in an error.Testing
uv run specify --helpuv sync && uv run pytestAI Disclosure
Fix devised and generated by Claude Opus 4.6