From a2dc3d9ce28080f6481890af17f8a4c34237d66f Mon Sep 17 00:00:00 2001 From: Rishet Mehra Date: Fri, 31 Jul 2026 09:25:19 +0530 Subject: [PATCH] fix(build): coerce non-string node ids so a numeric id cannot abort the build A backend can emit {"id": 10} where the schema says {"id": "10"}. Every id consumer downstream assumes str, so one numeric id aborted the whole run at the final merge step, after the full extraction had already been paid for. There are three crash sites, not the one in the report: - dedup._pick_winner's _CHUNK_SUFFIX.search(n["id"]) raises "expected string or bytes-like object, got 'int'" (the reported traceback), but only when the node lands in a duplicate or fuzzy group - build_from_json's sorted(node_set) raises "'<' not supported between instances of 'str' and 'int'" for a lone node with nothing to dedup - the same sort, reached via the fuzzy-group path A str() cast at the reported line therefore does not fix the common case: a single numeric-id node never reaches _pick_winner. Coerce at ingest instead, in _coerce_non_string_ids, called from build() before dedup (which keys on id) and from build_from_json (the direct entry used by cli.py, watch.py and diagnostics.py, which never goes through build()). The nested call on the build() path is an idempotent no-op. Edge endpoints and hyperedge members are coerced alongside the nodes on purpose: coercing node ids alone 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 aliases are included because dedup reads them directly (#803). Only bool-free numeric scalars are coerced. A None, list or dict id is left for validate_extraction to report, since str(None) == "None" would fabricate an id that no edge references. Closes #2326 --- graphify/build.py | 64 ++++++++++++++ tests/test_non_string_node_ids.py | 136 ++++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+) create mode 100644 tests/test_non_string_node_ids.py diff --git a/graphify/build.py b/graphify/build.py index 4ea30a45..75af8abd 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 00000000..f6f1d6c4 --- /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")