fix(windows): read-only directory removal, and a CLI crash without a home dir - #2410
fix(windows): read-only directory removal, and a CLI crash without a home dir#2410NodirbekGaniyev14 wants to merge 3 commits into
Conversation
The lockfile still pinned graphifyy 0.9.31 while pyproject.toml declares 0.9.32, so every `uv sync` / `uv run` rewrote uv.lock as a side effect and left the working tree dirty. Regenerated with `uv lock`; the resolution is otherwise unchanged (single line). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
shutil.rmtree fails with PermissionError (WinError 5) on the final rmdir when a directory carries FILE_ATTRIBUTE_READONLY, which Windows derives from a missing owner-write bit. OneDrive marks the directories it syncs read-only, and copytree's copystat hands that attribute to every copy, so a references/ sidecar installed from a OneDrive-synced package could never be removed again. Add clear_readonly() and rmtree() to graphify.paths (already the shared home for filesystem helpers) and route every removal through them: - install.py: all 7 rmtree call sites. _install_skill_references also strips the attribute from the staged copy, so a fresh install is writable in the first place rather than merely removable afterwards. - cache.py: _cleanup_stale_ast_entries silently skipped read-only entries (rmtree with ignore_errors=True, unlink inside `except OSError: pass`), so AST cache entries written by older versions accumulated forever. - test_install_references.py: the fake_bundle teardown cleared the staged bundle with ignore_errors=True, then shutil.move'd the real one back. On a read-only dir the clear failed silently and move NESTED the backup instead of replacing it, deleting the committed graphify/skills/claude/references/ from the working tree on every run. It now uses the robust rmtree and asserts the slot is empty before moving, so the real bundle stays safe in its temp dir if it is not. Windows test failures drop from 114 to 43. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_run_cli resolves every platform's skill destination up front to check for a stale version stamp. Resolving one needs a home directory, and in an environment without HOME/USERPROFILE (`env -i`, a bare CI container, a service account) Path.home() raises RuntimeError. That propagated out of an advisory check and killed even `graphify --version`, a command that touches no filesystem at all. Skip the stamp check when the destinations cannot be resolved, and pin the behaviour with a regression test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 replaces direct shutil.rmtree and file-deletion calls across cache.py, install.py, and a test helper with new shared helpers (rmtree and clear_readonly) added to graphify/paths.py, which strip the read-only attribute before deleting to handle Windows/OneDrive-synced directories. It also strips the read-only bit from staged skill-reference copies during install, and wraps the CLI's per-platform skill-version stamp check in a try/except so a missing home directory no longer aborts commands like --version. The test surface adds a _restore_bundle helper and related test updates covering reference installation and version-warning behavior in environments without a resolvable home directory.
No blocking issues surfaced. 6 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1325 functions depend on the 295 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
_copy_skill_file()— 8 callers, 4 callees - worse:
uninstall_all()— 2 callers, 13 callees
Verification — 1325 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: 1064 function(s) in the blast radius were not formally verified this run
· 1 grounded finding(s) anchored inline below; 1 more finding(s) on lines outside this diff (see the check run).
| shutil.rmtree(refs_staged, ignore_errors=True) | ||
| _rmtree(refs_staged, ignore_errors=True) | ||
| raise | ||
| def _copy_skill_file(platform_name: str, *, project: bool = False, project_dir: Path | None = None) -> Path: |
There was a problem hiding this comment.
_copy_skill_file()
8 callers depend on it (afferent coupling).
Grounded coupling-delta finding (deterministic), not an LLM guess.
Two independent Windows bugs, found while running the suite on Windows 11 with the
checkout under a OneDrive-synced folder. Both are invisible to CI, which only runs
ubuntu-latest.Windows test failures go from 114 to 43. Nothing else changes:
3914 passedbefore and after on the same host, no test newly fails.
A third, unrelated commit rides along:
uv.lockstill pinnedgraphifyy 0.9.31while
pyproject.tomldeclares0.9.32, so everyuv sync/uv runrewrotethe lockfile as a side effect and left the tree dirty. Regenerated with
uv lock— a single line, no other package re-resolved. Happy to drop it if youwould rather handle the lockfile separately.
1. Read-only directories break every tree removal
shutil.rmtreedeletes the files inside a directory fine, then fails on the finalrmdir:Windows derives
FILE_ATTRIBUTE_READONLYfrom a missing owner-write bit, andRemoveDirectoryWrefuses any directory carrying it. OneDrive marks thedirectories it syncs read-only, and
copytree'scopystathands that attributeto every copy — so a
references/sidecar installed out of a OneDrive-syncedpackage can never be removed again.
graphify install(reinstall path),graphify uninstall, and ~90 install tests all died on this.The fix
clear_readonly()andrmtree()ingraphify/paths.py— already the shared homefor filesystem helpers (
_atomic_replace,write_text_atomic), and it alreadydocuments a sibling Windows
PermissionErrorworkaround. The hook signature iscompatible with both the 3.12+
onexcparameter and the olderonerrorone, soit works across the declared
requires-python = ">=3.10"range.Every removal in the package now routes through it:
graphify/install.py— all 7rmtreecall sites._install_skill_referencesadditionally strips the attribute from the staged copy after
copytree, so afresh install is writable in the first place rather than merely removable
afterwards.
graphify/cache.py—_cleanup_stale_ast_entrieswas silently skippingread-only entries in two places:
rmtree(child, ignore_errors=True)for stalev*/dirs, andchild.unlink()inside a bareexcept OSError: passfor thepre-versioning flat
*.jsonentries. Neither surfaced anything, so AST cacheentries written by older versions accumulated forever.
tests/test_install_references.py— this one is worth calling out separately,because the test suite was deleting committed files from the working tree on
every run. The
fake_bundlefixture moves the realgraphify/skills/claude/bundle aside, stages a fake in its slot, then restores on teardown:
On a read-only directory the
rmtreefails silently (ignore_errors=True), sobundle_dirstill exists — andshutil.moveonto an existing directory neststhe source inside it rather than replacing it. Net result on my checkout, twice:
The teardown is now a shared
_restore_bundle()helper used by both copies ofthis pattern. It uses the robust
rmtreeand asserts the slot is actually emptybefore moving, so if the clear ever fails again the real bundle stays safe in its
temp dir and the test fails loudly instead of destroying the checkout.
2.
graphify --versioncrashes without a home directory_run_cliresolves every platform's skill destination up front to check for astale version stamp. Resolving one needs a home directory, and in an environment
without
HOME/USERPROFILE—env -i, a bare CI container, a service account —Path.home()raises. That propagated out of a purely advisory check and took downthe whole CLI, including
graphify --version, which touches no filesystem at all.The stamp check now degrades to a no-op when the destinations cannot be resolved.
_check_skill_versionitself was already written this defensively (everyfilesystem call guarded); only the destination resolution upstream of it was not.
Covered by a regression test in
tests/test_skill_version_warning.py, verified tofail without the fix.
Testing
Host: Windows 11, Python 3.13.14, uv 0.11.19, checkout under OneDrive.
The remaining 43 failures are pre-existing and unrelated to this change — all
platform assumptions in the tests rather than product bugs. For the record, since
they are equally invisible to your CI:
OSError: [WinError 1314]\vs/path separator in assertionsassert 'sub\\b.py' == 'sub/b.py'%LOCALAPPDATA%, gemini uses~/.agentson Windows — deliberate ininstall.py, but the tests assert the POSIX pathsMAX_PATH260 limitassert (33206 & 511) == 420'mod_处理数据' != 'mod_处理数据'Happy to split this into separate PRs if you would rather review them
individually — they are already three independent commits: