mirror of
https://github.com/safishamsi/graphify.git
synced 2026-07-12 10:27:11 +00:00
fix(build): don't let an ambiguous legacy-stem alias silently merge files
build_from_json's "pre-migration alias index" (#1504) registers each real file's OLD-style bare-stem id (extension dropped) as an alias so a stale cached fragment referencing that old form still resolves after an id-scheme migration. It never checked for collisions: two unrelated real files easily compute the same bare alias (e.g. "ping.h" and "ping.php" in different directories both alias to "ping"), and dict.setdefault let whichever file happened to iterate first in a Python set win, arbitrarily. A dangling edge with a bare, deliberately-unscoped fallback target (e.g. the C/C++ extractor's last-resort id for an #include it couldn't resolve to a real path) would ride that alias onto whatever unrelated file won the collision -- silently wiring, say, a C++ server file to an unrelated PHP script, purely because both files happen to share a common basename somewhere in the corpus. Now every candidate for an alias is collected before any of them are committed to norm_to_id, and the alias is only trusted when exactly one real file claims it. Ambiguous aliases are dropped entirely, so the dangling edge correctly stays dangling (and gets discarded) instead of merging two unrelated files -- same "don't guess through ambiguity" principle already applied to stub-node disambiguation and cross-file call resolution elsewhere in this codebase. Found via graphify-practice round 2: a `path` query between two unrelated symbols routed through exactly this kind of bogus edge, traced back to Dev/Poker/TDServer/server.cpp's unresolved `#include "ping.h"` / `#include "utility.h"` landing on unrelated www.masque.com PHP scripts of the same bare name.
This commit is contained in:
+18
-2
@@ -507,7 +507,20 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
# fragment (e.g. an incremental update whose fragment references a symbol in a
|
||||
# file that was NOT re-extracted) still resolves to the migrated node instead
|
||||
# of dangling. Only fills gaps — never overrides a real node id.
|
||||
#
|
||||
# The old-stem form drops the extension and (for the file node itself) every
|
||||
# directory but the immediate parent, so it collapses easily: "ping.h" and
|
||||
# "ping.php" in different directories both alias to bare "ping". Collecting
|
||||
# every candidate for an alias BEFORE committing any of them — and only
|
||||
# committing when exactly one candidate claims it — keeps this a precise
|
||||
# re-keying aid instead of a silent cross-file (and cross-language) merge.
|
||||
# Without this, a dangling edge to a bare, deliberately-unscoped fallback id
|
||||
# (e.g. the C/C++ extractor's last-resort target for an #include it couldn't
|
||||
# resolve to a real path) could ride this alias onto whichever unrelated
|
||||
# same-stem file happened to be inserted first into ``node_set`` — a Python
|
||||
# set, so "first" is hash-order, not anything meaningful.
|
||||
from graphify.extractors.base import _file_stem as _fs
|
||||
_alias_candidates: dict[str, set[str]] = {}
|
||||
for nid in node_set:
|
||||
attrs = G.nodes[nid]
|
||||
sf = attrs.get("source_file")
|
||||
@@ -524,8 +537,11 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
if old_stem == new_stem:
|
||||
continue
|
||||
alias = old_stem + suffix
|
||||
norm_to_id.setdefault(_normalize_id(alias), nid)
|
||||
norm_to_id.setdefault(alias, nid)
|
||||
_alias_candidates.setdefault(_normalize_id(alias), set()).add(nid)
|
||||
_alias_candidates.setdefault(alias, set()).add(nid)
|
||||
for alias_key, candidates in _alias_candidates.items():
|
||||
if len(candidates) == 1:
|
||||
norm_to_id.setdefault(alias_key, next(iter(candidates)))
|
||||
# Iterate edges in a deterministic order. The graph is undirected and stores
|
||||
# direction in _src/_tgt; when two edges collapse onto the same node pair the
|
||||
# last write wins, so an unstable iteration order flips _src/_tgt run-to-run
|
||||
|
||||
@@ -505,6 +505,65 @@ def test_build_relativizes_absolute_source_file(tmp_path):
|
||||
assert sf == "src/main.py"
|
||||
|
||||
|
||||
def test_build_from_json_ambiguous_old_stem_alias_stays_dangling(tmp_path):
|
||||
"""The #1504 old-stem alias (e.g. "ping.h" -> bare "ping") is meant to let a
|
||||
stale-id edge from an un-re-extracted fragment still find its own file after
|
||||
a rekey. But the old-stem form drops the extension and most of the path, so
|
||||
two unrelated real files easily collapse onto the same bare alias (a C header
|
||||
and a PHP script both named "ping", in different directories). A dangling
|
||||
edge produced by an unrelated third file's own unscoped fallback id (e.g. the
|
||||
C/C++ extractor's last-resort target for an #include it couldn't resolve to
|
||||
a real path) must not silently ride that alias onto an arbitrary one of them
|
||||
— it should stay dangling and get dropped, same as any other unresolvable
|
||||
edge, rather than wire two unrelated files/languages together by accident."""
|
||||
root = tmp_path / "repo"
|
||||
root.mkdir()
|
||||
extraction = {
|
||||
"nodes": [
|
||||
# Ids given in their canonical (post-extract.py, extension-stripped)
|
||||
# form, matching what a real graphify update run would already have
|
||||
# produced before build_from_json assembles the final graph.
|
||||
{"id": "dev_monitoring_ping", "label": "ping.h", "file_type": "code",
|
||||
"source_file": "Dev/monitoring/ping.h"},
|
||||
{"id": "www_pages_api_ping", "label": "ping.php", "file_type": "code",
|
||||
"source_file": "www/pages/api/ping.php"},
|
||||
{"id": "dev_poker_server", "label": "server.cpp", "file_type": "code",
|
||||
"source_file": "Dev/poker/server.cpp"},
|
||||
],
|
||||
"edges": [
|
||||
# The unscoped, deliberately-unresolved fallback edge a C/C++ #include
|
||||
# resolver leaves behind when it can't find the header on disk.
|
||||
{"source": "dev_poker_server", "target": "ping", "relation": "imports",
|
||||
"confidence": "EXTRACTED", "source_file": "Dev/poker/server.cpp"},
|
||||
],
|
||||
}
|
||||
G = build_from_json(extraction, root=root)
|
||||
assert not G.has_edge("dev_poker_server", "dev_monitoring_ping")
|
||||
assert not G.has_edge("dev_poker_server", "www_pages_api_ping")
|
||||
|
||||
|
||||
def test_build_from_json_unambiguous_old_stem_alias_still_resolves(tmp_path):
|
||||
"""Companion to the ambiguous case above: when exactly one real file claims
|
||||
an old-stem alias, a dangling edge to that bare alias should still resolve
|
||||
to it — the #1504 migration-compat behavior this index exists for."""
|
||||
root = tmp_path / "repo"
|
||||
root.mkdir()
|
||||
extraction = {
|
||||
"nodes": [
|
||||
{"id": "dev_monitoring_utility", "label": "utility.h", "file_type": "code",
|
||||
"source_file": "Dev/monitoring/utility.h"},
|
||||
{"id": "dev_poker_server", "label": "server.cpp", "file_type": "code",
|
||||
"source_file": "Dev/poker/server.cpp"},
|
||||
],
|
||||
"edges": [
|
||||
{"source": "dev_poker_server", "target": "utility", "relation": "imports",
|
||||
"confidence": "EXTRACTED", "source_file": "Dev/poker/server.cpp"},
|
||||
],
|
||||
}
|
||||
G = build_from_json(extraction, root=root)
|
||||
assert G.has_edge("dev_poker_server", "dev_monitoring_utility")
|
||||
|
||||
|
||||
def test_build_from_json_relative_source_file_unchanged(tmp_path):
|
||||
"""Already-relative source_file paths must not be modified."""
|
||||
extraction = {
|
||||
|
||||
Reference in New Issue
Block a user