Fix: validate_extraction crashes on non-hashable node id / edge endpoint
validate_extraction() is documented to return a list of error strings ("empty
list means valid"), but raised TypeError: unhashable type: 'list' when a node
id -- or an edge source/target -- was a non-hashable value such as a list. This
occurs in practice when an LLM extraction subagent emits malformed JSON like
{"id": ["foo", "bar"], ...}. Crash sites: the node_ids set comprehension and
the `edge[...] not in node_ids` membership tests.
Because build_from_json() validates at its start, a single malformed node
aborted the entire build, losing an otherwise-complete extraction of a large
corpus. build_from_json() itself would also raise (G.add_node(<list>) and the
`not in node_set` test) if the validator were bypassed.
- validate.py: build node_ids during the node pass, adding only hashable ids;
report a non-hashable id/endpoint as an error string instead of crashing.
All existing messages and the dangling-edge checks are preserved.
- build.py: skip dict nodes with a missing/non-hashable id and edges with
non-hashable endpoints (stderr warning). Non-dict nodes are deliberately
left to raise so the multigraph diagnostic still observes shape errors.
- tests: 3 cases in test_validate.py and 2 in test_build.py.
This commit is contained in:
dschwartzi
safishamsi
+32
-2
@@ -182,8 +182,25 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
print(f"[graphify] Extraction warning ({len(real_errors)} issues): {real_errors[0]}", file=sys.stderr)
|
||||
G: nx.Graph = nx.DiGraph() if directed else nx.Graph()
|
||||
for node in extraction.get("nodes", []):
|
||||
if "source_file" in node:
|
||||
node["source_file"] = _norm_source_file(node["source_file"], _root)
|
||||
# Skip dict nodes with a missing or non-hashable id (e.g. a list emitted
|
||||
# by a buggy LLM extraction) so NetworkX add_node never raises
|
||||
# TypeError: unhashable type. Non-dict nodes are deliberately left to
|
||||
# raise as before, so callers that probe build for shape errors (e.g.
|
||||
# the multigraph diagnostic) still observe the malformed shape.
|
||||
if isinstance(node, dict):
|
||||
if "id" not in node:
|
||||
continue
|
||||
try:
|
||||
hash(node["id"])
|
||||
except TypeError:
|
||||
print(
|
||||
f"[graphify] WARNING: skipping node with non-hashable id "
|
||||
f"{node['id']!r} (must be a string).",
|
||||
file=sys.stderr,
|
||||
)
|
||||
continue
|
||||
if "source_file" in node:
|
||||
node["source_file"] = _norm_source_file(node["source_file"], _root)
|
||||
G.add_node(node["id"], **{k: v for k, v in node.items() if k != "id"})
|
||||
node_set = set(G.nodes())
|
||||
|
||||
@@ -276,6 +293,19 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat
|
||||
if "source" not in edge or "target" not in edge:
|
||||
continue
|
||||
src, tgt = edge["source"], edge["target"]
|
||||
# Skip edges with non-hashable endpoints (e.g. a list emitted by a buggy
|
||||
# LLM extraction) so the `not in node_set` membership test below never
|
||||
# raises TypeError: unhashable type. The validator already reported these.
|
||||
try:
|
||||
hash(src)
|
||||
hash(tgt)
|
||||
except TypeError:
|
||||
print(
|
||||
f"[graphify] WARNING: skipping edge with non-hashable endpoint "
|
||||
f"(source={src!r}, target={tgt!r}).",
|
||||
file=sys.stderr,
|
||||
)
|
||||
continue
|
||||
# Remap mismatched IDs via normalization before dropping the edge.
|
||||
if src not in node_set:
|
||||
src = norm_to_id.get(_normalize_id(src), src)
|
||||
|
||||
+28
-5
@@ -17,6 +17,12 @@ def validate_extraction(data: dict) -> list[str]:
|
||||
|
||||
errors: list[str] = []
|
||||
|
||||
# Collected during the node pass so the edge pass can reuse it. Only
|
||||
# hashable ids land here; a non-hashable id (e.g. a list emitted by a
|
||||
# malformed LLM extraction) is reported as an error rather than crashing
|
||||
# the validator on set construction.
|
||||
node_ids: set = set()
|
||||
|
||||
# Nodes
|
||||
if "nodes" not in data:
|
||||
errors.append("Missing required key 'nodes'")
|
||||
@@ -30,6 +36,15 @@ def validate_extraction(data: dict) -> list[str]:
|
||||
for field in REQUIRED_NODE_FIELDS:
|
||||
if field not in node:
|
||||
errors.append(f"Node {i} (id={node.get('id', '?')!r}) missing required field '{field}'")
|
||||
if "id" in node:
|
||||
try:
|
||||
hash(node["id"])
|
||||
except TypeError:
|
||||
errors.append(
|
||||
f"Node {i} has non-hashable id {node['id']!r} - id must be a string"
|
||||
)
|
||||
else:
|
||||
node_ids.add(node["id"])
|
||||
if "file_type" in node and node["file_type"] not in VALID_FILE_TYPES:
|
||||
errors.append(
|
||||
f"Node {i} (id={node.get('id', '?')!r}) has invalid file_type "
|
||||
@@ -43,7 +58,6 @@ def validate_extraction(data: dict) -> list[str]:
|
||||
elif not isinstance(edge_list, list):
|
||||
errors.append("'edges' must be a list")
|
||||
else:
|
||||
node_ids = {n["id"] for n in data.get("nodes", []) if isinstance(n, dict) and "id" in n}
|
||||
for i, edge in enumerate(edge_list):
|
||||
if not isinstance(edge, dict):
|
||||
errors.append(f"Edge {i} must be an object")
|
||||
@@ -56,10 +70,19 @@ def validate_extraction(data: dict) -> list[str]:
|
||||
f"Edge {i} has invalid confidence '{edge['confidence']}' "
|
||||
f"- must be one of {sorted(VALID_CONFIDENCES)}"
|
||||
)
|
||||
if "source" in edge and node_ids and edge["source"] not in node_ids:
|
||||
errors.append(f"Edge {i} source '{edge['source']}' does not match any node id")
|
||||
if "target" in edge and node_ids and edge["target"] not in node_ids:
|
||||
errors.append(f"Edge {i} target '{edge['target']}' does not match any node id")
|
||||
for endpoint in ("source", "target"):
|
||||
if endpoint not in edge:
|
||||
continue
|
||||
val = edge[endpoint]
|
||||
try:
|
||||
unmatched = bool(node_ids) and val not in node_ids
|
||||
except TypeError:
|
||||
errors.append(
|
||||
f"Edge {i} {endpoint} {val!r} is non-hashable - must be a string"
|
||||
)
|
||||
continue
|
||||
if unmatched:
|
||||
errors.append(f"Edge {i} {endpoint} '{val}' does not match any node id")
|
||||
|
||||
return errors
|
||||
|
||||
|
||||
@@ -677,3 +677,40 @@ def test_build_merge_rejects_oversized_existing_graph(monkeypatch, tmp_path):
|
||||
monkeypatch.setattr("graphify.security._MAX_GRAPH_FILE_BYTES", 8)
|
||||
with pytest.raises(ValueError, match="exceeds"):
|
||||
build_merge([], graph_path, dedup=False)
|
||||
|
||||
|
||||
def test_build_from_json_skips_non_hashable_node_id():
|
||||
# A malformed LLM extraction can emit a list-valued id; build_from_json must
|
||||
# skip it (NetworkX add_node would otherwise raise unhashable type) and still
|
||||
# build the graph from the well-formed nodes.
|
||||
extraction = {
|
||||
"nodes": [
|
||||
{"id": "a", "label": "A", "file_type": "code", "source_file": "a.py"},
|
||||
{"id": ["x", "y"], "label": "B", "file_type": "code", "source_file": "b.py"},
|
||||
{"label": "C", "file_type": "code", "source_file": "c.py"}, # missing id
|
||||
],
|
||||
"edges": [],
|
||||
}
|
||||
G = build_from_json(extraction)
|
||||
assert set(G.nodes()) == {"a"}
|
||||
|
||||
|
||||
def test_build_from_json_skips_edge_with_non_hashable_endpoint():
|
||||
# A list-valued edge endpoint must be skipped rather than crash the
|
||||
# `not in node_set` membership test. The well-formed edge survives.
|
||||
extraction = {
|
||||
"nodes": [
|
||||
{"id": "a", "label": "A", "file_type": "code", "source_file": "a.py"},
|
||||
{"id": "b", "label": "B", "file_type": "code", "source_file": "b.py"},
|
||||
],
|
||||
"edges": [
|
||||
{"source": "a", "target": ["b", "c"], "relation": "calls",
|
||||
"confidence": "INFERRED", "source_file": "a.py"},
|
||||
{"source": "a", "target": "b", "relation": "imports",
|
||||
"confidence": "EXTRACTED", "source_file": "a.py"},
|
||||
],
|
||||
}
|
||||
G = build_from_json(extraction)
|
||||
assert G.number_of_nodes() == 2
|
||||
assert G.number_of_edges() == 1
|
||||
assert G.has_edge("a", "b")
|
||||
|
||||
@@ -85,3 +85,53 @@ def test_assert_valid_raises_on_errors():
|
||||
|
||||
def test_assert_valid_passes_silently():
|
||||
assert_valid(VALID) # should not raise
|
||||
|
||||
|
||||
def test_non_hashable_node_id_reported_not_raised():
|
||||
# A malformed LLM extraction can emit a list-valued id. The validator must
|
||||
# report it as an error string (its documented contract) rather than crash
|
||||
# with TypeError: unhashable type on set construction.
|
||||
data = {
|
||||
"nodes": [
|
||||
{"id": "n1", "label": "A", "file_type": "code", "source_file": "a.py"},
|
||||
{"id": ["x", "y"], "label": "B", "file_type": "code", "source_file": "b.py"},
|
||||
],
|
||||
"edges": [],
|
||||
}
|
||||
errors = validate_extraction(data)
|
||||
assert any("non-hashable id" in e for e in errors)
|
||||
|
||||
|
||||
def test_non_hashable_edge_endpoint_reported_not_raised():
|
||||
# A list-valued edge source/target must be reported, not crash the
|
||||
# membership test against the node-id set.
|
||||
data = {
|
||||
"nodes": [
|
||||
{"id": "n1", "label": "A", "file_type": "code", "source_file": "a.py"},
|
||||
{"id": "n2", "label": "B", "file_type": "code", "source_file": "b.py"},
|
||||
],
|
||||
"edges": [
|
||||
{"source": "n1", "target": ["n2", "n3"], "relation": "calls",
|
||||
"confidence": "INFERRED", "source_file": "a.py"},
|
||||
],
|
||||
}
|
||||
errors = validate_extraction(data)
|
||||
assert any("target" in e and "non-hashable" in e for e in errors)
|
||||
|
||||
|
||||
def test_non_hashable_node_id_does_not_mask_valid_ids():
|
||||
# The valid node id must still be collected so a legitimately-dangling edge
|
||||
# is still flagged even when a sibling node has a bad id.
|
||||
data = {
|
||||
"nodes": [
|
||||
{"id": "n1", "label": "A", "file_type": "code", "source_file": "a.py"},
|
||||
{"id": {"oops": 1}, "label": "B", "file_type": "code", "source_file": "b.py"},
|
||||
],
|
||||
"edges": [
|
||||
{"source": "n1", "target": "ghost", "relation": "calls",
|
||||
"confidence": "EXTRACTED", "source_file": "a.py"},
|
||||
],
|
||||
}
|
||||
errors = validate_extraction(data)
|
||||
assert any("non-hashable id" in e for e in errors)
|
||||
assert any("target" in e and "ghost" in e for e in errors)
|
||||
|
||||
Reference in New Issue
Block a user