fix: install fails when the packaged bundle is read-only - #2453
fix: install fails when the packaged bundle is read-only#2453bensleveritt wants to merge 1 commit into
Conversation
_install_skill_references stages references/ with shutil.copytree, which preserves the source's mode bits. When the package lives somewhere read-only those are 0o555, and renaming a directory needs write permission on the directory itself to update its '..' entry, so os.replace raises PermissionError (EACCES) and the whole install aborts. Restore owner-write on the staged copy before the rename. The atomic staging design is unchanged. Affects any read-only install location: Nix store paths, root-owned site-packages, container image layers, read-only pipx venvs.
There was a problem hiding this comment.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
This PR modifies the skill references installation logic in graphify/install.py so that, after copying a references directory into a staging location, it restores owner-write permission on the staged copy and all its contents before the atomic rename. The stated intent is to handle packaged bundles that are read-only (e.g. Nix store paths, root-owned site-packages, container layers), where the preserved read-only mode bits would otherwise cause the rename to fail. It also adds a new test (test_install_from_read_only_package_dir) that sets the source bundle to read-only modes, runs an install, and asserts the references land correctly and remain writable. The changed symbol list appears to be inflated with many unrelated identifiers, but the actual diff only touches _install_skill_references and adds one test function.
Worth a look
- Non-atomic check-then-act: rmtree then replace leaves window with no references dir —
graphify/install.py:176· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 393 functions depend on the 181 functions this change touches.
Health — grade B; 10 existing hotspot(s) in the area this change touches (pre-existing, not introduced here):
dispatch_command()— 2 callers, 110 callees (high)codebuddy_install()— 20 callers, 4 callees (high)dispatch_install_cli()— 2 callers, 31 callees (high)_project_uninstall()— 5 callers, 12 callees (high)gemini_install()— 10 callers, 6 callees (high)claude_install()— 19 callers, 3 callees (high)claude_uninstall()— 17 callers, 3 callees (high)install()— 5 callers, 10 callees (high)- …and 2 more
Verification — 393 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 342 function(s) in the blast radius were not formally verified this run
The bug
graphify installaborts when the installed package sits somewhere read-only:_install_skill_referencesstages the packaged sidecar withshutil.copytree, which preserves the source's mode bits. Where the package is read-only those are0o555. Renaming a directory requires write permission on the directory itself, because the rename updates its..entry — soos.replace(refs_staged, refs_dst)fails withEACCES, theexceptclause cleans up, and the install dies beforeSKILL.mdis written.Minimal reproduction, independent of graphify:
Who hits it
Anywhere the package directory is not writable:
0o555— this is where I found itsite-packagesunder a system-managed PythonIt is not a niche-installer problem: nothing about the code path is Nix-specific, and the failure is deterministic rather than intermittent.
The fix
Restore owner-write on the staged copy before the rename. Eight lines including the comment, and the atomic staging design is untouched —
references.tmpis still built fully, then renamed into place in one step, so an interrupted install still never exposes a half-writtenreferences/.Test
test_install_from_read_only_package_dirintests/test_install_references.py, built on the existingfake_bundlefixture and_installhelper. It chmods the packaged bundle to0o555, installs, and restores the original modes in afinallyso teardown can still clean up.Verified it is a real regression test: with the fix reverted it fails with the same
PermissionError: [Errno 13], and it passes with the fix applied. The wider install suite stays green — 209 passed, 1 skipped acrosstest_install*,test_uninstall_scope,test_agents_platformandtest_atomic_writes.One thing I left alone
_copy_skill_filecopiesSKILL.mdwithshutil.copy, which also preserves mode bits, so from a read-only package the installedSKILL.mdlands0o444. That does not break anything — renaming a file only needs write permission on the parent directory, so upgrades still work — and users rarely edit it. I have kept this PR to the failure that actually blocks installation, but say the word and I will switch it toshutil.copyfilein a follow-up.Context
Found while packaging graphify as a Nix flake (bensleveritt/graphify-nix). Happy to adjust naming or placement to suit your conventions.