Unify node-ID normalization into a single source of truth (#1378)

extract._make_id and build._normalize_id were copy-pasted forks of the same
NFKC/casefold recipe kept in sync by mirrored docstrings -- the root of the
recurring ID-drift ghost-node bug class (#811/#550/#1033/#1104). Move the recipe
to graphify/ids.py and have all four producers delegate to it: extract, build,
and (completing the migration) mcp_ingest and symbol_resolution, whose
"avoid an import cycle" copies are moot now that ids.py is dependency-free. The
contract test asserts all four resolve to the shared recipe, with hypothesis
property tests for make_id == normalize_id and idempotency.

Co-Authored-By: danielnguyenfinhub <danielnguyenfinhub@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

graphify/__main__.py

@@ -3524,10 +3524,18 @@ def main() -> None:
             except TypeError:
                 G = _jg.node_link_graph(data)
             graphs.append(G)
+        # nx.compose requires all graphs to be the same type.  When input graphs
+        # come from different sources (e.g. an AST-only run vs a full LLM run) one
+        # may be a MultiGraph and another a Graph.  Normalise everything to Graph
+        # (the graphify default) by converting MultiGraphs with nx.Graph().
+        def _to_simple(g: "_nx.Graph") -> "_nx.Graph":
+            if isinstance(g, _nx.MultiGraph):
+                return _nx.Graph(g)
+            return g
         merged = _nx.Graph()
         for G, gp in zip(graphs, graph_paths):
             repo_tag = gp.parent.parent.name  # graphify-out/../ → repo dir name
-            prefixed = _prefix(G, repo_tag)
+            prefixed = _to_simple(_prefix(G, repo_tag))
             merged = _nx.compose(merged, prefixed)
         try:
             out_data = _jg.node_link_data(merged, edges="links")

graphify/build.py

@@ -28,6 +28,7 @@ import sys
 import unicodedata
 from pathlib import Path
 import networkx as nx
+from .ids import normalize_id as _normalize_id
 from .validate import validate_extraction
 
 
@@ -51,20 +52,6 @@ _FILE_TYPE_SYNONYMS = {
 }
 
 
-def _normalize_id(s: str) -> str:
-    r"""Normalize an ID string the same way extract._make_id does.
-
-    Used to reconcile edge endpoints when the LLM generates IDs with slightly
-    different punctuation or casing than the AST extractor. Must stay in sync
-    with extract._make_id — NFKC normalization, \w with re.UNICODE, underscore
-    collapse, and casefold must all match (#811).
-    """
-    s = unicodedata.normalize("NFKC", s)
-    cleaned = re.sub(r"[^\w]+", "_", s, flags=re.UNICODE)
-    cleaned = re.sub(r"_+", "_", cleaned)
-    return cleaned.strip("_").casefold()
-
-
 def _norm_source_file(p: str | None, root: str | None = None) -> str | None:
     """Normalize path separators and relativize absolute paths.
 

graphify/extract.py

@@ -6,12 +6,12 @@ import json
 import os
 import re
 import sys
-import unicodedata
 from dataclasses import dataclass, field
 from pathlib import Path
 from typing import Any, Callable
 
 from .cache import load_cached, save_cached
+from .ids import make_id
 from .mcp_ingest import extract_mcp_config, is_mcp_config_path
 
 _RECURSION_LIMIT = 10_000
@@ -65,17 +65,14 @@ def _safe_extract(extractor: Callable, path: Path) -> dict:
 def _make_id(*parts: str) -> str:
     r"""Build a stable node ID from one or more name parts.
 
+    Thin wrapper over :func:`graphify.ids.make_id`, the single source of truth
+    shared with ``build._normalize_id`` so the two can no longer drift (#811).
     Preserves Unicode letters/digits (CJK, Cyrillic, Arabic, accented Latin,
-    etc.) so non-ASCII identifiers produce distinct IDs and don't collapse to
-    a single per-file node (#811). NFKC normalization ensures composed and
-    decomposed forms of the same character (e.g. é vs e+combining-acute)
-    produce the same ID. Must stay in sync with build._normalize_id.
+    etc.) so non-ASCII identifiers produce distinct IDs and don't collapse to a
+    single per-file node; NFKC normalization collapses composed/decomposed forms
+    of the same character (e.g. é vs e+combining-acute) to one ID.
     """
-    combined = "_".join(p.strip("_.") for p in parts if p)
-    combined = unicodedata.normalize("NFKC", combined)
-    cleaned = re.sub(r"[^\w]+", "_", combined, flags=re.UNICODE)
-    cleaned = re.sub(r"_+", "_", cleaned)
-    return cleaned.strip("_").casefold()
+    return make_id(*parts)
 
 
 def _file_stem(path: Path) -> str:

graphify/ids.py

@@ -0,0 +1,50 @@
+"""Single source of truth for node-ID normalization.
+
+Three independent producers must agree on node IDs or the graph splits a single
+entity into disconnected ghost nodes:
+
+1. The AST extractor (``extract._make_id``) — deterministic, per-language.
+2. The semantic subagents (LLM) — follow the node-ID spec in the skill prompt.
+3. The graph builder (``build._normalize_id``) — reconciles edge endpoints when
+   the LLM emits IDs with slightly different punctuation or casing than the AST.
+
+Historically the normalization recipe was copy-pasted into ``extract._make_id``
+and ``build._normalize_id`` and kept in sync only by mirrored docstrings, which
+is exactly how the recurring ID-drift bug class crept in (#811 Unicode collapse,
+#550 same-filename collisions, #1033 AST-vs-LLM file-node mismatch, #1104). This
+module exists so the recipe lives in one place and the two callers can no longer
+diverge.
+
+The recipe: NFKC-normalize (so composed/decomposed Unicode forms collapse),
+replace runs of non-word characters with a single underscore (``re.UNICODE`` so
+CJK/Cyrillic/Arabic/accented-Latin letters survive instead of collapsing to a
+per-file node), collapse repeated underscores, strip leading/trailing
+underscores, and casefold.
+"""
+from __future__ import annotations
+
+import re
+import unicodedata
+
+__all__ = ["normalize_id", "make_id"]
+
+
+def normalize_id(s: str) -> str:
+    r"""Normalize a single ID string to its canonical form.
+
+    Idempotent: ``normalize_id(normalize_id(s)) == normalize_id(s)``.
+    """
+    s = unicodedata.normalize("NFKC", s)
+    s = re.sub(r"[^\w]+", "_", s, flags=re.UNICODE)
+    s = re.sub(r"_+", "_", s)
+    return s.strip("_").casefold()
+
+
+def make_id(*parts: str) -> str:
+    """Build a canonical node ID from one or more name parts.
+
+    Parts are joined with ``_`` (after stripping stray ``_``/``.`` edges from each
+    part) and then run through :func:`normalize_id`, so the result is identical to
+    what the builder produces from the joined string.
+    """
+    return normalize_id("_".join(p.strip("_.") for p in parts if p))

graphify/mcp_ingest.py

@@ -63,6 +63,7 @@ import unicodedata
 from pathlib import Path
 from typing import Any
 
+from graphify.ids import make_id as _shared_make_id
 from graphify.security import sanitize_label
 
 
@@ -376,12 +377,8 @@ def _add_edge(
 
 
 def _make_id(*parts: str) -> str:
-    """Build a stable node ID. Must match extract._make_id's normalisation rules."""
-    combined = "_".join(p.strip("_.") for p in parts if p)
-    combined = unicodedata.normalize("NFKC", combined)
-    cleaned = re.sub(r"[^\w]+", "_", combined, flags=re.UNICODE)
-    cleaned = re.sub(r"_+", "_", cleaned)
-    return cleaned.strip("_").casefold()
+    """Build a stable node ID via the single shared recipe (#1378)."""
+    return _shared_make_id(*parts)
 
 
 def _file_stem(path: Path) -> str:

graphify/symbol_resolution.py

@@ -10,6 +10,7 @@ from pathlib import Path
 from collections.abc import Sequence
 from typing import Any
 
+from graphify.ids import make_id as _shared_make_id
 from graphify.security import sanitize_metadata
 
 
@@ -357,12 +358,12 @@ def resolve_cross_file_raw_calls(
 
 
 def _bash_make_id(*parts: str) -> str:
-    """Exact copy of extract._make_id — kept here to avoid an import cycle."""
-    combined = "_".join(p.strip("_.") for p in parts if p)
-    combined = unicodedata.normalize("NFKC", combined)
-    cleaned = re.sub(r"[^\w]+", "_", combined, flags=re.UNICODE)
-    cleaned = re.sub(r"_+", "_", cleaned)
-    return cleaned.strip("_").casefold()
+    """Bash symbol node ID via the single shared recipe (#1378).
+
+    Previously an inline copy to dodge an import cycle; ``graphify.ids`` is
+    dependency-free, so it can be imported directly.
+    """
+    return _shared_make_id(*parts)
 
 
 def _bash_file_stem(rel_path: Path) -> str:

tests/test_extraction_spec_ids.py

@@ -0,0 +1,95 @@
+"""Drift guard for the node-ID spec shown to LLM semantic subagents.
+
+`tools/skillgen/fragments/references/shared/extraction-spec.md` (and the per-host
+copies skillgen renders from it) tell the subagents how to build node IDs, with
+worked examples like::
+
+    `src/auth/session.py` + `ValidateToken` → `auth_session_validatetoken`
+
+Those examples are the LLM's ground truth, and they must reproduce exactly what
+the AST extractor emits (`extract._file_stem` + `extract._make_id`) or the two
+producers generate different IDs for the same symbol and the graph splits it into
+disconnected ghost nodes — the bug class the prose itself warns about
+(#811/#550/#1033/#1104).
+
+The prose is hand-maintained, so it can silently drift from the code. This test
+parses every example straight out of the shipped specs and asserts the real
+functions reproduce each one. It fails if the spec examples are edited to a wrong
+value, OR if the ID functions change so the documented examples no longer hold.
+"""
+import re
+from pathlib import Path
+
+import pytest
+
+from graphify.extract import _file_stem, _make_id
+
+REPO_ROOT = Path(__file__).resolve().parents[1]
+
+# `path` + `entity` → `id`  (arrow is U+2192). Backtick-delimited so prose around
+# the examples never leaks in.
+_EXAMPLE_RE = re.compile(r"`([^`]+)`\s*\+\s*`([^`]+)`\s*→\s*`([^`]+)`")
+
+
+def _spec_files() -> list[Path]:
+    roots = [REPO_ROOT / "graphify" / "skills", REPO_ROOT / "tools" / "skillgen" / "fragments"]
+    files: list[Path] = []
+    for root in roots:
+        for p in root.rglob("extraction-spec.md"):
+            # build/ is a packaging artifact; expected/ is skillgen's own golden
+            # output and is already covered by `skillgen --check`.
+            if "/build/" in p.as_posix() or "/expected/" in p.as_posix():
+                continue
+            files.append(p)
+    return sorted(files)
+
+
+def _examples() -> list[tuple[Path, str, str, str]]:
+    out: list[tuple[Path, str, str, str]] = []
+    for f in _spec_files():
+        text = f.read_text(encoding="utf-8")
+        for path, entity, expected in _EXAMPLE_RE.findall(text):
+            out.append((f, path, entity, expected))
+    return out
+
+
+def _ast_symbol_id(path: str, entity: str) -> str:
+    """Reproduce the symbol ID the AST extractor emits for a file + symbol, using
+    the real production helpers (not a re-implementation)."""
+    return _make_id(_file_stem(Path(path)), entity)
+
+
+def test_spec_files_are_discoverable():
+    """Guard the guard: if the spec moves or the example format changes so nothing
+    parses, fail loudly rather than passing vacuously."""
+    files = _spec_files()
+    assert files, "no extraction-spec.md files found — did the skills tree move?"
+    examples = _examples()
+    assert len(examples) >= 13, (
+        f"expected the documented node-ID examples across host specs, parsed only "
+        f"{len(examples)} — the example format may have changed"
+    )
+
+
+@pytest.mark.parametrize(
+    "path,entity,expected",
+    [(p, e, x) for (_f, p, e, x) in _examples()],
+    ids=[f"{f.parent.parent.name}:{p}+{e}" for (f, p, e, x) in _examples()],
+)
+def test_spec_node_id_examples_match_ast_extractor(path, entity, expected):
+    got = _ast_symbol_id(path, entity)
+    assert got == expected, (
+        f"node-ID spec drift: spec says `{path}` + `{entity}` → `{expected}`, but "
+        f"extract._make_id(_file_stem(...), ...) produces `{got}`. Update the spec "
+        f"examples and the ID functions together."
+    )
+
+
+def test_cautionary_wrong_forms_are_actually_wrong():
+    """The canonical spec warns against the filename-only and full-path ID forms.
+    Lock those anti-examples to the code too, so the warning can't go stale."""
+    correct = _ast_symbol_id("src/auth/session.py", "ValidateToken")
+    assert correct == "auth_session_validatetoken"
+    # filename-only (drops the parent dir) and full-path (keeps every segment)
+    assert _make_id("session", "ValidateToken") != correct
+    assert _make_id("src", "auth", "session", "ValidateToken") != correct

tests/test_id_normalization_contract.py

@@ -0,0 +1,119 @@
+"""Drift guard for the node-ID normalization contract.
+
+Three independent producers must agree on node IDs or the graph splits one entity
+into disconnected ghost nodes: the AST extractor (``extract._make_id``), the
+semantic subagents (the skill prompt's node-ID spec), and the graph builder
+(``build._normalize_id``, which reconciles edge endpoints). The recipe used to be
+copy-pasted into ``_make_id`` and ``_normalize_id`` and kept in sync only by
+mirrored docstrings — exactly how the recurring ID-drift bug class crept in
+(#811 Unicode collapse, #550 same-filename collisions, #1033 AST-vs-LLM file-node
+mismatch, #1104).
+
+Both callers now delegate to :mod:`graphify.ids`, so they share one
+implementation and cannot diverge. These tests lock that contract: if a future
+change re-forks the normalization (a new local helper, an inlined regex, a
+dropped ``casefold``), they fail.
+"""
+import re
+
+import pytest
+
+from graphify.build import _normalize_id
+from graphify.extract import _make_id
+from graphify.ids import make_id, normalize_id
+
+# Inputs that previously diverged or are easy to get wrong. The single-part form
+# of `_make_id` must equal `_normalize_id` for every one of these.
+CONTRACT_CASES = [
+    "Session_ValidateToken",      # casing
+    "session.validate-token",     # punctuation -> underscore
+    "foo__bar..baz",              # repeated separators collapse
+    "  Leading_Trailing__  ",     # strip stray underscores/space
+    "A/B\\C",                     # path separators both directions
+    "MixedCASE",                  # #811: casefold
+    "café",                       # composed accented Latin (NFKC)
+    "café",                 # decomposed e + combining acute -> same as 'café'
+    "日本語クラス",                  # #811: CJK letters survive, not collapsed
+    "Кириллица",                  # Cyrillic survives
+    "naïve_Über",                 # mixed accented Latin
+    "x_c1",                       # must NOT be treated as a chunk suffix here
+    "__dunder__",                 # leading/trailing underscores stripped
+    "tab\tnewline\nspace ",       # whitespace runs -> single underscore
+]
+
+
+@pytest.mark.parametrize("raw", CONTRACT_CASES)
+def test_make_id_matches_normalize_id(raw):
+    """The AST id-maker and the builder's reconciler must agree, char for char."""
+    assert _make_id(raw) == _normalize_id(raw), (
+        f"ID drift for {raw!r}: extract._make_id -> {_make_id(raw)!r} but "
+        f"build._normalize_id -> {_normalize_id(raw)!r}"
+    )
+
+
+@pytest.mark.parametrize("raw", CONTRACT_CASES)
+def test_normalize_id_is_idempotent(raw):
+    once = normalize_id(raw)
+    assert normalize_id(once) == once, f"normalize_id not idempotent for {raw!r}"
+
+
+def test_make_id_joins_then_normalizes():
+    """Multi-part make_id == normalize_id of the joined parts (the builder only
+    ever sees the joined string, so these must coincide)."""
+    parts = ("auth", "session.py", "ValidateToken")
+    assert make_id(*parts) == normalize_id("_".join(parts))
+    # Documented spec example.
+    assert make_id("src/auth/session.py".split("/")[-2], "session", "ValidateToken") == \
+        "auth_session_validatetoken"
+
+
+def test_unicode_identifiers_do_not_collapse_to_empty():
+    """#811: non-ASCII identifiers must yield distinct, non-empty IDs rather than
+    collapsing to a single per-file node."""
+    a = _make_id("クラスА")
+    b = _make_id("クラスB")
+    assert a and b and a != b
+
+
+def test_normalized_ids_are_safe_node_ids():
+    """Output is lowercase and contains no path/punctuation separators."""
+    for raw in CONTRACT_CASES:
+        out = normalize_id(raw)
+        assert out == out.casefold()
+        assert not re.search(r"[./\\\s]", out), f"unsafe char in id {out!r}"
+        assert not out.startswith("_") and not out.endswith("_")
+
+
+def test_both_callers_share_one_implementation():
+    """Guard against re-forking: the two public callers must resolve to the same
+    underlying function object as graphify.ids.normalize_id."""
+    # build._normalize_id is imported directly from graphify.ids.
+    assert _normalize_id is normalize_id
+    # extract._make_id wraps make_id; prove it round-trips through the shared core.
+    assert _make_id("Foo.Bar") == normalize_id("Foo.Bar")
+    # The other two live ID producers — MCP config ingestion and bash symbol
+    # resolution — must also resolve to the shared recipe, or the "single source
+    # of truth" leaks back into copy-pasted forks (#1378).
+    from graphify.mcp_ingest import _make_id as _mcp_make_id
+    from graphify.symbol_resolution import _bash_make_id
+    for fn in (_make_id, _mcp_make_id, _bash_make_id):
+        assert fn("Foo.Bar", "baz") == make_id("Foo.Bar", "baz")
+        assert fn("Ångström", "Ⅳ") == make_id("Ångström", "Ⅳ")
+
+
+# Optional property-based fuzzing — hypothesis is a dev dependency. Skip cleanly
+# if it is unavailable so the deterministic cases above still run everywhere.
+hypothesis = pytest.importorskip("hypothesis")
+from hypothesis import given  # noqa: E402
+from hypothesis import strategies as st  # noqa: E402
+
+
+@given(st.text())
+def test_property_make_id_equals_normalize_id(s):
+    assert _make_id(s) == _normalize_id(s)
+
+
+@given(st.text())
+def test_property_normalize_id_idempotent(s):
+    once = normalize_id(s)
+    assert normalize_id(once) == once