diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f5679b..5bc971b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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. diff --git a/graphify/cache.py b/graphify/cache.py index 3970540..b35851b 100644 --- a/graphify/cache.py +++ b/graphify/cache.py @@ -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: diff --git a/graphify/extract.py b/graphify/extract.py index 59d4f82..a4ac6cb 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -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): diff --git a/tests/test_extract.py b/tests/test_extract.py index 519c9cc..dfb28d6 100644 --- a/tests/test_extract.py +++ b/tests/test_extract.py @@ -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 diff --git a/tests/test_extract_cache_location.py b/tests/test_extract_cache_location.py new file mode 100644 index 0000000..c8e04bc --- /dev/null +++ b/tests/test_extract_cache_location.py @@ -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)