diff --git a/graphify/build.py b/graphify/build.py index 4ea30a45e..75af8abdc 100644 --- a/graphify/build.py +++ b/graphify/build.py @@ -157,6 +157,62 @@ def _fold_edge_aliases(edge: dict) -> None: edge["confidence"] = "INFERRED" +def _coerce_id(value: object) -> object: + """Return a str for a numeric id, else the value unchanged. + + ``bool`` is excluded despite subclassing ``int``: an id of ``True`` is not a + number the model meant to name a node, and ``"True"`` would invent a label. + """ + if isinstance(value, bool) or not isinstance(value, (int, float)): + return value + return str(value) + + +def _coerce_non_string_ids(extraction: dict) -> None: + """Coerce numeric node ids and edge/hyperedge references to str, in place (#2326). + + A backend can emit ``{"id": 10}`` where the schema says ``{"id": "10"}``. + Every id consumer downstream assumes ``str``, so one int id aborted the build + in three places: ``_pick_winner``'s ``_CHUNK_SUFFIX.search(n["id"])`` raised + ``TypeError: expected string or bytes-like object``, and ``build_from_json``'s + ``sorted(node_set)`` raised ``'<' not supported between instances of 'str' + and 'int'`` — the latter for a lone node with nothing to dedup at all. + Coercing keeps the node and its edges rather than dropping either, which is + the same tolerate-and-heal treatment loose backend output already gets at the + parse chokepoint (#1631) and in the alias folds (#2194). + + Endpoints and hyperedge members are coerced with the nodes, not after: a + node-only coercion would renumber ``10`` to ``"10"`` and leave every edge + pointing at the vanished ``10``, trading a loud crash for a silently + disconnected graph. The legacy ``from``/``to`` endpoint aliases are included + because dedup reads them directly (#803). + + Runs in BOTH ``build`` (before dedup, which keys on id) and + ``build_from_json`` (the direct entry that reloads a persisted graph), for + the same two-site reason as the ``_fold_node_aliases`` fold (#2194). It is + idempotent, so the nested call on the ``build`` path is a no-op. + + Non-numeric non-str ids (``None``, lists, dicts) are left alone for + ``validate_extraction`` to report: ``str(None) == "None"`` would fabricate a + node id that no edge references. + """ + for node in extraction.get("nodes") or (): + if isinstance(node, dict) and "id" in node: + node["id"] = _coerce_id(node["id"]) + for edge in extraction.get("edges") or (): + if not isinstance(edge, dict): + continue + for key in ("source", "target", "from", "to"): + if key in edge: + edge[key] = _coerce_id(edge[key]) + for he in extraction.get("hyperedges") or (): + if not isinstance(he, dict): + continue + members = he.get("nodes") + if isinstance(members, list): + he["nodes"] = [_coerce_id(ref) for ref in members] + + def _norm_source_file(p: str | None, root: str | None = None) -> str | None: """Normalize path separators and relativize absolute paths. @@ -549,6 +605,10 @@ def build_from_json(extraction: dict, *, directed: bool = False, root: str | Pat if "edges" not in extraction and "links" in extraction: extraction = dict(extraction, edges=extraction["links"]) + # Numeric ids from a loose backend become str before anything keys on them + # (#2326) — after the links remap so aliased edges are covered too. + _coerce_non_string_ids(extraction) + # Canonicalize legacy node/edge schema before validation. for node in extraction.get("nodes", []): if not isinstance(node, dict): @@ -1048,6 +1108,10 @@ def build( combined["input_tokens"] += ext.get("input_tokens", 0) combined["output_tokens"] += ext.get("output_tokens", 0) if dedup and combined["nodes"]: + # Numeric ids must be str before dedup, which keys on them and would + # raise TypeError in _pick_winner's regex search (#2326). build_from_json + # coerces too, but that runs after dedup — too late for this path. + _coerce_non_string_ids(combined) # Fold legacy node field aliases before dedup (#2194): dedup runs BEFORE # build_from_json and keys on `label`, so a `name`/`path` alias node # would be invisible to it and only label-dedup one build later, after diff --git a/tests/test_non_string_node_ids.py b/tests/test_non_string_node_ids.py new file mode 100644 index 000000000..f6f1d6c46 --- /dev/null +++ b/tests/test_non_string_node_ids.py @@ -0,0 +1,136 @@ +"""Non-string node ids from LLM backends must not crash the build (#2326). + +A backend can emit ``{"id": 10}`` where the schema says ``{"id": "10"}``. Every +id consumer downstream assumes ``str``, so an int id used to abort the whole +build in three different places. These tests pin the crash sites and the +edge/hyperedge linkage that a node-only coercion would silently break. +""" +import networkx as nx +import pytest + +from graphify.build import build, build_from_json + + +def _node(nid, label, **kw): + return { + "id": nid, + "label": label, + "file_type": "concept", + "source_file": "a.py", + **kw, + } + + +def _edge(src, tgt): + return {"source": src, "target": tgt, "relation": "uses", "confidence": "EXTRACTED"} + + +def test_pick_winner_survives_int_id_in_duplicate_group(): + """dedup._pick_winner regex-searched the raw id (the issue's traceback). + + Driven through ``build`` because that is dedup's only production caller, so + ``build`` is where the coercion has to land for this path to be fixed. + """ + ext = {"nodes": [_node(10, "Alpha"), _node("alpha_c1", "Alpha")], "edges": []} + G = build([ext], dedup=True) + assert all(isinstance(nid, str) for nid in G.nodes) + + +def test_build_accepts_a_single_int_id_node_with_no_duplicate(): + """build_from_json's sorted(node_set) crashed even with nothing to dedup.""" + ext = {"nodes": [_node(10, "Alpha"), _node("b", "Beta")], "edges": [_edge(10, "b")]} + G = build([ext], dedup=True) + assert "10" in G.nodes + assert 10 not in G.nodes + + +def test_int_id_endpoints_stay_connected_after_coercion(): + """Coercing node ids without coercing endpoints would orphan the edge.""" + ext = {"nodes": [_node(10, "Alpha"), _node(20, "Beta")], "edges": [_edge(10, 20)]} + G = build([ext], dedup=True) + assert G.has_edge("10", "20") + + +def test_int_id_survives_a_fuzzy_dedup_group(): + ext = { + "nodes": [_node(10, "PaymentProcessor"), _node("b", "PaymentProcessors")], + "edges": [_edge(10, "b")], + } + G = build([ext], dedup=True) + assert all(isinstance(nid, str) for nid in G.nodes) + + +def test_float_id_is_coerced_too(): + ext = {"nodes": [_node(1.5, "Alpha"), _node("b", "Beta")], "edges": [_edge(1.5, "b")]} + G = build([ext], dedup=True) + assert G.has_edge("1.5", "b") + + +def test_legacy_from_to_endpoints_are_coerced(): + """dedup reads the legacy from/to aliases (#803), so they need it as well.""" + ext = { + "nodes": [_node(10, "Alpha"), _node("b", "Beta")], + "edges": [{"from": 10, "to": "b", "relation": "uses", "confidence": "EXTRACTED"}], + } + G = build([ext], dedup=True) + assert G.has_edge("10", "b") + + +def test_hyperedge_members_are_coerced_with_their_nodes(): + ext = { + "nodes": [_node(10, "Alpha"), _node("b", "Beta")], + "edges": [], + "hyperedges": [{"id": "he1", "label": "grp", "nodes": [10, "b"]}], + } + G = build([ext], dedup=True) + members = G.graph["hyperedges"][0]["nodes"] + assert members == ["10", "b"] + + +def test_build_from_json_coerces_on_the_direct_entry(): + """Reloading a persisted graph does not go through build()/dedup.""" + G = build_from_json({"nodes": [_node(10, "Alpha")], "edges": []}) + assert list(G.nodes) == ["10"] + + +def test_numeric_endpoint_with_no_matching_node_matches_the_string_case(): + """A numeric endpoint with no node of its own must behave like a string one. + + Both are dangling references, which build_from_json drops — the point is that + coercion makes the int indistinguishable from the str, rather than crashing + or leaving a half-typed endpoint behind. + """ + def graph_for(target): + G = build_from_json( + {"nodes": [_node("a", "Alpha")], "edges": [_edge("a", target)]} + ) + return sorted(G.nodes), sorted(G.edges) + + assert graph_for(99) == graph_for("99") + + +@pytest.mark.parametrize("bad", [None, ["x"], {"k": "v"}]) +def test_non_scalar_ids_are_left_for_validation(bad): + """Only numeric scalars are coerced; str(None) == 'None' would be a lie.""" + from graphify.build import _coerce_non_string_ids + + ext = {"nodes": [{"id": bad, "label": "Alpha"}], "edges": []} + _coerce_non_string_ids(ext) + assert ext["nodes"][0]["id"] == bad + + +def test_bool_id_is_not_coerced(): + from graphify.build import _coerce_non_string_ids + + ext = {"nodes": [{"id": True, "label": "Alpha"}], "edges": []} + _coerce_non_string_ids(ext) + assert ext["nodes"][0]["id"] is True + + +def test_string_ids_are_untouched(): + """Regression guard: the normal path must be byte-identical.""" + ext = {"nodes": [_node("a", "Alpha"), _node("b", "Beta")], "edges": [_edge("a", "b")]} + G = build([ext], dedup=True) + assert isinstance(G, nx.Graph) + assert set(G.nodes) == {"a", "b"} + assert G.has_edge("a", "b")