Repository navigation
fix(cache): read and write AST cache entries via extended-length paths on Windows - #4542
omsenjalia wants to merge 1 commit into
Conversation
…s on Windows Under a deep root, the cache entry <root>/graphify-out/cache/ast/v<ver>-s<schema>-g<grammars>/<64 hex>.json, and mkstemp's even longer <hash>.<random>.tmp temp name, overrun Windows MAX_PATH. save_cached raised FileNotFoundError and `graphify extract` failed with "AST extraction failed" (e.g. any project under the default %TEMP% of a long user name). Move detect._os_path (the \?\ helper from Graphify-Labs#1655) to paths.os_long_path so cache.py can share it, and use it for the entry and temp file in save_cached and load_cached. detect._os_path stays as an alias. Keys are unaffected: _normalize_path still strips the prefix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the pull request, @omsenjalia. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
There was a problem hiding this comment.
Graphify reviewed this change.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; formal verification proved 0 of 2 changed function(s) (2 not verified).
Formal verification. PR-changed functions: 0/2 verified (0 proven, 0 may-equivalent, 0 distinguished) · 2 not verified (2 unsupported).
Not verified on this run: load\_cached (unsupported), save\_cached (unsupported).
Graphify review — findings
Fixes cache saves and loads failing on Windows when the cache root is deep enough to push entry paths (and mkstemp's longer .tmp name) past MAX_PATH, which made graphify extract crash with FileNotFoundError. save_cached and load_cached now build entry and temp paths in the extended-length \\?\ form; non-Windows paths pass through unchanged. The long-path helper moves out of detect.py into paths.py as os_long_path so the cache and detect share one implementation, and detect.py keeps _os_path as an alias for its existing callers. Adds a Windows-only round-trip test that pads the cache root so the entry lands past 260 chars while the directory stays creatable, and checks no stray .tmp files remain.
No blocking issues surfaced. 3 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 4678 functions depend on the 279 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
load_cached()— 62 callers, 10 callees - worse:
save_cached()— 49 callers, 10 callees - worse:
cache_dir()— 22 callers, 3 callees
Verification — 4678 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: 2724 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
205 of 433 test file(s) selected (47%) via static blast radius.
tests/test_affected_cli.py— impacttests/test_agents_platform.py— impacttests/test_alias_folding.py— impacttests/test_analyze.py— impacttests/test_astro_extraction.py— impacttests/test_atomic_canvas_export.py— impacttests/test_atomic_version_stamp.py— impacttests/test_atomic_writes.py— impacttests/test_backend_opt_in.py— impacttests/test_benchmark.py— impacttests/test_benchmark_raw_graph.py— impacttests/test_benchmark_real_query.py— impacttests/test_build.py— impacttests/test_build_located_semantic_identity.py— impacttests/test_build_merge_dedup_scope.py— impacttests/test_build_merge_hyperedges_and_prune.py— impacttests/test_build_merge_shrink_guard.py— impacttests/test_c_include_resolution.py— impacttests/test_cache.py— impacttests/test_cache_long_paths.py— impact, changed-testtests/test_cache_stale_import_target.py— impacttests/test_callflow_html.py— impacttests/test_cargo_introspect.py— impacttests/test_carried_hyperedge_remap.py— impacttests/test_charmap_encoding.py— impacttests/test_chunking.py— impacttests/test_cjs_module_extension.py— impacttests/test_claude_cli_backend.py— impacttests/test_cli_export.py— impacttests/test_cluster.py— impacttests/test_codebuddy.py— impacttests/test_community_labels_skill.py— impacttests/test_compact_graph_json.py— impacttests/test_confidence.py— impacttests/test_corrupt_graph_json.py— impacttests/test_cpp_objc_cross_file_calls.py— impacttests/test_cross_extension_reexport_self_cycle.py— impacttests/test_csharp_object_creation.py— impacttests/test_csharp_partial_classes.py— impacttests/test_dedup.py— impacttests/test_dedup_remaps_hyperedges.py— impacttests/test_dedup_shrink_refuses_force_write.py— impacttests/test_definition_file_portability.py— impacttests/test_detect.py— impacttests/test_devin.py— impacttests/test_docx_to_markdown.py— impacttests/test_dotnet.py— impacttests/test_duplicate_annotation_edges.py— impacttests/test_ejs_extractor.py— impacttests/test_evidence_binding.py— impact- … and 155 more
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify load\_cached.
The verifier did not have enough to check load\_cached, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: parameter `prompt` is annotated `'str | Path | None'` — outside the synthesizable primitive/collection set
Could not verify: Could not verify save\_cached.
The verifier did not have enough to check save\_cached, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: parameter `prompt` is annotated `'str | Path | None'` — outside the synthesizable primitive/collection set
· 3 more finding(s) on lines outside this diff (see the check run).
|
On the "Health: worse" lines: I diffed the AST graph at the merge-base (
I'm keeping it this way. The two alternatives are worse:
|
Problem
On Windows,
graphify extractfails outright when the project sits under a deep path:The AST cache path
<root>\graphify-out\cache\ast\v<ver>-s<schema>-g<grammars>\<64 hex>.jsonadds about 120 characters to the root.save_cached'smkstemptemp name (<hash>.<random>.tmp) adds 8 more. When the root is about 140 characters or longer, the path passes the 260-character MAX_PATH limit and the write raises. A long user name under the default%TEMP%is enough to trigger it. In my repro the temp path was 268 characters; the same project underDesktop\worked.Change
detect._os_path, the\?\extended-length helper from [Windows] Long paths (>260 chars) never hashed by save_manifest/_md5_file — files re-flagged as changed on every update #1655, topaths.os_long_pathsocache.pycan share it.detect._os_pathstays as an alias, so its callers and the tests intest_long_path_hashing.pyare unchanged.save_cachedcreates the temp file and writes the entry through the extended-length path.load_cachedreads the entry, and the legacy fallback entry, the same way._normalize_pathstill strips the prefix.os_long_pathreturns the path unchanged.Tests
tests/test_cache_long_paths.py(Windows only, like the MAX_PATH test intest_atomic_writes.py). It saves and loads an AST entry whose path is past MAX_PATH while its folder can still be created, and checks that no.tmpfile is left behind. It fails without the fix (FileNotFoundErrorintempfile.mkstemp) and passes with it.graphify extractin the deep folder now writesgraph.json, and a second run reports1 files cached/unchanged.ruffis clean.pyrightreports the same 14 existing errors indetect.pyas untouched v8.test_ext_tagged_id_clash,test_skillgen_input_path_injection, the memoization getcwd tests, and others). No new failures.Not covered
Only
load_cached/save_cacheduse the long-path form. Other cache writers (the stat index,cached_files's glob) write into the cache folder rather than to a 64-hex file name, so they stay under the limit in this case.AI disclosure (per CONTRIBUTING.md): Claude Opus 5.5 (Claude Code) materially contributed to this change.
🤖 Generated with Claude Code