mirror of
https://github.com/safishamsi/graphify.git
synced 2026-07-13 02:47:00 +00:00
fix: don't resolve symlinks when relativizing manifest/cache keys (#777)
`_to_relative_for_storage` (detect.py) and `_relativize_source_files_in` (cache.py) called `.resolve()` on the file key before `relative_to(root)`, so an in-root symlink — e.g. `alias.py -> sub/target.py` — was stored under the resolved target's path (`sub/target.py`) instead of `alias.py`. `detect()` emits unresolved keys (from `os.walk`), so on the next incremental run the `alias.py` key missed the manifest lookup → that file re-extracted on every run. Not a correctness break, but a redundant full-extract-per-symlink regression. Switch to `os.path.relpath` against a resolved root only. The key itself stays symbolic, so symlinks round-trip under their own name. Preserve the prior out-of-root semantics by falling back to the absolute key when the computed relative path escapes root (`..` prefix). Same fix applied to `cache.py` for consistency — lower-impact there since cache lookup is content-hashed, not key-matched. Adds two regression tests covering the in-root symlink round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
+10
-2
@@ -153,6 +153,11 @@ def _relativize_source_files_in(payload: dict, root: Path) -> None:
|
||||
Mirror of :func:`graphify.watch._relativize_source_files` so cached
|
||||
extraction fragments persist in portable form (#777). Already-relative
|
||||
fields and out-of-root paths pass through unchanged.
|
||||
|
||||
Only ``root`` is resolved — ``source_file`` itself is relativized
|
||||
symbolically so in-root symlinks keep their original name rather than
|
||||
pointing at the resolved target. Same reasoning as
|
||||
:func:`graphify.detect._to_relative_for_storage`.
|
||||
"""
|
||||
try:
|
||||
root_resolved = Path(root).resolve()
|
||||
@@ -169,9 +174,12 @@ def _relativize_source_files_in(payload: dict, root: Path) -> None:
|
||||
if not sp.is_absolute():
|
||||
continue
|
||||
try:
|
||||
item["source_file"] = sp.resolve().relative_to(root_resolved).as_posix()
|
||||
rel = os.path.relpath(sp, root_resolved)
|
||||
except (ValueError, OSError):
|
||||
continue
|
||||
continue # out-of-root (e.g. Windows cross-drive)
|
||||
if rel == ".." or rel.startswith(".." + os.sep) or rel.startswith("../"):
|
||||
continue # escaped root — keep absolute
|
||||
item["source_file"] = rel.replace(os.sep, "/")
|
||||
|
||||
|
||||
def _absolutize_source_files_in(payload: dict, root: Path) -> None:
|
||||
|
||||
+14
-2
@@ -1099,14 +1099,26 @@ def _to_relative_for_storage(key: str, root: Path) -> str:
|
||||
fallback in :func:`graphify.watch._relativize_source_files` so the
|
||||
on-disk artifact survives the round-trip even when some paths cannot be
|
||||
portably encoded.
|
||||
|
||||
Only ``root`` is resolved — the key itself is relativized symbolically
|
||||
so an in-root symlink (e.g. ``alias.py -> sub/target.py``) is stored
|
||||
under its own name. Resolving the key would point the stored entry at
|
||||
the symlink target, and the original key would then miss on reload and
|
||||
re-extract on every incremental run.
|
||||
"""
|
||||
p = Path(key)
|
||||
if not p.is_absolute():
|
||||
return key
|
||||
try:
|
||||
return p.resolve().relative_to(Path(root).resolve()).as_posix()
|
||||
rel = os.path.relpath(p, Path(root).resolve())
|
||||
except (ValueError, OSError):
|
||||
return key # outside root or unresolvable
|
||||
return key # outside root (e.g. Windows cross-drive)
|
||||
# ``os.path.relpath`` happily produces ``../foo`` for paths outside
|
||||
# root; mirror the prior ``relative_to``-raises-ValueError semantics by
|
||||
# keeping out-of-root entries in their absolute form.
|
||||
if rel == ".." or rel.startswith(".." + os.sep) or rel.startswith("../"):
|
||||
return key
|
||||
return rel.replace(os.sep, "/")
|
||||
|
||||
|
||||
def _to_absolute_from_storage(key: str, root: Path) -> str:
|
||||
|
||||
@@ -237,3 +237,36 @@ def test_cache_portable_across_roots(tmp_path):
|
||||
# Source path re-anchored to the new root, not the old one.
|
||||
assert loaded["nodes"][0]["source_file"] == str(src_b.resolve())
|
||||
assert not str(repo_a) in loaded["nodes"][0]["source_file"]
|
||||
|
||||
|
||||
def test_save_cached_in_root_symlink_keeps_symlink_name(tmp_path):
|
||||
"""``source_file`` for an in-root symlink must be stored under the
|
||||
symlink's own name, not the resolved target. Lower-impact than the
|
||||
manifest case (cache lookup is content-hashed, not key-matched), but
|
||||
keeps the on-disk shape consistent with what callers passed in."""
|
||||
import json
|
||||
from graphify.cache import save_cached, file_hash, cache_dir
|
||||
|
||||
(tmp_path / "sub").mkdir()
|
||||
target = tmp_path / "sub" / "target.py"
|
||||
target.write_text("pass\n")
|
||||
alias = tmp_path / "alias.py"
|
||||
try:
|
||||
alias.symlink_to(target)
|
||||
except (OSError, NotImplementedError):
|
||||
import pytest
|
||||
pytest.skip("filesystem does not support symlinks")
|
||||
|
||||
abs_alias = str(alias) # caller's view — the symlink path, unresolved
|
||||
save_cached(alias, {
|
||||
"nodes": [{"id": "n1", "source_file": abs_alias}],
|
||||
"edges": [],
|
||||
}, root=tmp_path, kind="ast")
|
||||
|
||||
h = file_hash(alias, tmp_path)
|
||||
entry = cache_dir(tmp_path, "ast") / f"{h}.json"
|
||||
on_disk = json.loads(entry.read_text(encoding="utf-8"))
|
||||
assert on_disk["nodes"][0]["source_file"] == "alias.py", (
|
||||
f"cache must store symlink name, not resolved target; got "
|
||||
f"{on_disk['nodes'][0]['source_file']!r}"
|
||||
)
|
||||
|
||||
@@ -1202,3 +1202,37 @@ def test_detect_incremental_portable_across_paths(tmp_path):
|
||||
assert inc["new_total"] == 0, (
|
||||
f"manifest must port across absolute paths; got new_total={inc['new_total']}"
|
||||
)
|
||||
|
||||
|
||||
def test_save_manifest_in_root_symlink_roundtrips(tmp_path):
|
||||
"""In-root symlinks must store under the symlink's own name, not the
|
||||
resolved target. Resolving the key when relativizing pointed the stored
|
||||
entry at ``sub/target.py`` instead of ``alias.py``, so the original
|
||||
``alias.py`` key missed on reload and re-extracted on every incremental
|
||||
run."""
|
||||
import json
|
||||
from graphify.detect import save_manifest, load_manifest
|
||||
|
||||
(tmp_path / "sub").mkdir()
|
||||
target = tmp_path / "sub" / "target.py"
|
||||
target.write_text("pass\n")
|
||||
alias = tmp_path / "alias.py"
|
||||
try:
|
||||
alias.symlink_to(target)
|
||||
except (OSError, NotImplementedError):
|
||||
import pytest
|
||||
pytest.skip("filesystem does not support symlinks")
|
||||
|
||||
manifest_path = str(tmp_path / "graphify-out" / "manifest.json")
|
||||
save_manifest({"code": [str(alias)]}, manifest_path, root=tmp_path)
|
||||
|
||||
raw = json.loads(Path(manifest_path).read_text(encoding="utf-8"))
|
||||
assert "alias.py" in raw, (
|
||||
f"in-root symlink must be stored under its own name, got {list(raw)}"
|
||||
)
|
||||
assert "sub/target.py" not in raw, (
|
||||
f"symlink must not be stored under resolved target path; got {list(raw)}"
|
||||
)
|
||||
|
||||
loaded = load_manifest(manifest_path, root=tmp_path)
|
||||
assert str(tmp_path.resolve() / "alias.py") in loaded
|
||||
|
||||
Reference in New Issue
Block a user