mirror of
https://github.com/safishamsi/graphify.git
synced 2026-09-15 10:05:36 +00:00
fix(build): ghost-merge keys on full source_file, not bare basename (#2068)
build_from_json's #1145 ghost-duplicate merge keyed on (Path(source_file).name, label), discarding the directory, so unrelated nodes from different files sharing a common basename (index.md, README.md, ...) and a generic label were silently merged onto one survivor with their edges rewired — corrupting multi-corpus doc graphs. The AST/LLM ghost twins the merge legitimately targets always share the same source_file, so keying on the full normalized source_file preserves #1145 while making cross-directory false merges impossible. This subsumes the #1753/#1257 cross-file ambiguity guard (now removed as dead code). Independent of the #2032 label pass. Updated the #1257 test to the now-correct precise merge.
This commit is contained in:
+20
-26
@@ -625,9 +625,9 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
# populates source_location, so those ghosts survived. Extended fix: use
|
||||
# _origin=="ast" as the canonical signal. AST nodes always win; any non-AST
|
||||
# node sharing (basename, label) with an AST node is a ghost.
|
||||
_loc_nodes: dict[tuple[str, str], str] = {} # (basename, label) -> canonical node id
|
||||
_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] = {} # (basename, label) -> ghost node id
|
||||
_noloc_nodes: dict[tuple[str, str], str] = {} # (source_file, label) -> ghost node id
|
||||
|
||||
# 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
|
||||
@@ -643,36 +643,31 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
attrs = G.nodes[nid]
|
||||
label = str(attrs.get("label", "")).strip()
|
||||
sf = str(attrs.get("source_file", ""))
|
||||
basename = Path(sf).name if sf else ""
|
||||
if not label or not basename:
|
||||
if not label or not sf:
|
||||
continue
|
||||
is_ast = attrs.get("_origin") == "ast"
|
||||
if attrs.get("source_location") or is_ast:
|
||||
key = (basename, label)
|
||||
# Key on the FULL normalized source_file, not the bare basename
|
||||
# (#2068): the AST/LLM ghost twins of #1145 always share the same
|
||||
# source_file (different ids, same file), so full-path keying still
|
||||
# collapses them, while unrelated same-basename nodes in DIFFERENT
|
||||
# directories (docs/a/index.md vs docs/b/index.md) now get distinct
|
||||
# keys and are never falsely merged. This subsumes the #1753/#1257
|
||||
# cross-file ambiguity guard, which is why the non-AST branch below
|
||||
# no longer needs it.
|
||||
key = (sf, label)
|
||||
if is_ast:
|
||||
# Two AST nodes on the same key is an ambiguous collision.
|
||||
# Two AST nodes on the same key (same file, same label) is an
|
||||
# ambiguous collision.
|
||||
if key in _loc_nodes and G.nodes[_loc_nodes[key]].get("_origin") == "ast":
|
||||
_loc_collisions.add(key)
|
||||
# AST-origin nodes always overwrite a prior non-AST entry.
|
||||
_loc_nodes[key] = nid
|
||||
else:
|
||||
existing = _loc_nodes.get(key)
|
||||
if existing is None:
|
||||
_loc_nodes[key] = nid
|
||||
elif (
|
||||
G.nodes[existing].get("_origin") != "ast"
|
||||
and str(G.nodes[existing].get("source_file", "")) != sf
|
||||
):
|
||||
# Two NON-AST nodes sharing (basename, label) but coming from
|
||||
# DIFFERENT files are distinct concepts (e.g. a same-named
|
||||
# concept in dir_a/update.md and dir_b/update.md), not an AST
|
||||
# ghost/canonical twin. Merging them would drop a real node
|
||||
# and pick the survivor arbitrarily via iteration order
|
||||
# (#1753). Mark the key ambiguous so Pass 2 leaves both, the
|
||||
# same conservatism the AST/AST case uses (#1257). A genuine
|
||||
# same-file duplicate (identical source_file) is not flagged
|
||||
# and still collapses.
|
||||
_loc_collisions.add(key)
|
||||
# First non-AST node for this (file, label) wins as canonical; a
|
||||
# later same-key node is a genuine same-file duplicate and still
|
||||
# collapses in Pass 2.
|
||||
_loc_nodes.setdefault(key, nid)
|
||||
|
||||
# Pass 2: find ghosts — non-AST nodes that have an AST canonical twin.
|
||||
for nid in sorted(node_set):
|
||||
@@ -681,10 +676,9 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
continue # AST nodes are never ghosts
|
||||
label = str(attrs.get("label", "")).strip()
|
||||
sf = str(attrs.get("source_file", ""))
|
||||
basename = Path(sf).name if sf else ""
|
||||
if not label or not basename:
|
||||
if not label or not sf:
|
||||
continue
|
||||
key = (basename, label)
|
||||
key = (sf, label)
|
||||
if key in _loc_collisions:
|
||||
continue # ambiguous key: no safe canonical winner, leave ghost intact
|
||||
if key in _loc_nodes and _loc_nodes[key] != nid:
|
||||
|
||||
+35
-10
@@ -270,10 +270,12 @@ def test_ghost_merge_unique_located_node_still_merges():
|
||||
assert G.has_edge("caller", "ast_render")
|
||||
|
||||
|
||||
def test_ghost_merge_skipped_on_basename_collision():
|
||||
"""#1257: when two files with the same basename both define a symbol with the
|
||||
same label, the (basename, label) key is ambiguous and the semantic ghost
|
||||
must not be merged into an arbitrary one of them."""
|
||||
def test_ghost_merge_uses_source_file_not_basename():
|
||||
"""#2068: the ghost-merge key is the full source_file, not the bare basename.
|
||||
A ghost from src/a/index.ts merges into THAT file's AST node (a_render), never
|
||||
the unrelated same-basename b_render in src/b/index.ts. (Pre-#2068 the
|
||||
(basename, label) key made ('index.ts','render') ambiguous and skipped the
|
||||
merge; the directory-aware key resolves it precisely.)"""
|
||||
ext = {
|
||||
"nodes": [
|
||||
{"id": "a_render", "label": "render", "file_type": "code",
|
||||
@@ -290,13 +292,36 @@ def test_ghost_merge_skipped_on_basename_collision():
|
||||
"input_tokens": 0, "output_tokens": 0,
|
||||
}
|
||||
G = build_from_json(ext)
|
||||
# The ghost survives: merging it into either a_render or b_render would
|
||||
# pick an arbitrary winner (set iteration order over node_set).
|
||||
assert "ghost_render" in G.nodes()
|
||||
assert G.number_of_nodes() == 4
|
||||
assert G.has_edge("caller", "ghost_render")
|
||||
assert not G.has_edge("caller", "a_render")
|
||||
# Ghost merges into its same-file twin; the edge re-points to a_render only.
|
||||
assert "ghost_render" not in G.nodes()
|
||||
assert G.has_edge("caller", "a_render")
|
||||
assert not G.has_edge("caller", "b_render")
|
||||
# The unrelated same-basename node in another directory is untouched.
|
||||
assert "b_render" in G.nodes()
|
||||
|
||||
|
||||
def test_ghost_merge_not_across_directories_same_basename():
|
||||
"""#2068: two unrelated non-AST nodes with the same basename+label in
|
||||
DIFFERENT directories must NOT be merged onto one survivor (the bug: bare
|
||||
basename collapsed docs/product_a/index.md and docs/product_b/index.md)."""
|
||||
ext = {
|
||||
"nodes": [
|
||||
{"id": "docs_a_index", "label": "Quickstart", "file_type": "document",
|
||||
"source_file": "docs/product_a/index.md", "source_location": "L1"},
|
||||
{"id": "docs_b_index", "label": "Quickstart", "file_type": "document",
|
||||
"source_file": "docs/product_b/index.md"},
|
||||
{"id": "docs_hub", "label": "Docs", "file_type": "concept",
|
||||
"source_file": "docs/hub.md", "source_location": "L1"},
|
||||
],
|
||||
"edges": [{"source": "docs_hub", "target": "docs_b_index", "relation": "links_to",
|
||||
"confidence": "INFERRED", "source_file": "docs/hub.md"}],
|
||||
"input_tokens": 0, "output_tokens": 0,
|
||||
}
|
||||
G = build_from_json(ext, directed=False)
|
||||
# Both docs survive; the edge stays on the file it was authored against.
|
||||
assert "docs_a_index" in G.nodes() and "docs_b_index" in G.nodes()
|
||||
assert G.has_edge("docs_hub", "docs_b_index")
|
||||
assert not G.has_edge("docs_hub", "docs_a_index")
|
||||
|
||||
|
||||
def test_ghost_merge_non_ast_different_files_both_survive():
|
||||
|
||||
Reference in New Issue
Block a user