fix(extract): write AST cache to CWD, decoupled from the key anchor (#1774)
With no explicit cache_root, extract() wrote graphify-out/cache/ under the inferred common parent of the inputs — the analyzed source tree — so scanning a read-only/foreign corpus silently polluted it. The naive fix (point the root at CWD) breaks two other things that shared the same parameter: file_hash keys become absolute/non-portable for an out-of-CWD corpus, and the XAML/C# project-scan boundary would scan CWD instead of the corpus. So split cache LOCATION from key/id ANCHOR: load_cached/save_cached gain a cache_root arg for where the dir lives, while `root` (inferred common parent) still anchors file_hash keys, source_file relativization, node ids, and the XAML boundary. extract() now locates the cache at CWD (or cache_root) but anchors on `root`; the parallel worker tuple carries both. Existing callers passing cache_root (CLI, watcher) are unchanged. Adopts @SimiSips's #1802 (the CWD default + the two location tests) and adds the decoupling plus a regression test that keys stay relative for a corpus outside CWD — the property the one-line version would have lost. Co-Authored-By: SimiSips <SimiSips@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
SimiSips
Claude Opus 4.8
parent
6ca8604142
commit
c3a42a35eb
@@ -8,6 +8,8 @@ Full release notes with details on each version: [GitHub Releases](https://githu
|
||||
|
||||
- Fix: the CLI no longer crashes with exit code 255 when a downstream reader closes the pipe early (#1807 / #1811, thanks @varuntej07). Truncating output with `head`, PowerShell's `Select-Object -First N`, or `sed q` disconnected the reader mid-write, graphify hit an unhandled `BrokenPipeError` (or `OSError(EINVAL)` on Windows) and exited 255 — so CI wrappers and agent harnesses that both trim output and check the exit code read a successful query as a command failure. An early-closing reader is now treated as success: stdout is flushed inside the guard (piped stdout is block-buffered, so a small output would otherwise only flush at interpreter shutdown, where the error escapes as a noisy "Exception ignored" and a nonzero exit), then redirected to devnull so the shutdown flush can't raise again, and the process exits 0.
|
||||
|
||||
- Fix: `extract()` no longer writes its AST cache into the analyzed source tree (#1774 / #1802, thanks @SimiSips). With no explicit `cache_root`, the cache defaulted to the inferred common parent of the input files — the source tree — so analyzing a read-only or foreign corpus silently created `graphify-out/cache/` inside it. The cache is an output, so it now defaults to the current working directory. Crucially, the cache *location* is decoupled from the key/id *anchor*: the inferred common parent still anchors the content-hash keys, node ids, symbol resolution, and the XAML/C# project-scan boundary, so keys stay relative and portable (shared/CI cache reuse keeps working) and out-of-CWD corpora aren't mis-scanned. An explicit `cache_root` (as the CLI and watcher pass) is unchanged.
|
||||
|
||||
- Fix: `graphify query` no longer floods results with homonymous generic symbols (#1766 / #1832, thanks @devcool20). When dozens of nodes share one generic label — framework route handlers all labelled `GET`/`POST`, a repeated `handler` — they used to consume every BFS seed slot, so the traversal explored many near-identical neighborhoods and drowned out the query's actual target. Seed selection now deduplicates by normalized label (`GET`/`Get`/`get` collapse together), keeping at most one representative per label while still guaranteeing a seed for each distinct query term. The per-label cap is scoping-only — the shared node scorer (also used by `shortest_path`/`explain` endpoint resolution) is left untouched, so those are unaffected.
|
||||
|
||||
- Fix: semantic cache writes are now scoped to the files actually dispatched in each extraction batch (#1757 / #1835, thanks @TPAteeq). A model can attribute a node's `source_file` to another corpus file; `save_semantic_cache` would then replace (or, mid-run, pollute) that other file's complete cache entry with a stray fragment — silently, with no shrink guard. Writes now honor an `allowed_source_files` allowlist: the final CLI write is scoped to the uncached file set, and the per-chunk incremental checkpoint (`extract_corpus_parallel`, the default path for `extract`/`update`) is scoped to that chunk's own files, so an out-of-scope attribution is skipped with a warning instead of clobbering a legitimate entry.
|
||||
|
||||
+19
-4
@@ -354,24 +354,33 @@ def cache_dir(root: Path = Path("."), kind: str = "ast") -> Path:
|
||||
return d
|
||||
|
||||
|
||||
def load_cached(path: Path, root: Path = Path("."), kind: str = "ast") -> dict | None:
|
||||
def load_cached(path: Path, root: Path = Path("."), kind: str = "ast",
|
||||
cache_root: Path | None = None) -> dict | None:
|
||||
"""Return cached extraction for this file if hash matches, else None.
|
||||
|
||||
Cache key: SHA256 of file contents.
|
||||
Cache value: stored as graphify-out/cache/{kind}/{hash}.json (AST entries
|
||||
under the per-version subdirectory, see :func:`cache_dir`).
|
||||
|
||||
``root`` anchors the content-hash key and source_file relativization (it
|
||||
must stay the inferred common parent so keys remain portable). ``cache_root``
|
||||
decouples *where* the cache directory lives from that anchor — the cache is
|
||||
an output and must not land inside a read-only/analyzed source tree (#1774).
|
||||
When ``cache_root`` is None the location falls back to ``root`` (unchanged
|
||||
behavior for existing callers).
|
||||
|
||||
AST entries written by other graphify versions — including the legacy
|
||||
flat cache/ layout (pre-0.5.3) and the unversioned cache/ast/ layout —
|
||||
are deliberately not consulted: they were produced by a different
|
||||
extractor and may be stale.
|
||||
Returns None if no cache entry or file has changed.
|
||||
"""
|
||||
location = cache_root if cache_root is not None else root
|
||||
try:
|
||||
h = file_hash(path, root)
|
||||
except OSError:
|
||||
return None
|
||||
entry = cache_dir(root, kind) / f"{h}.json"
|
||||
entry = cache_dir(location, kind) / f"{h}.json"
|
||||
if entry.exists():
|
||||
try:
|
||||
result = json.loads(entry.read_text(encoding="utf-8"))
|
||||
@@ -386,12 +395,17 @@ def load_cached(path: Path, root: Path = Path("."), kind: str = "ast") -> dict |
|
||||
return None
|
||||
|
||||
|
||||
def save_cached(path: Path, result: dict, root: Path = Path("."), kind: str = "ast") -> None:
|
||||
def save_cached(path: Path, result: dict, root: Path = Path("."), kind: str = "ast",
|
||||
cache_root: Path | None = None) -> None:
|
||||
"""Save extraction result for this file.
|
||||
|
||||
Stores as graphify-out/cache/{kind}/{hash}.json where hash = SHA256 of current file contents.
|
||||
result should be a dict with 'nodes' and 'edges' lists.
|
||||
|
||||
``root`` anchors the content-hash key and source_file relativization;
|
||||
``cache_root`` (when given) is where the cache directory is written, decoupled
|
||||
from ``root`` so the cache never lands inside the analyzed source tree (#1774).
|
||||
|
||||
No-ops if `path` is not a regular file. Subagent-produced semantic fragments
|
||||
occasionally carry a directory path in `source_file`; skipping them prevents
|
||||
IsADirectoryError from aborting the whole batch.
|
||||
@@ -415,7 +429,8 @@ def save_cached(path: Path, result: dict, root: Path = Path("."), kind: str = "a
|
||||
on_disk = _copy.deepcopy(result)
|
||||
_relativize_source_files_in(on_disk, root)
|
||||
h = file_hash(p, root)
|
||||
target_dir = cache_dir(root, kind)
|
||||
location = cache_root if cache_root is not None else root
|
||||
target_dir = cache_dir(location, kind)
|
||||
entry = target_dir / f"{h}.json"
|
||||
fd, tmp_path = tempfile.mkstemp(dir=target_dir, prefix=f"{h}.", suffix=".tmp")
|
||||
try:
|
||||
|
||||
+36
-16
@@ -4060,20 +4060,28 @@ def _extract_single_file(args: tuple) -> tuple[int, dict]:
|
||||
ProcessPoolExecutor.
|
||||
|
||||
Args:
|
||||
args: (index, path_str, cache_root_str) tuple
|
||||
args: (index, path_str, root_str, cache_location_str) tuple. ``root``
|
||||
anchors hash keys / node ids / the XAML boundary; ``cache_location``
|
||||
is where the cache dir is written, decoupled per #1774. A legacy
|
||||
3-tuple (no cache_location) is still accepted for back-compat.
|
||||
|
||||
Returns:
|
||||
(index, result_dict) so results can be placed back in order.
|
||||
"""
|
||||
idx, path_str, cache_root_str = args
|
||||
if len(args) == 4:
|
||||
idx, path_str, root_str, cache_location_str = args
|
||||
else: # legacy 3-tuple: location == anchor
|
||||
idx, path_str, root_str = args
|
||||
cache_location_str = root_str
|
||||
path = Path(path_str)
|
||||
cache_root = Path(cache_root_str)
|
||||
root = Path(root_str)
|
||||
cache_location = Path(cache_location_str)
|
||||
_raise_recursion_limit()
|
||||
bypass_cache = path.suffix in _JS_CACHE_BYPASS_SUFFIXES
|
||||
|
||||
# Check cache first (avoid re-extraction)
|
||||
if not bypass_cache:
|
||||
cached = load_cached(path, cache_root)
|
||||
cached = load_cached(path, root, cache_root=cache_location)
|
||||
if cached is not None:
|
||||
return idx, cached
|
||||
|
||||
@@ -4081,23 +4089,24 @@ def _extract_single_file(args: tuple) -> tuple[int, dict]:
|
||||
if extractor is None:
|
||||
return idx, {"nodes": [], "edges": []}
|
||||
|
||||
result = _safe_extract_with_xaml_root(extractor, path, cache_root)
|
||||
result = _safe_extract_with_xaml_root(extractor, path, root)
|
||||
# Never cache a zero-node result for an extractable file. Every supported
|
||||
# source produces at least a file node, so an empty node list is anomalous
|
||||
# (e.g. a transient batch/parallel hiccup). Caching it makes the empty
|
||||
# byte-stable across runs and silently blinds affected/explain to and
|
||||
# through the file (#1666); skipping the write lets a rerun self-heal.
|
||||
if not bypass_cache and "error" not in result and result.get("nodes"):
|
||||
save_cached(path, result, cache_root)
|
||||
save_cached(path, result, root, cache_root=cache_location)
|
||||
return idx, result
|
||||
|
||||
|
||||
def _extract_parallel(
|
||||
uncached_work: list[tuple[int, Path]],
|
||||
per_file: list[dict | None],
|
||||
effective_root: Path,
|
||||
root: Path,
|
||||
max_workers: int | None,
|
||||
total_files: int,
|
||||
cache_location: Path | None = None,
|
||||
) -> bool:
|
||||
"""Extract uncached files in parallel using ProcessPoolExecutor.
|
||||
|
||||
@@ -4134,8 +4143,11 @@ def _extract_parallel(
|
||||
max_workers = min(max_workers, 61)
|
||||
max_workers = max(max_workers, 1)
|
||||
|
||||
root_str = str(effective_root)
|
||||
work_items = [(idx, str(path), root_str) for idx, path in uncached_work]
|
||||
# root anchors hash keys / node ids / XAML boundary; cache_location is where
|
||||
# the cache dir is written (defaults to root when not decoupled) (#1774).
|
||||
root_str = str(root)
|
||||
cache_loc_str = str(cache_location if cache_location is not None else root)
|
||||
work_items = [(idx, str(path), root_str, cache_loc_str) for idx, path in uncached_work]
|
||||
|
||||
done_count = 0
|
||||
_PROGRESS_INTERVAL = 100
|
||||
@@ -4195,8 +4207,9 @@ def _extract_parallel(
|
||||
def _extract_sequential(
|
||||
uncached_work: list[tuple[int, Path]],
|
||||
per_file: list[dict | None],
|
||||
effective_root: Path,
|
||||
root: Path,
|
||||
total_files: int,
|
||||
cache_location: Path | None = None,
|
||||
) -> None:
|
||||
"""Extract uncached files sequentially (fallback for small batches)."""
|
||||
_PROGRESS_INTERVAL = 100
|
||||
@@ -4215,10 +4228,11 @@ def _extract_sequential(
|
||||
per_file[idx] = {"nodes": [], "edges": []}
|
||||
continue
|
||||
bypass_cache = path.suffix in _JS_CACHE_BYPASS_SUFFIXES
|
||||
result = _safe_extract_with_xaml_root(extractor, path, effective_root)
|
||||
# XAML boundary anchors on `root` (the corpus), not the cache location.
|
||||
result = _safe_extract_with_xaml_root(extractor, path, root)
|
||||
# See _extract_single_file: don't cache an anomalous zero-node result (#1666).
|
||||
if not bypass_cache and "error" not in result and result.get("nodes"):
|
||||
save_cached(path, result, effective_root)
|
||||
save_cached(path, result, root, cache_root=cache_location)
|
||||
per_file[idx] = result
|
||||
if total_files >= _PROGRESS_INTERVAL:
|
||||
# Consistent denominator with the intermediate lines (#1693).
|
||||
@@ -4281,7 +4295,13 @@ def extract(
|
||||
root = cache_root
|
||||
root = root.resolve()
|
||||
|
||||
effective_root = cache_root or root
|
||||
# #1774: the cache is an OUTPUT, so when no explicit cache_root is given it is
|
||||
# written under the current working directory — never `root` (the inferred
|
||||
# common parent of the inputs), which would drop graphify-out/ inside a
|
||||
# read-only or foreign corpus. `root` still anchors the content-hash keys,
|
||||
# node ids, symbol resolution, and the XAML project-scan boundary; only the
|
||||
# cache directory's location diverges from it.
|
||||
cache_location = (cache_root if cache_root is not None else Path(".")).resolve()
|
||||
total = len(paths)
|
||||
|
||||
# Phase 1: separate cached hits from uncached work
|
||||
@@ -4294,7 +4314,7 @@ def extract(
|
||||
continue
|
||||
bypass_cache = path.suffix in _JS_CACHE_BYPASS_SUFFIXES
|
||||
if not bypass_cache:
|
||||
cached = load_cached(path, effective_root)
|
||||
cached = load_cached(path, root, cache_root=cache_location)
|
||||
if cached is not None:
|
||||
per_file[i] = cached
|
||||
continue
|
||||
@@ -4305,10 +4325,10 @@ def extract(
|
||||
ran_parallel = False
|
||||
if parallel and len(uncached_work) >= _PARALLEL_THRESHOLD:
|
||||
ran_parallel = _extract_parallel(
|
||||
uncached_work, per_file, effective_root, max_workers, total
|
||||
uncached_work, per_file, root, max_workers, total, cache_location
|
||||
)
|
||||
if not ran_parallel:
|
||||
_extract_sequential(uncached_work, per_file, effective_root, total)
|
||||
_extract_sequential(uncached_work, per_file, root, total, cache_location)
|
||||
|
||||
# Fill any remaining None slots (shouldn't happen, but defensive)
|
||||
for i in range(total):
|
||||
|
||||
@@ -1114,7 +1114,7 @@ def test_extract_falls_back_to_sequential_when_parallel_returns_false(tmp_path,
|
||||
calls = {"parallel": 0, "sequential": 0}
|
||||
real_sequential = extract_mod._extract_sequential
|
||||
|
||||
def fake_parallel(uncached_work, per_file, effective_root, max_workers, total_files):
|
||||
def fake_parallel(uncached_work, per_file, root, max_workers, total_files, cache_location=None):
|
||||
calls["parallel"] += 1
|
||||
return False # simulate the post-fix BrokenProcessPool branch
|
||||
|
||||
|
||||
@@ -0,0 +1,105 @@
|
||||
"""#1774 — extract() must never write its AST cache into the analyzed source tree.
|
||||
|
||||
The cache is an output. When no cache_root is given it used to default to the
|
||||
inferred common parent of the input files — the source tree — so analyzing a
|
||||
read-only corpus (someone else's repo, a knowledge base) silently created
|
||||
graphify-out/cache/ inside it. It now defaults to the current working directory;
|
||||
an explicit cache_root still wins.
|
||||
|
||||
Crucially, the cache *location* is decoupled from the key/id *anchor*: the
|
||||
inferred common parent still anchors the content-hash keys, node ids and the
|
||||
XAML scan boundary, so keys stay relative and portable even when the corpus
|
||||
lives outside CWD. (An earlier one-line fix that pointed the anchor itself at
|
||||
CWD would have made keys absolute and machine-specific for out-of-CWD corpora.)
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
import graphify.extract as ex
|
||||
from graphify.cache import load_cached, file_hash
|
||||
|
||||
|
||||
def _make_corpus(base: Path) -> Path:
|
||||
corpus = base / "corpus"
|
||||
corpus.mkdir()
|
||||
(corpus / "a.py").write_text("class Base:\n def hello(self):\n return 1\n")
|
||||
(corpus / "b.py").write_text("from a import Base\n\nclass Sub(Base):\n pass\n")
|
||||
return corpus
|
||||
|
||||
|
||||
def test_default_cache_lands_in_cwd_not_source_tree(tmp_path, monkeypatch):
|
||||
corpus = _make_corpus(tmp_path)
|
||||
work = tmp_path / "work"
|
||||
work.mkdir()
|
||||
monkeypatch.chdir(work)
|
||||
|
||||
result = ex.extract([corpus / "a.py", corpus / "b.py"], parallel=False)
|
||||
|
||||
assert result["nodes"], "extraction should still produce nodes"
|
||||
assert not (corpus / "graphify-out").exists(), (
|
||||
"cache written into the analyzed source tree (#1774)"
|
||||
)
|
||||
assert (work / "graphify-out" / "cache").is_dir(), "cache should land under CWD"
|
||||
|
||||
|
||||
def test_explicit_cache_root_still_wins(tmp_path, monkeypatch):
|
||||
corpus = _make_corpus(tmp_path)
|
||||
work = tmp_path / "work"
|
||||
work.mkdir()
|
||||
out = tmp_path / "out"
|
||||
monkeypatch.chdir(work)
|
||||
|
||||
ex.extract([corpus / "a.py"], cache_root=out, parallel=False)
|
||||
|
||||
assert (out / "graphify-out" / "cache").is_dir()
|
||||
assert not (corpus / "graphify-out").exists()
|
||||
assert not (work / "graphify-out").exists()
|
||||
|
||||
|
||||
def test_default_cache_round_trips_via_extract(tmp_path, monkeypatch):
|
||||
"""A second extract() of the same corpus must hit the CWD cache the first
|
||||
wrote — the real contract (both runs anchor keys on the inferred corpus root
|
||||
and locate the cache at CWD)."""
|
||||
corpus = _make_corpus(tmp_path)
|
||||
work = tmp_path / "work"
|
||||
work.mkdir()
|
||||
monkeypatch.chdir(work)
|
||||
|
||||
ex.extract([corpus / "a.py"], parallel=False)
|
||||
# Look up with the same anchor extract() uses (the corpus dir) and the CWD
|
||||
# cache location — this must hit.
|
||||
hit = load_cached(corpus / "a.py", corpus.resolve(), cache_root=Path(".").resolve())
|
||||
assert hit is not None, "second run should hit the CWD cache written by the first"
|
||||
|
||||
|
||||
def test_cache_keys_stay_relative_for_out_of_cwd_corpus(tmp_path, monkeypatch):
|
||||
"""The location/anchor split must keep content-hash keys anchored on the
|
||||
corpus (relative, portable) even when the corpus is outside CWD — not
|
||||
re-anchored to CWD, which would bake a machine-specific absolute path into
|
||||
the key and break shared/CI cache reuse (#1774 regression guard)."""
|
||||
import hashlib
|
||||
|
||||
corpus = _make_corpus(tmp_path)
|
||||
work = tmp_path / "elsewhere" / "work"
|
||||
work.mkdir(parents=True)
|
||||
monkeypatch.chdir(work)
|
||||
|
||||
ex.extract([corpus / "a.py"], parallel=False)
|
||||
|
||||
root = corpus.resolve()
|
||||
key = file_hash(corpus / "a.py", root)
|
||||
raw = (corpus / "a.py").read_bytes()
|
||||
|
||||
def _key_with(anchor_rel: str) -> str:
|
||||
h = hashlib.sha256()
|
||||
h.update(raw)
|
||||
h.update(b"\x00")
|
||||
h.update(anchor_rel.encode())
|
||||
return h.hexdigest()
|
||||
|
||||
# Portable: keyed on the relative path within the corpus...
|
||||
assert key == _key_with("a.py")
|
||||
# ...not on the absolute path (which the CWD-anchor one-liner would produce).
|
||||
abs_rel = str((corpus / "a.py").resolve()).lower()
|
||||
assert key != _key_with(abs_rel)
|
||||
Reference in New Issue
Block a user