From 8883991c2eab56c9b98e7a2b1c6f7de9eccd9493 Mon Sep 17 00:00:00 2001 From: Safi Date: Thu, 18 Jun 2026 22:55:44 +0100 Subject: [PATCH] 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 Co-Authored-By: Claude Opus 4.8 (1M context) --- graphify/__main__.py | 10 +- graphify/build.py | 15 +-- graphify/extract.py | 17 ++-- graphify/ids.py | 50 ++++++++++ graphify/mcp_ingest.py | 9 +- graphify/symbol_resolution.py | 13 +-- tests/test_extraction_spec_ids.py | 95 +++++++++++++++++++ tests/test_id_normalization_contract.py | 119 ++++++++++++++++++++++++ 8 files changed, 291 insertions(+), 37 deletions(-) create mode 100644 graphify/ids.py create mode 100644 tests/test_extraction_spec_ids.py create mode 100644 tests/test_id_normalization_contract.py diff --git a/graphify/__main__.py b/graphify/__main__.py index 3bc27c3..f4d1697 100644 --- a/graphify/__main__.py +++ b/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") diff --git a/graphify/build.py b/graphify/build.py index aa4cfff..b33f9e6 100644 --- a/graphify/build.py +++ b/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. diff --git a/graphify/extract.py b/graphify/extract.py index e72afdf..e6de255 100644 --- a/graphify/extract.py +++ b/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: diff --git a/graphify/ids.py b/graphify/ids.py new file mode 100644 index 0000000..0740d0a --- /dev/null +++ b/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)) diff --git a/graphify/mcp_ingest.py b/graphify/mcp_ingest.py index 1879dcc..3c5dbad 100644 --- a/graphify/mcp_ingest.py +++ b/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: diff --git a/graphify/symbol_resolution.py b/graphify/symbol_resolution.py index 39cc60f..67ab6b0 100644 --- a/graphify/symbol_resolution.py +++ b/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: diff --git a/tests/test_extraction_spec_ids.py b/tests/test_extraction_spec_ids.py new file mode 100644 index 0000000..13c6ecd --- /dev/null +++ b/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 diff --git a/tests/test_id_normalization_contract.py b/tests/test_id_normalization_contract.py new file mode 100644 index 0000000..9da1da9 --- /dev/null +++ b/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