From db410d0d09ff804f3b1d955f0436a45de9b6831a Mon Sep 17 00:00:00 2001 From: safishamsi Date: Mon, 20 Jul 2026 15:28:59 +0100 Subject: [PATCH] fix(detect): stop silent data loss from env-dir pruning and absolute-path sidecars (#2058, #2059) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2058: `_is_noise_dir` treated any directory named `env`/`.env`/`*_env` as a Python virtualenv and pruned it during the walk — before `.graphifyignore` negation, with zero trace in any returned bucket. Real source dirs with those names (common in UVM/ASIC verification trees) were silently lost. The venv heuristic for those names is now gated on actual markers (`pyvenv.cfg`, `bin`/`Scripts/activate`, `lib/python*`, `conda-meta/`); `venv`/`.venv`/`*_venv` stay name-only. Pruned-as-noise dirs are recorded in a new `pruned_noise_dirs` bucket for traceability, and extract.py's walk call sites pass the parent so genuine venvs are still marker-checked and pruned. #2059: Office and Google-Workspace sidecars were named with a hash of the resolved ABSOLUTE source path, so the same tracked file in two clones/worktrees produced two differently-named byte-identical sidecars — unbounded duplicates when graphify-out/ is committed, each ingested as a distinct source doc. The hash is now over the scan-root-relative (NFC-normalized) path, stable across checkouts while still disambiguating same-stem files; out-of-root sources fall back to the old absolute form. Also fixes the same bug in google_workspace's `_sidecar_path` (which additionally never had the #1226 NFC fix). --- graphify/detect.py | 76 ++++++++++++++++++++---- graphify/extract.py | 4 +- graphify/google_workspace.py | 20 ++++++- tests/test_detect.py | 108 ++++++++++++++++++++++++++++++++++- 4 files changed, 190 insertions(+), 18 deletions(-) diff --git a/graphify/detect.py b/graphify/detect.py index c2638a0..75c2c29 100644 --- a/graphify/detect.py +++ b/graphify/detect.py @@ -652,7 +652,7 @@ def xlsx_extract_structure(path: Path) -> dict: return {"nodes": nodes, "edges": edges} -def convert_office_file(path: Path, out_dir: Path) -> Path | None: +def convert_office_file(path: Path, out_dir: Path, root: "Path | None" = None) -> Path | None: """Convert a .docx or .xlsx to a markdown sidecar in out_dir. Returns the path of the converted .md file, or None if conversion failed @@ -671,14 +671,30 @@ def convert_office_file(path: Path, out_dir: Path) -> Path | None: out_dir.mkdir(parents=True, exist_ok=True) # Use a stable name derived from the original path to avoid collisions. - # Normalize the resolved path to NFC before hashing: on macOS (HFS+/APFS) - # os.walk/rglob return filenames in NFD, while Python string literals and - # directly-constructed Path objects are NFC, so the same source file would - # otherwise hash to different sidecar names across runs — causing --update - # to treat every Office file as new and re-extract it (#1226). + # Hash the path RELATIVE to the scan root, not the absolute path: the + # absolute form salts the name with the checkout location, so the same + # tracked .xlsx in two clones/worktrees emits two differently-named, + # byte-identical sidecars — unbounded duplicates when graphify-out/ is + # committed, each ingested as a distinct source doc (#2059). The relative + # path still disambiguates same-stem files in different directories. + # Normalize to NFC before hashing: on macOS (HFS+/APFS) os.walk/rglob return + # filenames in NFD, while Python string literals and directly-constructed + # Path objects are NFC, so the same source file would otherwise hash to + # different sidecar names across runs — making --update treat every Office + # file as new and re-extract it (#1226). import hashlib import unicodedata - normalized_path = unicodedata.normalize("NFC", str(path.resolve())) + if root is None: + # Default layout: out_dir is //converted. + root = out_dir.parent.parent + try: + key = path.resolve().relative_to(Path(root).resolve()).as_posix() + except (ValueError, OSError): + # Not under the scan root (custom GRAPHIFY_OUT layouts, --include + # sources, direct API callers): keep the previous absolute form rather + # than guessing, so behavior is unchanged for those cases. + key = str(path.resolve()) + normalized_path = unicodedata.normalize("NFC", key) name_hash = hashlib.sha256(normalized_path.encode()).hexdigest()[:8] out_path = out_dir / f"{path.stem}_{name_hash}.md" # Skip re-writing only when the sidecar is present AND at least as new as the @@ -718,7 +734,7 @@ def count_words(path: Path) -> int: # Directory names to always skip - venvs, caches, build artifacts, deps _SKIP_DIRS = { - "venv", ".venv", "env", ".env", + "venv", ".venv", # "env"/".env"/"*_env" are gated on venv markers below (#2058) "node_modules", "__pycache__", ".git", "dist", "build", "target", "out", "site-packages", "lib64", @@ -753,10 +769,39 @@ _SKIP_FILES = { _JS_SNAPSHOT_TEST_ROOTS = frozenset({"__tests__", "__test__"}) +def _has_venv_markers(d: "Path") -> bool: + """True only when *d* has actual virtualenv/conda structure on disk. + + ``env``/``.env``/``*_env`` is a real source-directory convention (UVM/ASIC + verification trees, and others), so pruning it by name alone silently drops + legitimate source with no trace (#2058). Prune it only on real evidence: a + ``pyvenv.cfg``, an ``activate`` script, a ``lib/python*`` tree, or conda's + ``conda-meta/`` (``conda create -p ./env`` writes no pyvenv.cfg). + """ + try: + if (d / "pyvenv.cfg").is_file(): + return True + if (d / "bin" / "activate").is_file() or (d / "Scripts" / "activate").is_file(): + return True + if next(d.glob("lib/python*"), None) is not None: + return True + if (d / "conda-meta").is_dir(): + return True + except OSError: + pass + return False + + def _is_noise_dir(part: str, parent: "Path | None" = None) -> bool: """Return True if this directory name looks like a venv, cache, or dep dir.""" if part in _SKIP_DIRS: return True + if part in ("env", ".env") or part.endswith("_env"): + # Ambiguous: a real venv OR a real source dir. Prune only on actual venv + # evidence, mirroring the "snapshots" gating (#1666/#2058). + if parent is None: + return False # cannot verify; keep a possibly-real code dir + return _has_venv_markers(parent / part) if part == "snapshots": # Prune only when it looks like an actual JS/Vitest snapshot dir. if parent is None: @@ -770,8 +815,9 @@ def _is_noise_dir(part: str, parent: "Path | None" = None) -> bool: except OSError: pass return False - # Catch *_venv, *_repo/site-packages patterns - if part.endswith("_venv") or part.endswith("_env"): + # Catch *_venv (unambiguous — "venv" is always a virtualenv signal). "*_env" + # is gated on markers above (#2058), not pruned by name. + if part.endswith("_venv"): return True if part.endswith(".egg-info"): return True @@ -1218,6 +1264,7 @@ def detect(root: Path, *, follow_symlinks: bool | None = None, google_workspace: # of silently vanishing from the graph (#1922). Directory-level entries keep # this bounded — a pruned `data/` is one entry, not one per contained file. ignored: list[str] = [] + pruned_noise: list[str] = [] ignore_patterns = _load_graphifyignore(root, gitignore=gitignore) ignore_cache: dict[Path, bool] = {} # shared across all _is_ignored calls in this scan # CLI --exclude patterns are anchored at the scan root and appended last @@ -1292,6 +1339,10 @@ def detect(root: Path, *, follow_symlinks: bool | None = None, google_workspace: kept_dirs: list[str] = [] for d in dirnames: if _is_noise_dir(d, dp): + # Record pruned-as-noise dirs so a wrongly-pruned real + # source dir is at least traceable in the output rather + # than vanishing silently (#2058). + pruned_noise.append(str(dp / d) + os.sep) continue if _is_ignored(dp / d, root, ignore_patterns, _cache=ignore_cache): ignored.append(str(dp / d) + os.sep) @@ -1353,7 +1404,7 @@ def detect(root: Path, *, follow_symlinks: bool | None = None, google_workspace: ) continue try: - md_path = convert_google_workspace_file(p, converted_dir, xlsx_to_markdown=xlsx_to_markdown) + md_path = convert_google_workspace_file(p, converted_dir, xlsx_to_markdown=xlsx_to_markdown, root=root) except Exception as exc: skipped_sensitive.append(str(p) + f" [Google Workspace export failed: {exc}]") continue @@ -1367,7 +1418,7 @@ def detect(root: Path, *, follow_symlinks: bool | None = None, google_workspace: continue # Office files: convert to markdown sidecar so subagents can read them if p.suffix.lower() in OFFICE_EXTENSIONS: - md_path = convert_office_file(p, converted_dir) + md_path = convert_office_file(p, converted_dir, root=root) if md_path: if _is_ignored(md_path, root, ignore_patterns, _cache=ignore_cache): continue @@ -1411,6 +1462,7 @@ def detect(root: Path, *, follow_symlinks: bool | None = None, google_workspace: "unclassified": sorted(unclassified), "walk_errors": walk_errors, "ignored": sorted(ignored), + "pruned_noise_dirs": sorted(pruned_noise), "graphifyignore_patterns": len(ignore_patterns), "scan_root": str(root.resolve()), } diff --git a/graphify/extract.py b/graphify/extract.py index 49aa4fc..401f983 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -5199,7 +5199,7 @@ def collect_files(target: Path, *, follow_symlinks: bool = False, root: Path | N dp = Path(dirpath) dirnames[:] = [ d for d in dirnames - if not _is_noise_dir(d) + if not _is_noise_dir(d, dp) # pass parent so "env"/"*_env" is marker-gated (#2058) and (has_negation or not _ignored(dp / d)) ] for fname in filenames: @@ -5220,7 +5220,7 @@ def collect_files(target: Path, *, follow_symlinks: bool = False, root: Path | N dp = Path(dirpath) dirnames[:] = [ d for d in dirnames - if not _is_noise_dir(d) + if not _is_noise_dir(d, dp) # pass parent so "env"/"*_env" is marker-gated (#2058) and (not (dp / d).is_symlink() or _resolves_under_root(dp / d, containment_root)) ] for fname in filenames: diff --git a/graphify/google_workspace.py b/graphify/google_workspace.py index e9e60d8..1feeb8a 100644 --- a/graphify/google_workspace.py +++ b/graphify/google_workspace.py @@ -121,8 +121,21 @@ def _run_gws_export(file_id: str, mime_type: str, output: Path, resource_key: st raise RuntimeError(f"gws export failed for {file_id}: {stderr}") -def _sidecar_path(path: Path, out_dir: Path) -> Path: - name_hash = hashlib.sha256(str(path.resolve()).encode()).hexdigest()[:8] +def _sidecar_path(path: Path, out_dir: Path, root: "Path | None" = None) -> Path: + # Hash the scan-root-relative, NFC-normalized path — not the absolute path. + # The absolute form salts the sidecar name with the checkout location, so the + # same shortcut in two clones/worktrees emits differently-named byte-identical + # sidecars, each ingested as a distinct source doc when graphify-out/ is + # committed (#2059; mirrors convert_office_file). NFC guards macOS NFD drift + # (#1226). The relative path still disambiguates same-stem files. + import unicodedata + if root is None: + root = out_dir.parent.parent + try: + key = path.resolve().relative_to(Path(root).resolve()).as_posix() + except (ValueError, OSError): + key = str(path.resolve()) + name_hash = hashlib.sha256(unicodedata.normalize("NFC", key).encode()).hexdigest()[:8] return out_dir / f"{path.stem}_{name_hash}.md" @@ -152,6 +165,7 @@ def convert_google_workspace_file( out_dir: Path, *, xlsx_to_markdown: Callable[[Path], str] | None = None, + root: "Path | None" = None, ) -> Path | None: """Export a Google Workspace shortcut to a Markdown sidecar. @@ -164,7 +178,7 @@ def convert_google_workspace_file( shortcut = read_google_shortcut(path) out_dir.mkdir(parents=True, exist_ok=True) - out_path = _sidecar_path(path, out_dir) + out_path = _sidecar_path(path, out_dir, root=root) if ext == ".gdoc": with tempfile.NamedTemporaryFile("w+b", suffix=".md", delete=False, dir=out_dir) as tmp: diff --git a/tests/test_detect.py b/tests/test_detect.py index 837d8cf..f6f1f32 100644 --- a/tests/test_detect.py +++ b/tests/test_detect.py @@ -544,7 +544,7 @@ def test_detect_converts_google_workspace_shortcuts_when_enabled(tmp_path, monke shortcut = tmp_path / "notes.gdoc" shortcut.write_text('{"doc_id":"doc-1"}', encoding="utf-8") - def fake_convert(path, out_dir, *, xlsx_to_markdown=None): + def fake_convert(path, out_dir, *, xlsx_to_markdown=None, root=None): out_dir.mkdir(parents=True, exist_ok=True) out = out_dir / "notes_converted.md" out.write_text("# Notes\n\nA converted Google Doc.", encoding="utf-8") @@ -1909,6 +1909,112 @@ def test_convert_office_file_does_not_rewrite_existing_sidecar(tmp_path, monkeyp assert second.stat().st_mtime_ns == mtime_before +def test_convert_office_file_sidecar_name_stable_across_checkouts(tmp_path, monkeypatch): + """#2059: the sidecar name must depend on the scan-root-RELATIVE path, not the + absolute checkout location, so the same tracked file in two clones/worktrees + produces the same sidecar name (no unbounded duplicates when graphify-out/ is + committed). Also verifies the no-root fallback matches the explicit form.""" + monkeypatch.setattr(detect_mod, "xlsx_to_markdown", lambda p: "sheet body") + + def _sidecar(root): + src = root / "docs" / "report.xlsx" + out_dir = root / "graphify-out" / "converted" + return detect_mod.convert_office_file(src, out_dir, root=root) + + checkout_a = tmp_path / "checkout-a" + checkout_b = tmp_path / "somewhere-else" / "checkout-b" + (checkout_a / "docs").mkdir(parents=True) + (checkout_b / "docs").mkdir(parents=True) + out_a = _sidecar(checkout_a) + out_b = _sidecar(checkout_b) + assert out_a is not None and out_b is not None + assert out_a.name == out_b.name, "sidecar name must be stable across checkouts (#2059)" + assert out_a.parent != out_b.parent # sanity: genuinely different locations + + # No explicit root -> the out_dir.parent.parent fallback yields the same name. + fallback = detect_mod.convert_office_file( + checkout_a / "docs" / "report.xlsx", checkout_a / "graphify-out" / "converted" + ) + assert fallback is not None and fallback.name == out_a.name + + +def test_convert_office_file_hash_disambiguates_same_stem(tmp_path, monkeypatch): + """Two same-stem Office files in different subdirs must still get distinct + sidecar names — the relative-path hash preserves the disambiguation purpose.""" + monkeypatch.setattr(detect_mod, "xlsx_to_markdown", lambda p: "body") + root = tmp_path / "repo" + (root / "a").mkdir(parents=True) + (root / "b").mkdir(parents=True) + out_dir = root / "graphify-out" / "converted" + out_a = detect_mod.convert_office_file(root / "a" / "report.xlsx", out_dir, root=root) + out_b = detect_mod.convert_office_file(root / "b" / "report.xlsx", out_dir, root=root) + assert out_a is not None and out_b is not None + assert out_a.name != out_b.name, "same-stem files in different dirs must differ (#2059)" + + +def test_convert_office_file_outside_root_falls_back(tmp_path, monkeypatch): + """A source outside the scan root (--include, custom layouts) falls back to the + absolute-path hash without raising, and stays deterministic.""" + monkeypatch.setattr(detect_mod, "docx_to_markdown", lambda p: "body") + root = tmp_path / "repo" + (root / "graphify-out" / "converted").mkdir(parents=True) + outside = tmp_path / "elsewhere" / "doc.docx" + out_dir = root / "graphify-out" / "converted" + out1 = detect_mod.convert_office_file(outside, out_dir, root=root) + out2 = detect_mod.convert_office_file(outside, out_dir, root=root) + assert out1 is not None and out1.name == out2.name + + +def test_detect_keeps_env_source_dirs(tmp_path): + """#2058: a real source directory named env/ or *_env/ with no virtualenv + markers must be indexed, not silently pruned as a false-positive venv.""" + src_env = tmp_path / "src_env" + (src_env / "env").mkdir(parents=True) + (src_env / "env" / "ctrl_mem_env.py").write_text("def build_env():\n return 1\n") + (src_env / "other_dir").mkdir() + (src_env / "other_dir" / "also_real.py").write_text("def x():\n return 2\n") + + all_files = [f for files in detect(tmp_path)["files"].values() for f in files] + assert any("ctrl_mem_env.py" in f for f in all_files), "env/ source dir wrongly pruned (#2058)" + assert any("also_real.py" in f for f in all_files), "*_env/ subtree wrongly pruned (#2058)" + + # Nested env/ under a scan root that IS the *_env dir (issue's exact-match case). + nested = [f for files in detect(src_env)["files"].values() for f in files] + assert any("ctrl_mem_env.py" in f for f in nested), "nested env/ pruned when scanned directly (#2058)" + + +def test_detect_still_prunes_real_env_venv(tmp_path): + """#2058: an env/ dir that IS a real virtualenv (has markers) is still pruned, + and the pruned dir is recorded in the traceable pruned_noise_dirs bucket.""" + venv = tmp_path / "env" + (venv / "lib").mkdir(parents=True) + (venv / "pyvenv.cfg").write_text("home = /usr/bin\n") + (venv / "lib" / "sixish.py").write_text("x = 1\n") + (tmp_path / "main.py").write_text("def main():\n return 1\n") + + result = detect(tmp_path) + all_files = [f for files in result["files"].values() for f in files] + assert not any("sixish.py" in f for f in all_files), "real venv env/ must still be pruned" + assert any("main.py" in f for f in all_files) + assert any(f"{os.sep}env{os.sep}" in d for d in result["pruned_noise_dirs"]), ( + "pruned venv must be traceable in pruned_noise_dirs (#2058)" + ) + + +def test_detect_prunes_venv_names_without_markers(tmp_path): + """#2058 must not loosen the unambiguous names: venv/.venv/*_venv are still + pruned by name alone (no markers needed).""" + for name in ("venv", ".venv", "my_venv"): + d = tmp_path / name + d.mkdir() + (d / "mod.py").write_text("y = 1\n") + (tmp_path / "app.py").write_text("def a():\n return 1\n") + all_files = [f for files in detect(tmp_path)["files"].values() for f in files] + assert any("app.py" in f for f in all_files) + for name in ("venv", ".venv", "my_venv"): + assert not any(f"{os.sep}{name}{os.sep}" in f for f in all_files), f"{name} must stay pruned" + + def test_detect_records_unclassified_extensionless_files(tmp_path): # #1692: extensionless, non-shebang project files (Dockerfile, Makefile, ...) # were considered but left no trace. detect() now lists them under