mirror of
https://github.com/safishamsi/graphify.git
synced 2026-10-14 16:09:26 +00:00
fix(build): ghost-merge nodes whose source_file is the referencing doc, not the referenced file
The #1145-extended ghost-merge (build_from_json) keys exclusively on (source_file, label). When a semantic pass reads a document that only *mentions* a real file by name (a saved graphify-out/memory/*.md query answer, a runbook like "see App.tsx"/"backup.sh does X"), the new node it mints gets source_file = the document being read, not the file actually named in the prose. That wrong source_file means the key can never match the real AST node's, so the ghost survives every rebuild. Add a conservative label-only fallback pass reusing the existing _is_file_node_label() predicate (already used for file-node label disambiguation) against every AST file-self node collected in Pass 1. Only remaps when the match is unique across the graph — 0 or 2+ candidates (e.g. two different index.ts files) are left alone, same conservatism the existing (source_file, label) key already applies via _loc_collisions. Verified against a real ~4600-node project graph: 10 long-standing ghost nodes removed across two unrelated documents (a memory query file and an ops runbook), confirming this is a general failure mode of the "document mentions a file by name" pattern, not a narrow edge case. Both hyperedges that had cited the phantoms now correctly cite only real, AST-backed nodes. Fixes #3344
This commit is contained in:
1 parent
937e59a547
commit
a67e7060b0
2 files changed
+102
No files matched your search
@@ -993,6 +993,7 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
_loc_nodes: dict[tuple[str, str], str] = {} # (source_file, label) -> canonical node id
|
||||
_loc_collisions: set[tuple[str, str]] = set() # keys shared by 2+ AST nodes
|
||||
_noloc_nodes: dict[tuple[str, str], str] = {} # (source_file, label) -> ghost node id
|
||||
_ast_file_nodes: list[tuple[str, str]] = [] # (node_id, source_file) for AST file-self nodes (#2963)
|
||||
|
||||
# Pass 1: collect canonical nodes — AST-origin nodes take precedence over LLM nodes.
|
||||
# When 2+ AST nodes share a key (same-named symbols in same-named files across
|
||||
@@ -1035,6 +1036,20 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
_loc_collisions.add(key)
|
||||
# AST-origin nodes always overwrite a prior non-AST entry.
|
||||
_loc_nodes[key] = nid
|
||||
# #2963: a self-referential semantic pass (e.g. re-extracting a
|
||||
# saved graphify-out/memory/*.md query answer) mints a NEW
|
||||
# non-AST node for every bare file path/basename it mentions in
|
||||
# prose ("App.tsx", "customer-app/index.ts"), stamped with
|
||||
# source_file = the memory doc being read, NOT the file named in
|
||||
# the prose. That wrong source_file means such a ghost can never
|
||||
# hit the (sf, label) key above — same underlying bug as #1145,
|
||||
# just with the (source_file, label) *pair* broken instead of
|
||||
# only the id. Record every AST node whose own label already
|
||||
# names its own file (_is_file_node_label — the same "is this a
|
||||
# file node" predicate the label-disambiguation pass uses) so
|
||||
# Pass 2b below can catch these by label alone.
|
||||
if _is_file_node_label(label, sf):
|
||||
_ast_file_nodes.append((nid, sf))
|
||||
else:
|
||||
# First non-AST node for this (file, label) wins as canonical; a
|
||||
# later same-key node is a genuine same-file duplicate and still
|
||||
@@ -1061,6 +1076,37 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
ast_id = _loc_nodes.get(key)
|
||||
if ast_id is not None:
|
||||
_ghost_remap[sem_id] = ast_id
|
||||
|
||||
# Pass 2b (#2963): catch ghosts the (source_file, label) key above cannot,
|
||||
# because their source_file is simply wrong — a semantic pass over a
|
||||
# document that only *mentions* a file (a saved graphify-out/memory/*.md
|
||||
# query answer, a README, an ADR) stamps the file's own name as a new
|
||||
# node's label but the DOCUMENT's path as source_file, since it has no way
|
||||
# to know the mentioned file's real path. Resolve these by label alone
|
||||
# against every AST file-self node collected in Pass 1, reusing
|
||||
# _is_file_node_label so "App.tsx" matches source_file ".../App.tsx" and
|
||||
# "customer-app/index.ts" matches ".../apps/customer-app/index.ts" (a
|
||||
# directory-qualified suffix, e.g. the leading "apps/" the prose dropped).
|
||||
# Conservative by construction: a label matching 0 or 2+ AST files is left
|
||||
# alone (0 = no known file, 2+ = genuinely ambiguous — same "no safe
|
||||
# canonical winner" rule Pass 2's _loc_collisions already applies).
|
||||
if _ast_file_nodes:
|
||||
for nid in sorted(node_set):
|
||||
if nid in _ghost_remap:
|
||||
continue # already resolved by the exact (sf, label) key
|
||||
attrs = G.nodes[nid]
|
||||
if attrs.get("_origin") == "ast":
|
||||
continue
|
||||
label = str(attrs.get("label", "")).strip()
|
||||
if not label:
|
||||
continue
|
||||
matches = {
|
||||
ast_id for ast_id, ast_sf in _ast_file_nodes
|
||||
if _is_file_node_label(label, ast_sf)
|
||||
}
|
||||
if len(matches) == 1:
|
||||
_ghost_remap[nid] = next(iter(matches))
|
||||
|
||||
# Remove ghost nodes from the graph; edges will be re-pointed via norm_to_id.
|
||||
for ghost_id in _ghost_remap:
|
||||
G.remove_node(ghost_id)
|
||||
|
||||
@@ -577,6 +577,62 @@ def test_ghost_merge_non_ast_same_file_still_merges():
|
||||
assert G.number_of_nodes() == 1
|
||||
|
||||
|
||||
def test_ghost_merge_wrong_source_file_resolved_by_label():
|
||||
"""#2963: a semantic node that only MENTIONS a file (a saved
|
||||
graphify-out/memory/*.md query answer, a runbook narrating "see App.tsx")
|
||||
is stamped with source_file = the document being read, not the file named
|
||||
in its prose — so the (source_file, label) key can never match the real
|
||||
AST node's key. Resolve it by label alone against every AST file-self node
|
||||
(_is_file_node_label), including when the doc's prose dropped a leading
|
||||
path segment ("customer-app/index.ts" for the real
|
||||
"apps/customer-app/index.ts")."""
|
||||
ext = {
|
||||
"nodes": [
|
||||
{"id": "apps_customer_app_app", "label": "App.tsx", "file_type": "code",
|
||||
"source_file": "apps/customer-app/App.tsx", "source_location": "L1", "_origin": "ast"},
|
||||
{"id": "apps_customer_app_index", "label": "customer-app/index.ts", "file_type": "code",
|
||||
"source_file": "apps/customer-app/index.ts", "source_location": "L1", "_origin": "ast"},
|
||||
{"id": "app", "label": "App.tsx", "file_type": "code",
|
||||
"source_file": "graphify-out/memory/query_fake.md", "_origin": "semantic"},
|
||||
{"id": "customer_app_index", "label": "customer-app/index.ts", "file_type": "code",
|
||||
"source_file": "graphify-out/memory/query_fake.md", "_origin": "semantic"},
|
||||
{"id": "some_query_node", "label": "Query: why does X connect Y?", "file_type": "document",
|
||||
"source_file": "graphify-out/memory/query_fake.md", "_origin": "semantic"},
|
||||
],
|
||||
"edges": [
|
||||
{"source": "some_query_node", "target": "app", "relation": "references",
|
||||
"confidence": "EXTRACTED", "source_file": "graphify-out/memory/query_fake.md"},
|
||||
{"source": "some_query_node", "target": "customer_app_index", "relation": "references",
|
||||
"confidence": "EXTRACTED", "source_file": "graphify-out/memory/query_fake.md"},
|
||||
],
|
||||
}
|
||||
G = build_from_json(ext, directed=False)
|
||||
assert "app" not in G.nodes() and "customer_app_index" not in G.nodes()
|
||||
assert G.has_edge("some_query_node", "apps_customer_app_app")
|
||||
assert G.has_edge("some_query_node", "apps_customer_app_index")
|
||||
|
||||
|
||||
def test_ghost_merge_wrong_source_file_ambiguous_basename_left_alone():
|
||||
"""#2963: the label-alone fallback must stay conservative — a phantom
|
||||
mentioning a bare basename that TWO different real files share (an
|
||||
"index.ts" in two different directories) has no safe unique winner and
|
||||
must be left as-is rather than merged into an arbitrary one."""
|
||||
ext = {
|
||||
"nodes": [
|
||||
{"id": "apps_admin_web_index", "label": "index.ts", "file_type": "code",
|
||||
"source_file": "apps/admin-web/src/lib/index.ts", "source_location": "L1", "_origin": "ast"},
|
||||
{"id": "apps_api_index", "label": "index.ts", "file_type": "code",
|
||||
"source_file": "apps/api/src/lib/index.ts", "source_location": "L1", "_origin": "ast"},
|
||||
{"id": "phantom_index", "label": "index.ts", "file_type": "code",
|
||||
"source_file": "graphify-out/memory/query_fake2.md", "_origin": "semantic"},
|
||||
],
|
||||
"edges": [],
|
||||
}
|
||||
G = build_from_json(ext, directed=False)
|
||||
assert "phantom_index" in G.nodes()
|
||||
assert "apps_admin_web_index" in G.nodes() and "apps_api_index" in G.nodes()
|
||||
|
||||
|
||||
def test_build_merge_preserves_call_edge_direction(tmp_path):
|
||||
"""Regression for #760.
|
||||
|
||||
|
||||
Reference in new issue
Block a user