mirror of
https://github.com/safishamsi/graphify.git
synced 2026-07-20 22:31:48 +00:00
match AST file-level node IDs to the skill.md spec (fixes #1033)
AST file nodes were ID'd from the full relative path plus extension
(match_script_pipeline_step_py) while semantic subagents follow the
{parent_dir}_{stem} spec (script_pipeline_step), so every file split
into two disconnected ghost nodes.
Fix at the single remap chokepoint in extract(): file node IDs and all
edge endpoints already funnel through the #502 relative-path remap, so
changing that remap to emit _file_node_id (one parent dir, no extension)
converts the node and every referencing edge together - Python, TS, Lua,
C and bash import edges all stay connected. symbol_resolution pre-computes
the canonical form directly (bypassing the remap) so it is synced too.
Per-site conversion (as attempted in #1038/#1065) orphans edges because
it moves the node without the edge targets; the chokepoint approach
avoids that entirely.
Backward compat for existing graphs: graphify extract --force, as the
skill.md spec already documents.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 4.6
parent
8db19d6c85
commit
c898dc62cd
+22
-5
@@ -85,6 +85,16 @@ def _file_stem(path: Path) -> str:
|
||||
return path.stem
|
||||
|
||||
|
||||
def _file_node_id(rel_path: Path) -> str:
|
||||
"""File-level node ID matching the skill.md spec: ``{parent_dir}_{stem}`` —
|
||||
one parent directory level, no extension. ``rel_path`` MUST be relative to
|
||||
the project root so top-level files collapse to a bare stem (``setup.py`` ->
|
||||
``setup``) instead of picking up the root directory name. This must equal the
|
||||
ID semantic subagents generate, or AST and semantic extraction split a file
|
||||
into two disconnected ghost nodes (#1033)."""
|
||||
return _make_id(_file_stem(rel_path))
|
||||
|
||||
|
||||
_TSCONFIG_ALIAS_CACHE: dict[str, dict[str, str]] = {}
|
||||
_WORKSPACE_PACKAGE_CACHE: dict[str, dict[str, Path]] = {}
|
||||
_JS_CACHE_BYPASS_SUFFIXES = {".js", ".jsx", ".mjs", ".ts", ".tsx", ".vue", ".svelte"}
|
||||
@@ -10370,15 +10380,22 @@ def extract(
|
||||
|
||||
_augment_symbol_resolution_edges(paths, all_nodes, all_edges, root)
|
||||
|
||||
# Remap file node IDs from absolute-path-derived to project-relative so
|
||||
# graph.json edge endpoints are stable across machines (#502)
|
||||
# Remap file node IDs from absolute-path-derived to the canonical
|
||||
# {parent_dir}_{stem} spec form so (a) graph.json edge endpoints are stable
|
||||
# across machines (#502) and (b) AST file nodes match the IDs semantic
|
||||
# subagents generate (#1033). Resolve before relativizing so paths passed in
|
||||
# relative form still anchor to the (resolved) root.
|
||||
id_remap: dict[str, str] = {}
|
||||
for path in paths:
|
||||
old_id = _make_id(str(path))
|
||||
try:
|
||||
new_id = _make_id(str(path.relative_to(root)))
|
||||
rel = path.relative_to(root)
|
||||
except ValueError:
|
||||
continue
|
||||
try:
|
||||
rel = path.resolve().relative_to(root)
|
||||
except ValueError:
|
||||
continue
|
||||
new_id = _file_node_id(rel)
|
||||
if old_id != new_id:
|
||||
id_remap[old_id] = new_id
|
||||
if id_remap:
|
||||
@@ -10462,7 +10479,7 @@ def extract(
|
||||
sf_rel = sf_path.relative_to(root) if sf_path.is_absolute() else sf_path
|
||||
except ValueError:
|
||||
sf_rel = sf_path
|
||||
nid_to_file_nid[n["id"]] = _make_id(str(sf_rel))
|
||||
nid_to_file_nid[n["id"]] = _file_node_id(sf_rel)
|
||||
|
||||
existing_pairs = {(e["source"], e["target"]) for e in all_edges}
|
||||
for rc in all_raw_calls:
|
||||
|
||||
@@ -365,14 +365,24 @@ def _bash_make_id(*parts: str) -> str:
|
||||
return cleaned.strip("_").casefold()
|
||||
|
||||
|
||||
def _bash_file_stem(rel_path: Path) -> str:
|
||||
"""Exact copy of extract._file_stem — kept here to avoid an import cycle."""
|
||||
parent = rel_path.parent.name
|
||||
if parent and parent not in (".", ""):
|
||||
return f"{parent}.{rel_path.stem}"
|
||||
return rel_path.stem
|
||||
|
||||
|
||||
def _file_node_id_for_path(path: Path, root: Path) -> str:
|
||||
# Resolve both sides so callers that pass relative or non-canonical roots
|
||||
# get the same canonical relative path that extract()'s id_remap produces.
|
||||
# _bash_make_id is an exact copy of extract._make_id, so IDs match.
|
||||
# Produce the canonical {parent_dir}_{stem} file-node ID that extract()'s
|
||||
# id_remap generates (#1033), so bash `source` edges land on the real file
|
||||
# node instead of an orphan. _bash_make_id / _bash_file_stem are exact copies
|
||||
# of extract._make_id / extract._file_stem, so IDs match.
|
||||
try:
|
||||
return _bash_make_id(str(path.resolve().relative_to(root.resolve())))
|
||||
rel = path.resolve().relative_to(root.resolve())
|
||||
except ValueError:
|
||||
return _bash_make_id(str(path)) # path outside root: hash absolute path as fallback
|
||||
return _bash_make_id(_bash_file_stem(rel))
|
||||
|
||||
|
||||
def resolve_bash_source_edges(
|
||||
|
||||
@@ -0,0 +1,112 @@
|
||||
"""Regression tests for issue #1033: AST file-level node IDs must match the
|
||||
skill.md `{parent_dir}_{stem}` spec (one parent level, no extension) so AST and
|
||||
semantic extraction produce the SAME node for a file instead of two disconnected
|
||||
ghosts.
|
||||
|
||||
skill.md spec (line ~390):
|
||||
stem = {parent_dir}_{filename_without_ext}, lowercased, non-alphanumeric -> _
|
||||
examples:
|
||||
src/auth/session.py + ValidateToken -> auth_session_validatetoken
|
||||
match/script/pipeline_step.py (file node) -> script_pipeline_step
|
||||
setup.py (top-level) -> setup
|
||||
"""
|
||||
from pathlib import Path
|
||||
|
||||
from graphify.extract import extract
|
||||
|
||||
|
||||
def _file_nodes(extraction: dict) -> list[dict]:
|
||||
# File-level nodes carry a label equal to the file's basename.
|
||||
return [
|
||||
n for n in extraction["nodes"]
|
||||
if n.get("source_file", "").endswith(n.get("label", "\0"))
|
||||
and n.get("file_type") == "code"
|
||||
]
|
||||
|
||||
|
||||
def test_file_node_id_uses_parent_dir_and_stem_no_extension(tmp_path):
|
||||
"""match/script/pipeline_step.py -> file node id 'script_pipeline_step'."""
|
||||
sub = tmp_path / "match" / "script"
|
||||
sub.mkdir(parents=True)
|
||||
f = sub / "pipeline_step.py"
|
||||
f.write_text("def run():\n pass\n")
|
||||
|
||||
extraction = extract([f], cache_root=tmp_path)
|
||||
ids = {n["id"] for n in extraction["nodes"]}
|
||||
|
||||
assert "script_pipeline_step" in ids, (
|
||||
f"expected spec-format file id 'script_pipeline_step', got {sorted(ids)}"
|
||||
)
|
||||
# The old buggy full-path-with-extension id must be gone.
|
||||
assert "match_script_pipeline_step_py" not in ids
|
||||
assert not any(i.endswith("_py") for i in ids if "pipeline_step" in i)
|
||||
|
||||
|
||||
def test_top_level_file_node_id_is_bare_stem(tmp_path):
|
||||
"""A file directly at the project root collapses to just its stem."""
|
||||
f = tmp_path / "setup.py"
|
||||
f.write_text("def configure():\n pass\n")
|
||||
|
||||
extraction = extract([f], cache_root=tmp_path)
|
||||
ids = {n["id"] for n in extraction["nodes"]}
|
||||
|
||||
assert "setup" in ids, f"expected bare stem 'setup', got {sorted(ids)}"
|
||||
assert "setup_py" not in ids
|
||||
|
||||
|
||||
def test_symbol_and_file_ids_share_the_same_stem(tmp_path):
|
||||
"""Symbol ids already use {parent}_{stem}_{name}; the file node must share
|
||||
that stem prefix so 'contains' edges connect file -> symbol."""
|
||||
sub = tmp_path / "match" / "script"
|
||||
sub.mkdir(parents=True)
|
||||
f = sub / "pipeline_step.py"
|
||||
f.write_text("def run():\n pass\n\nclass Stage:\n pass\n")
|
||||
|
||||
extraction = extract([f], cache_root=tmp_path)
|
||||
ids = {n["id"] for n in extraction["nodes"]}
|
||||
|
||||
assert "script_pipeline_step" in ids # file node
|
||||
assert "script_pipeline_step_stage" in ids # class symbol shares stem
|
||||
|
||||
# The file -> class 'contains' edge must reference the real file node id.
|
||||
contains = [
|
||||
e for e in extraction["edges"]
|
||||
if e["relation"] == "contains" and e["target"] == "script_pipeline_step_stage"
|
||||
]
|
||||
assert contains, "no 'contains' edge to the class symbol"
|
||||
assert contains[0]["source"] == "script_pipeline_step", (
|
||||
f"contains edge source {contains[0]['source']!r} does not match file node"
|
||||
)
|
||||
|
||||
|
||||
def test_cross_file_import_edges_stay_connected(tmp_path):
|
||||
"""Changing the file-id format must not orphan import edges: the import
|
||||
target must resolve to the imported file's (new-format) node id."""
|
||||
pkg = tmp_path / "pkg"
|
||||
pkg.mkdir()
|
||||
(pkg / "models.py").write_text("class User:\n pass\n")
|
||||
(pkg / "auth.py").write_text(
|
||||
"from models import User\n\n"
|
||||
"class Session:\n"
|
||||
" def check(self):\n"
|
||||
" return User()\n"
|
||||
)
|
||||
|
||||
files = [pkg / "models.py", pkg / "auth.py"]
|
||||
extraction = extract(files, cache_root=tmp_path)
|
||||
ids = {n["id"] for n in extraction["nodes"]}
|
||||
|
||||
assert "pkg_models" in ids
|
||||
assert "pkg_auth" in ids
|
||||
|
||||
# Every edge endpoint that looks like a file node must point at a real node
|
||||
# (no dangling '*_py' ghosts left behind by the old format).
|
||||
node_ids = ids
|
||||
for e in extraction["edges"]:
|
||||
for endpoint in (e["source"], e["target"]):
|
||||
assert not endpoint.endswith("_py"), (
|
||||
f"edge endpoint {endpoint!r} kept the old extension-suffixed format"
|
||||
)
|
||||
# imports_from edges between files must land on a known node.
|
||||
if e["relation"] == "imports_from" and e["source"] == "pkg_auth":
|
||||
assert e["target"] in node_ids or "models" in e["target"]
|
||||
@@ -3,7 +3,7 @@ from __future__ import annotations
|
||||
import json
|
||||
from pathlib import Path
|
||||
|
||||
from graphify.extract import _file_stem, _make_id, extract
|
||||
from graphify.extract import _file_node_id, _file_stem, _make_id, extract
|
||||
|
||||
|
||||
def _write(path: Path, text: str) -> Path:
|
||||
@@ -17,7 +17,7 @@ def _extract_for(paths: list[Path], root: Path):
|
||||
|
||||
|
||||
def _has_edge(result: dict, source: str, target: str, relation: str = "imports_from") -> bool:
|
||||
expected = (_make_id(source), _make_id(target), relation)
|
||||
expected = (_file_node_id(Path(source)), _file_node_id(Path(target)), relation)
|
||||
actual = {
|
||||
(edge["source"], edge["target"], edge["relation"])
|
||||
for edge in result["edges"]
|
||||
@@ -32,7 +32,7 @@ def _has_symbol_edge(
|
||||
symbol: str,
|
||||
relation: str = "imports",
|
||||
) -> bool:
|
||||
expected = (_make_id(source), _make_id(_file_stem(Path(target_file)), symbol), relation)
|
||||
expected = (_file_node_id(Path(source)), _make_id(_file_stem(Path(target_file)), symbol), relation)
|
||||
actual = {
|
||||
(edge["source"], edge["target"], edge["relation"])
|
||||
for edge in result["edges"]
|
||||
|
||||
Reference in New Issue
Block a user