From 42d067728624b9f6c2513f366723a0eff5e8647b Mon Sep 17 00:00:00 2001 From: Kaushik Samadder Date: Wed, 29 Jul 2026 07:58:26 +0530 Subject: [PATCH] fix(cache): re-anchor cached ids so a warm hit can't replay another root (#2257) Extractors mint node ids from the path STRING they are handed (_make_id(str(path)), _file_node_id(path)), so an AST cache entry written under root A embeds A's slug in every id and edge endpoint. save_cached relativized only source_file, never the ids. Because extract()'s id-remap / final-canonicalization passes key their rewrites off the CURRENT run's paths, an A-derived id matches no key on a warm hit under root B (a clone, a moved checkout, a second mount) and the stale machine slug survives into graph.json. Distinct from #2231/#2243, which fix producers on a cold run, and from #2199 (stat-index portability). Entries are now stored root-anchored and re-anchored on read, the same store-portable/re-anchor-on-load contract source_file (#777) and the stat index (#2199) already use: _relativize_ids_in replaces the root's contribution with a $graphify-root$ marker on write, _absolutize_ids_in restores what the current run's extractor would mint on read. That is the pre-remap form every downstream pass in extract() expects, so a replay reproduces a cold run exactly and no other pass changed. The anchor is derived per entry rather than assumed equal to the scan root (normalize_id distributes over path joins), so a symlinked root or relative inputs decompose exactly; only absolute root spellings may anchor, or a relative root ("src") would rewrite an already-canonical src_utils_foo into an absolute-derived id on the semantic path. The walk covers the whole payload rather than a bucket list, since the id form is self-identifying: that also reaches raw_calls[].caller_nid, swift_extensions[].nid, edges[].target_file, bash_sources[].source_file and *_type_table.path, the last three being resolution inputs that would otherwise still point at root A. save_cached's deepcopy is now unconditional; the old truthiness gate skipped it for a payload whose only content lives outside nodes/edges, which would have let the transform mutate the caller's dict and break cold-run remapping. Pre-fix entries carry no marker and their content hash never changes, so they cannot self-heal; they are swept when the release bumps the version, since AST entries live under cache/ast/v{version}/. Tests: extract a python/C/bash/markdown corpus under root A, copy the tree and graphify-out to root B, extract under B on the warm cache, and assert the run is genuinely warm (zero extractor calls), that no node id or edge endpoint carries A's slug, that the on-disk entries hold neither A's slug nor an absolute path, that a cold run still yields canonical ids, and that warm and cold match exactly. The fixture avoids JS/TS on purpose: those suffixes bypass the cache, which would make the warm assertions vacuous. --- graphify/cache.py | 214 +++++++++++++++++++++++++++++++++++++++++++- tests/test_cache.py | 191 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 404 insertions(+), 1 deletion(-) diff --git a/graphify/cache.py b/graphify/cache.py index 7257aca72..354f5213c 100644 --- a/graphify/cache.py +++ b/graphify/cache.py @@ -514,6 +514,202 @@ def _normalize_source_file_value(src: "str | Path", root_resolved: Path) -> str: return rel.replace(os.sep, "/") +# Storage marker standing in for the absolute root a cached id/path was minted +# under (#2257). Extractors mint node ids from the path STRING they are handed +# (``_make_id(str(path))``, ``_file_node_id(path)``), so a cache entry written +# under root A embeds A's slug in every id and edge endpoint. Those are only +# rewritten to the canonical root-relative form by extract()'s whole-graph +# id-remap, which keys its rewrites off the CURRENT run's paths — so on a warm +# hit under root B (a clone, a moved checkout, a second mount) the stored ids +# match no key and A's machine slug survives into graph.json. Entries are +# therefore stored root-anchored: the root's contribution is replaced by this +# marker on write and re-anchored to the current root on read, so a replay is +# portable by construction and reproduces exactly what a cold run under the +# current root would have minted (the pre-remap form every downstream pass in +# extract() expects). Same store-portable/re-anchor-on-load contract as +# ``source_file`` (#777) and the stat index (#2199). Neither ``$`` nor ``-`` can +# occur in a normalized id (``normalize_id`` drops every non-word character), and +# no plausible source literal — a shell ``$root``, a template ``${root}`` — opens +# with this exact token, so the marker cannot collide with extractor output. +_ROOT_MARKER = "$graphify-root$" + + +def _id_anchor(path_str: str, rel_str: str) -> str: + """Return the id-slug prefix ``path_str`` contributes above ``rel_str``. + + ``_make_id`` normalizes a whole path string, and normalization distributes + over path joins (every separator run collapses to one ``_``), so an id + minted from an absolute path decomposes exactly as + ``normalize_id(root) + "_" + normalize_id(rel)``. Recovering the root half + from the file's own two spellings — rather than assuming it equals the + scan root — keeps the decomposition exact when the extractor was handed an + unresolved or relative path (a symlinked root such as macOS ``/tmp`` -> + ``/private/tmp``, or inputs given relative to CWD). + + Returns "" when ``path_str`` contributes no prefix (it already IS the + relative form) or when the two spellings disagree about the tail — both + mean "nothing to re-anchor", which leaves the entry untouched. + """ + from graphify.ids import normalize_id # ids imports only re/unicodedata: no cycle + + full = normalize_id(path_str) + tail = normalize_id(rel_str) + if not tail or full == tail: + return "" + suffix = "_" + tail + return full[: -len(suffix)] if full.endswith(suffix) else "" + + +def _portability_anchors(path: "str | Path", root: "str | Path") -> tuple[list[str], str, list[str], str]: + """Root forms to strip from / restore into one cache entry (#2257). + + Returns ``(id_anchors, id_restore, path_anchors, path_restore)``. The two + ``*_anchors`` lists are the forms a stored value may have been minted from, + longest first so a shorter form can't shadow a longer one; the two + ``*_restore`` values are what the CURRENT run's extractor would mint. + + Several spellings are collected because one entry can hold ids minted from + different forms: this file's own path as passed and as resolved, plus + cross-file edge targets minted from ANOTHER in-root file's absolute path + (which share the scan root's spelling). They collapse to a single marker on + write; that is safe because extract()'s remap registers both the input-form + and absolute-resolved-form ids for every path (#1529) and maps them to the + same canonical id, so restoring either one canonicalizes identically. + """ + from graphify.ids import normalize_id + + try: + root_resolved = Path(root).resolve() + except OSError: + return [], "", [], "" + try: + path_resolved = Path(path).resolve() + except (OSError, RuntimeError): + path_resolved = Path(path) + try: + rel = os.path.relpath(path_resolved, root_resolved) + except (ValueError, OSError): + rel = "" + + # Ordered by preference for the restore form: the spelling the extractor was + # actually handed first, then its resolved form, then the scan root. + from_given = _id_anchor(str(path), rel) + from_resolved = _id_anchor(str(path_resolved), rel) + id_restore = next( + (a for a in (from_given, from_resolved, normalize_id(str(root_resolved))) if a), "" + ) + # Every strippable form must be one this same call would RESTORE, or an id + # is re-anchored under a prefix it was never minted with. That rules out a + # relative ``root`` spelling ("src"): with an absolute ``path`` the restore + # form is the resolved slug, so admitting ``src`` as an anchor would rewrite + # an already-canonical ``src_utils_foo`` into ``_utils_foo``. + # The two path-derived forms are always safe — they ARE the restore + # candidates — and cover a relative root on their own whenever the extractor + # was handed a matching relative path. + root_id_forms = (normalize_id(str(root_resolved)),) + if Path(root).is_absolute(): + root_id_forms += (normalize_id(str(root)),) + id_anchors = sorted( + {a for a in (from_given, from_resolved, *root_id_forms) if a}, + key=len, reverse=True, + ) + # Only absolute roots may anchor a PATH value: a relative one ("corpus") + # would also match a genuinely relative value that merely starts with the + # same segment, and there is no way to tell the two apart on read. + path_anchors = sorted( + {s for s in (str(root_resolved), str(root)) if Path(s).is_absolute()}, + key=len, reverse=True, + ) + return id_anchors, id_restore, path_anchors, str(root_resolved) + + +def _rewrite_strings(obj: object, fn) -> None: + """Apply ``fn`` to every string VALUE reachable in ``obj``, in place. + + Values only, never dict keys: no extractor bucket is keyed by a node id or a + path (the ``*_type_table`` maps are ``name -> type``), and rewriting keys + could silently collide two entries into one. + """ + if isinstance(obj, dict): + items: "Iterable" = obj.items() + elif isinstance(obj, list): + items = enumerate(obj) + else: + return + for key, value in list(items): + if isinstance(value, str): + new = fn(value) + if new != value: + obj[key] = new # type: ignore[index] + else: + _rewrite_strings(value, fn) + + +def _relativize_ids_in(payload: dict, path: "str | Path", root: Path) -> None: + """Replace the absolute root inside every stored id / path with the marker. + + Walks the WHOLE payload rather than a list of known buckets. The id form is + self-identifying — a long casefolded path slug that nothing but an + absolute-path-derived id can start with — so this reaches carriers a + hand-maintained bucket list misses or has yet to grow: ``nodes[].id``, + ``edges[].source``/``target``, hyperedge member lists under any of their + aliases, ``raw_calls[].caller_nid``, ``swift_extensions[].nid``, plus the + path-valued ``edges[].target_file``, ``bash_sources[].source_file`` and + ``*_type_table.path``, which resolution passes need pointing at the current + root to reproduce a cold run's edges. + + Call AFTER :func:`_relativize_source_files_in`: that stores ``source_file`` + as a bare relative path (the #777 format), and running this first would + leave it marker-prefixed instead, churning the on-disk format for nothing. + """ + id_anchors, _, path_anchors, _ = _portability_anchors(path, root) + if not id_anchors and not path_anchors: + return + + def anchor(value: str) -> str: + # Path form first: it requires a separator, which an id never contains. + for a in path_anchors: + if value == a: + return _ROOT_MARKER + for sep in ("/", "\\"): + if value.startswith(a + sep): + return _ROOT_MARKER + "/" + value[len(a) + 1:].replace("\\", "/") + for a in id_anchors: + if value.startswith(a + "_"): + return _ROOT_MARKER + "_" + value[len(a) + 1:] + return value + + _rewrite_strings(payload, anchor) + + +def _absolutize_ids_in(payload: dict, path: "str | Path", root: Path) -> None: + """Inverse of :func:`_relativize_ids_in` — re-anchor to the current root. + + The restored string is assembled by slicing, never by re-normalizing: the + stored form (``$graphify-root$_pkg_mod_base``) is a storage encoding, not a valid id, + and running it back through ``normalize_id`` would drop the marker's ``$`` + and fuse it into the slug. Entries written before #2257 carry no marker and + pass through untouched — they are swept anyway, since AST entries live under + a per-version directory (:func:`cache_dir`). + """ + _, id_restore, _, path_restore = _portability_anchors(path, root) + + def restore(value: str) -> str: + if not value.startswith(_ROOT_MARKER): + return value + rest = value[len(_ROOT_MARKER):] + if not rest: + return path_restore + if rest[0] == "/": + tail = rest[1:] + return str(Path(path_restore) / tail) if tail else path_restore + if rest[0] == "_": + return (id_restore + rest) if id_restore else rest[1:] + return value + + _rewrite_strings(payload, restore) + + def _absolutize_source_files_in(payload: dict, root: Path) -> None: """Inverse of :func:`_relativize_source_files_in`. @@ -644,6 +840,12 @@ def load_cached(path: Path, root: Path = Path("."), kind: str = "ast", # (#777). Legacy entries with absolute source_file pass through. if isinstance(result, dict): _absolutize_source_files_in(result, root) + # Same contract for the ids and remaining paths the entry embeds + # (#2257): without this a warm hit under a different absolute root + # replays ids minted from the ORIGINAL root, which extract()'s + # id-remap cannot fix because they match none of its current-path + # keys. Order is free — source_file never carries the marker. + _absolutize_ids_in(result, path, root) return result return None @@ -684,11 +886,21 @@ def save_cached(path: Path, result: dict, root: Path = Path("."), kind: str = "a # looks up Path(source_file).resolve() in a prefix table) depend on the # source_file field's original absolute form. Mutating the input here would # silently break those remaps on the first extraction pass. + # + # The copy is unconditional (it used to be gated on a non-empty + # nodes/edges/hyperedges/raw_calls bucket): a truthiness gate skips the copy + # for a result whose only payload lives in another bucket — an empty + # ``nodes`` beside a populated ``bash_sources`` — and the id/path anchoring + # below would then mutate the caller's dict for real. on_disk = result - if isinstance(result, dict) and any(result.get(k) for k in ("nodes", "edges", "hyperedges", "raw_calls")): + if isinstance(result, dict): import copy as _copy on_disk = _copy.deepcopy(result) _relativize_source_files_in(on_disk, root) + # Then replace the absolute root inside the ids and remaining paths, so + # the entry replays portably under any root (#2257). Strictly after the + # source_file pass, which owns that field's bare-relative format. + _relativize_ids_in(on_disk, p, root) h = file_hash(p, root, cache_root=cache_root) location = cache_root if cache_root is not None else root target_dir = cache_dir(location, kind, _resolve_prompt_fp(prompt, prompt_file)) diff --git a/tests/test_cache.py b/tests/test_cache.py index 993c5a0d2..fcba75fb4 100644 --- a/tests/test_cache.py +++ b/tests/test_cache.py @@ -299,6 +299,197 @@ def test_cache_portable_across_roots(tmp_path): assert not str(repo_a) in loaded["nodes"][0]["source_file"] +# --- AST cache id portability (#2257) --------------------------------------- +# Sibling of the source_file portability above. Extractors mint node ids from +# the path STRING they were handed, so a cached entry embeds the absolute scan +# root in every id (`_pkg_mod_base`). extract()'s id-remap keys those +# rewrites off the CURRENT path, so on a warm hit under a different root the +# stored ids match no key and the original root's slug survives into graph.json. + +def _reset_stat_index(): + """The stat-index location/anchor are chosen once per process via module + globals (#1747/#2199). Reset them so each test sees a fresh-process + decision — same pattern as tests/test_stat_index_portability.py.""" + from graphify import cache as _cache + + _cache._stat_index_root = None + _cache._stat_index_anchor = None + _cache._stat_index = {} + _cache._stat_index_dirty = False + + +def _portability_corpus(base: Path) -> Path: + """A corpus covering every id/path carrier a cache entry can hold. + + Deliberately NOT JavaScript/TypeScript: those suffixes are in + ``_JS_CACHE_BYPASS_SUFFIXES`` and are never cached, so a JS fixture would + make the warm-hit assertions below pass vacuously. + + - python -> cross-file `imports_from` with an already-canonical target + - C -> `edges[].target_file` (absolute) on the `#include` edge + - bash -> `bash_sources[].source_file` plus the `__entry` id suffix + - markdown-> `references` edge with a `target_file` stamp + """ + c = base / "corpus" + (c / "pkg").mkdir(parents=True) + (c / "lib").mkdir(parents=True) + (c / "pkg" / "mod.py").write_text("class Base:\n def hello(self):\n return 1\n") + (c / "app.py").write_text( + "from pkg.mod import Base\n\n\ndef run():\n return Base().hello()\n" + ) + (c / "lib" / "util.h").write_text("int util_add(int a, int b);\n") + (c / "main.c").write_text('#include "lib/util.h"\nint main(void) { return util_add(1,2); }\n') + (c / "lib" / "common.sh").write_text("greet() { echo hi; }\n") + (c / "run.sh").write_text("#!/bin/bash\nsource ./lib/common.sh\ngreet\n") + (c / "doc.md").write_text("# Doc\n\nSee [util](lib/util.h).\n") + return c + + +def _graph_ids(result: dict) -> tuple[list[str], list[tuple]]: + """Node ids + edge endpoint pairs — the granularity #2257 is about. + + Deliberately not whole-dict equality: transient hints such as + ``target_file`` are minted from the resolved path while other fields keep + the given spelling, so they can differ harmlessly between a warm and a cold + run under a symlinked root (consumers ``.resolve()`` them anyway). + """ + return ( + sorted(str(n.get("id")) for n in result["nodes"]), + sorted((str(e.get("source")), str(e.get("target"))) for e in result["edges"]), + ) + + +def test_warm_cache_from_another_root_does_not_leak_that_root(tmp_path, monkeypatch): + """#2257: extract corpus under root A (populating the cache), copy the tree + AND graphify-out to root B, extract under B on the warm cache. + + No node id or edge endpoint may carry root A's slug, and the ids must match + a cold B extraction exactly. + """ + import shutil + + import graphify.extract as ex + + a_slug = "aaa_root_marker" + b_slug = "bbb_root_marker" + corpus_a = _portability_corpus(tmp_path / a_slug) + paths_a = sorted(p for p in corpus_a.rglob("*") if p.is_file()) + + _reset_stat_index() + result_a = ex.extract(paths_a, cache_root=corpus_a, root=corpus_a, parallel=False) + from graphify import cache as _cache + _cache._flush_stat_index() + assert result_a["nodes"], "run A should have extracted something" + + # The entries on disk must be portable BY CONSTRUCTION: neither the scan + # root's slug (ids are casefolded, paths are not — compare case-insensitively) + # nor any absolute path from root A may be embedded in them. + entries = sorted((corpus_a / "graphify-out" / "cache" / "ast").rglob("*.json")) + assert entries, "run A should have written AST cache entries" + for entry in entries: + blob = entry.read_text(encoding="utf-8") + assert a_slug not in blob.lower(), ( + f"{entry.name} embeds the scan root's slug, so replaying it under a " + f"different root replays root A's ids (#2257)" + ) + assert str(corpus_a) not in blob, f"{entry.name} embeds an absolute scan path" + + # Move the corpus; graphify-out/ (cache + stat index) rides along. copy2 + # preserves mtime_ns so the stat-index fastpath stays warm. + corpus_b = tmp_path / b_slug / "corpus" + corpus_b.parent.mkdir() + shutil.copytree(corpus_a, corpus_b, copy_function=shutil.copy2) + paths_b = sorted(p for p in corpus_b.rglob("*") if p.is_file() + and "graphify-out" not in p.parts) + + # Warmth probe: _safe_extract_with_xaml_root runs only on a cache MISS. If + # run B silently re-extracts, cold ids come out clean and every assertion + # below passes while proving nothing. The probe requires parallel=False — + # the process pool extracts in a subprocess where this patch is invisible, + # so switching this call to parallel=True would make `misses` vacuously []. + misses = [] + real_extract = ex._safe_extract_with_xaml_root + + def _counting(extractor, path, root): + misses.append(str(path)) + return real_extract(extractor, path, root) + + monkeypatch.setattr(ex, "_safe_extract_with_xaml_root", _counting) + + _reset_stat_index() + warm_b = ex.extract(paths_b, cache_root=corpus_b, root=corpus_b, parallel=False) + assert misses == [], f"run B must be served entirely from the cache, re-extracted: {misses}" + + warm_ids, warm_edges = _graph_ids(warm_b) + leaked = [i for i in warm_ids if a_slug in i] + [ + p for p in warm_edges if any(a_slug in x for x in p) + ] + assert not leaked, f"root A's slug survived a warm cache hit into run B (#2257): {leaked}" + assert not [i for i in warm_ids if "$" in i], "the storage placeholder escaped into the graph" + + # ...and the replay is not merely clean but IDENTICAL to a cold B run. + monkeypatch.undo() + shutil.rmtree(corpus_b / "graphify-out") + _reset_stat_index() + cold_b = ex.extract(paths_b, cache_root=corpus_b, root=corpus_b, parallel=False) + cold_ids, cold_edges = _graph_ids(cold_b) + + # Guards the save-side transform against mutating the caller's dict: a cold + # run's ids must still be the canonical root-relative spec form, since + # extract()'s id-remap is keyed on the ABSOLUTE form the extractor minted. + assert {"app", "app_run", "pkg_mod", "pkg_mod_base", "pkg_mod_base_hello"} <= set(cold_ids), ( + f"cold run no longer produces canonical ids: {cold_ids}" + ) + assert (warm_ids, warm_edges) == (cold_ids, cold_edges), ( + "a warm cross-root cache hit must reproduce the cold extraction exactly" + ) + + +def test_cached_ids_round_trip_under_the_same_root(tmp_path): + """The stored placeholder form must restore to the exact absolute-derived id + the extractor minted, or a same-root warm hit would break extract()'s + id-remap (which is keyed on that absolute form).""" + root = tmp_path / "repo" + (root / "src").mkdir(parents=True) + f = root / "src" / "foo.py" + f.write_text("def x(): pass\n") + + from graphify.extract import _make_id + + minted = _make_id(str(f)) + result = { + "nodes": [{"id": minted, "source_file": str(f)}], + "edges": [{"source": minted, "target": minted + "_x", "source_file": str(f)}], + "raw_calls": [{"caller_nid": minted + "_x", "source_file": str(f)}], + } + save_cached(f, result, root=root, kind="ast") + + assert result["nodes"][0]["id"] == minted, "the caller's dict must not be mutated" + + loaded = load_cached(f, root=root, kind="ast") + assert loaded["nodes"][0]["id"] == minted + assert loaded["edges"][0]["source"] == minted + assert loaded["edges"][0]["target"] == minted + "_x" + assert loaded["raw_calls"][0]["caller_nid"] == minted + "_x" + + +def test_relative_root_does_not_reanchor_an_already_canonical_id(tmp_path, monkeypatch): + """A relative ``root`` (what save_semantic_cache forwards) must not be used + as an id anchor: with an absolute path the restore form is the RESOLVED + slug, so stripping the relative spelling would rewrite an already-canonical + id into an absolute-derived one — the very leak this guards against.""" + monkeypatch.chdir(tmp_path) + (tmp_path / "src" / "utils").mkdir(parents=True) + f = (tmp_path / "src" / "utils" / "foo.py").resolve() + f.write_text("x = 1\n") + + canonical = {"nodes": [{"id": "src_utils_foo", "source_file": str(f)}], "edges": []} + save_cached(f, canonical, root=Path("src"), kind="semantic") + + loaded = load_cached(f, root=Path("src"), kind="semantic") + assert loaded["nodes"][0]["id"] == "src_utils_foo" + + # --- AST cache versioning ---------------------------------------------------- # AST cache entries are the output of graphify's own extractor code, so they # are only valid for the graphify version that wrote them. Keying purely on