From f64e4a2c800d242ea63e53f6126f9c812c34741c Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Fri, 17 Jul 2026 11:42:51 +0700 Subject: [PATCH 1/2] feat(diff): compare call-graph edges in snapshot diff (closes #297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Snapshots stored 25,876 edges on disk but _diff_backend() only ever compared nodes, so any structural change that neither added nor removed a function was invisible. Edge identity is (file, impl_for, fn) per endpoint, not the raw node id. Node ids embed a line number, so id-keyed edges reported every edge of a function that merely shifted lines as removed and re-added: on the real 425-file polyglot workspace a whole-codebase line shift produced 3,178 false reports from zero real changes. Keying through the node map drops that to 0 while still detecting genuine edge additions. Resolved edges only. 82% of real edges are unresolved stdlib calls (append, strip, get) and would drown the signal, so they are tallied rather than enumerated. Pairs form a set: one edge is recorded per call site, and call-site count is not graph shape. via_self is a qualifier and stays out of identity. Detail lists are capped at 100 with a truncated flag; counts stay exact. Legacy node fields are untouched — commands/diff.py, dashboard, formatters and MCP read them. Co-Authored-By: Claude Opus 4.8 --- scripts/diff_engine.py | 109 ++++++++++++- tests/test_diff_engine_edges.py | 278 ++++++++++++++++++++++++++++++++ 2 files changed, 383 insertions(+), 4 deletions(-) create mode 100644 tests/test_diff_engine_edges.py diff --git a/scripts/diff_engine.py b/scripts/diff_engine.py index e60be5a3..bc2a975d 100755 --- a/scripts/diff_engine.py +++ b/scripts/diff_engine.py @@ -8,12 +8,16 @@ import os import copy from datetime import datetime, timezone -from typing import Dict, List, Any, Optional, Tuple +from typing import Dict, List, Any, Optional, Set, Tuple from utils import logger SNAPSHOTS_DIR = ".codelens/snapshots" +# Cap on per-edge detail entries. Counts stay exact; a file rename can shift +# thousands of pairs at once, which would otherwise flood the output. +EDGE_DETAIL_CAP = 100 + def save_snapshot(workspace: str, frontend: Dict, backend: Dict) -> str: """ @@ -123,7 +127,9 @@ def diff_snapshots( "changed": frontend_diff["changed_count"] + backend_diff["changed_count"], "new_collisions": len(frontend_diff.get("new_collisions", [])), "new_dead": len(frontend_diff.get("new_dead", [])) + len(backend_diff.get("new_dead", [])), - "resolved_dead": len(frontend_diff.get("resolved_dead", [])) + len(backend_diff.get("resolved_dead", [])) + "resolved_dead": len(frontend_diff.get("resolved_dead", [])) + len(backend_diff.get("resolved_dead", [])), + "edges_added": backend_diff.get("added_edge_count", 0), + "edges_removed": backend_diff.get("removed_edge_count", 0) } return { @@ -169,7 +175,9 @@ def diff_current_vs_last(workspace: str) -> Dict[str, Any]: "changed": frontend_diff["changed_count"] + backend_diff["changed_count"], "new_collisions": len(frontend_diff.get("new_collisions", [])), "new_dead": len(frontend_diff.get("new_dead", [])) + len(backend_diff.get("new_dead", [])), - "resolved_dead": len(frontend_diff.get("resolved_dead", [])) + len(backend_diff.get("resolved_dead", [])) + "resolved_dead": len(frontend_diff.get("resolved_dead", [])) + len(backend_diff.get("resolved_dead", [])), + "edges_added": backend_diff.get("added_edge_count", 0), + "edges_removed": backend_diff.get("removed_edge_count", 0) } return { @@ -300,6 +308,79 @@ def _diff_frontend(old: Dict, new: Dict) -> Dict: } +def _endpoint_key(node_id: str, node_map: Dict) -> Tuple[str, str, str]: + """ + Line-independent identity for an edge endpoint: (file, owner, fn). + + Node ids embed a line number (`engine.py:167`), so keying edges by raw id + reports every edge of a function that merely shifted lines as removed and + re-added. Resolving through the node map to (file, impl_for, fn) drops that + churn. `impl_for` (the owning class/struct) separates same-named methods + that share a file. Ids that resolve to no node — module-level synthetic + sources like `app.py:0:` deliberately carry no node — fall back to + the id itself, which is already line-independent. + """ + node = node_map.get(node_id) + if not node: + return ("", "", node_id) + # Normalise to strings: a missing owner must sort against a present one. + return ( + node.get("file") or "", + node.get("impl_for") or "", + node.get("fn") or node_id + ) + + +def _split_edges( + registry: Dict, + node_map: Dict +) -> Tuple[Set[Tuple[Tuple, Tuple]], int]: + """ + Split a backend registry's edges into resolved call pairs and an + unresolved tally. + + Resolved edges carry `to` (a node id); unresolved ones carry only `to_fn` + (a bare name, overwhelmingly stdlib/builtin like `append` or `strip`) and + are counted rather than enumerated. Pairs form a set, not a multiset: the + same caller/callee is recorded once per call site, and call-site count is + not graph shape. `via_self` is a qualifier, so it stays out of identity. + """ + resolved: Set[Tuple[Tuple, Tuple]] = set() + unresolved = 0 + + for edge in registry.get("edges", []): + if "to" in edge: + resolved.add(( + _endpoint_key(edge.get("from"), node_map), + _endpoint_key(edge["to"], node_map) + )) + elif "to_fn" in edge: + unresolved += 1 + + return resolved, unresolved + + +def _endpoint_label(key: Tuple[str, str, str]) -> str: + """Render an endpoint key as `Owner.fn`, or just `fn` when unowned.""" + _file, owner, fn = key + return f"{owner}.{fn}" if owner else fn + + +def _format_edges(pairs: Set[Tuple[Tuple, Tuple]]) -> Tuple[List[Dict[str, str]], bool]: + """Render edge pairs as capped, deterministically ordered detail entries.""" + ordered = sorted(pairs) + detail = [ + { + "from": _endpoint_label(src), + "from_file": src[0], + "to": _endpoint_label(dst), + "to_file": dst[0] + } + for src, dst in ordered[:EDGE_DETAIL_CAP] + ] + return detail, len(ordered) > EDGE_DETAIL_CAP + + def _diff_backend(old: Dict, new: Dict) -> Dict: """Diff two backend registries.""" old_nodes = {n["id"]: n for n in old.get("nodes", [])} @@ -341,6 +422,15 @@ def _diff_backend(old: Dict, new: Dict) -> Dict: if changes: changed_nodes.append({"name": new_n["fn"], "file": new_n.get("file", ""), **changes}) + old_edges, old_unresolved = _split_edges(old, old_nodes) + new_edges, new_unresolved = _split_edges(new, new_nodes) + + added_edge_pairs = new_edges - old_edges + removed_edge_pairs = old_edges - new_edges + + added_edges, added_truncated = _format_edges(added_edge_pairs) + removed_edges, removed_truncated = _format_edges(removed_edge_pairs) + return { "added_nodes": added_nodes, "removed_nodes": removed_nodes, @@ -349,7 +439,18 @@ def _diff_backend(old: Dict, new: Dict) -> Dict: "removed_count": len(removed_nodes), "changed_count": len(changed_nodes), "new_dead": new_dead, - "resolved_dead": resolved_dead + "resolved_dead": resolved_dead, + "added_edges": added_edges, + "removed_edges": removed_edges, + "added_edges_truncated": added_truncated, + "removed_edges_truncated": removed_truncated, + "added_edge_count": len(added_edge_pairs), + "removed_edge_count": len(removed_edge_pairs), + "unresolved_edges": { + "from": old_unresolved, + "to": new_unresolved, + "delta": new_unresolved - old_unresolved + } } diff --git a/tests/test_diff_engine_edges.py b/tests/test_diff_engine_edges.py new file mode 100644 index 00000000..ca7ab822 --- /dev/null +++ b/tests/test_diff_engine_edges.py @@ -0,0 +1,278 @@ +""" +Tests for call-graph edge comparison in the snapshot diff engine (issue #297). + +Covers: +- ``_endpoint_key()`` — line-independent identity, owner disambiguation, + fallback for ids that resolve to no node. +- ``_diff_backend()`` — added / removed edge detection, line-shift immunity, + call-site multiplicity ignored, unresolved edges counted not enumerated, + detail cap with exact counts, legacy node fields left untouched. +- ``diff_snapshots()`` — edge counts surfaced in the summary block. +""" + +from __future__ import annotations + +import os +import sys +import tempfile + +import pytest + +SCRIPTS_DIR = os.path.abspath( + os.path.join(os.path.dirname(__file__), "..", "scripts") +) +if SCRIPTS_DIR not in sys.path: + sys.path.insert(0, SCRIPTS_DIR) + +from diff_engine import ( # noqa: E402 + EDGE_DETAIL_CAP, + _diff_backend, + _endpoint_key, + _split_edges, + diff_snapshots, + save_snapshot, +) + + +def _node(node_id, fn, file="app.py", **extra): + node = {"id": node_id, "fn": fn, "file": file, "ref_count": 1, "status": "active"} + node.update(extra) + return node + + +def _backend(nodes, edges): + return {"nodes": nodes, "edges": edges} + + +# ─── _endpoint_key ─────────────────────────────────────── + +def test_endpoint_key_is_line_independent(): + """Same function at a different line yields the same key.""" + before = {"app.py:10": _node("app.py:10", "handler")} + after = {"app.py:42": _node("app.py:42", "handler")} + + assert _endpoint_key("app.py:10", before) == _endpoint_key("app.py:42", after) + + +def test_endpoint_key_separates_same_name_methods_by_owner(): + """Two classes in one file, both with `copy`, must not collapse.""" + nodes = { + "app.py:10": _node("app.py:10", "copy", impl_for="TaintInfo"), + "app.py:20": _node("app.py:20", "copy", impl_for="TaintState"), + } + + assert _endpoint_key("app.py:10", nodes) != _endpoint_key("app.py:20", nodes) + + +def test_endpoint_key_normalises_missing_owner_to_string(): + """A missing owner must stay sortable against a present one.""" + nodes = { + "app.py:10": _node("app.py:10", "free_fn"), + "app.py:20": _node("app.py:20", "method", impl_for="Cls"), + } + keys = [_endpoint_key("app.py:10", nodes), _endpoint_key("app.py:20", nodes)] + + assert all(isinstance(part, str) for key in keys for part in key) + sorted(keys) # must not raise TypeError + + +def test_endpoint_key_falls_back_to_raw_id_for_unknown_node(): + """Module-level synthetic sources carry no node by design.""" + assert _endpoint_key("app.py:0:", {}) == ("", "", "app.py:0:") + + +# ─── _diff_backend: the line-shift regression ──────────── + +def test_line_shift_alone_reports_no_edge_change(): + """ + Issue #297 root cause: node ids embed a line number, so keying edges by + raw id reported every edge of a shifted function as removed and re-added. + """ + old = _backend( + [_node("app.py:10", "caller"), _node("app.py:50", "callee")], + [{"from": "app.py:10", "to": "app.py:50"}], + ) + # Identical graph, everything moved down 5 lines. + new = _backend( + [_node("app.py:15", "caller"), _node("app.py:55", "callee")], + [{"from": "app.py:15", "to": "app.py:55"}], + ) + + result = _diff_backend(old, new) + + assert result["added_edge_count"] == 0 + assert result["removed_edge_count"] == 0 + assert result["added_edges"] == [] + assert result["removed_edges"] == [] + + +# ─── _diff_backend: real changes ───────────────────────── + +def test_added_edge_is_detected(): + nodes = [_node("app.py:10", "caller"), _node("app.py:50", "callee")] + old = _backend(nodes, []) + new = _backend(nodes, [{"from": "app.py:10", "to": "app.py:50"}]) + + result = _diff_backend(old, new) + + assert result["added_edge_count"] == 1 + assert result["removed_edge_count"] == 0 + assert result["added_edges"] == [ + {"from": "caller", "from_file": "app.py", "to": "callee", "to_file": "app.py"} + ] + + +def test_removed_edge_is_detected(): + nodes = [_node("app.py:10", "caller"), _node("app.py:50", "callee")] + old = _backend(nodes, [{"from": "app.py:10", "to": "app.py:50"}]) + new = _backend(nodes, []) + + result = _diff_backend(old, new) + + assert result["removed_edge_count"] == 1 + assert result["added_edge_count"] == 0 + assert result["removed_edges"][0]["to"] == "callee" + + +def test_owner_qualified_label_in_detail(): + nodes = [ + _node("app.py:10", "charge", impl_for="Checkout"), + _node("app.py:50", "send", impl_for="Gateway"), + ] + new = _backend(nodes, [{"from": "app.py:10", "to": "app.py:50"}]) + + result = _diff_backend(_backend(nodes, []), new) + + assert result["added_edges"][0]["from"] == "Checkout.charge" + assert result["added_edges"][0]["to"] == "Gateway.send" + + +def test_extra_call_site_is_not_a_graph_change(): + """One edge per call site: multiplicity is not shape.""" + nodes = [_node("app.py:10", "caller"), _node("app.py:50", "callee")] + edge = {"from": "app.py:10", "to": "app.py:50"} + old = _backend(nodes, [edge]) + new = _backend(nodes, [edge, dict(edge)]) + + result = _diff_backend(old, new) + + assert result["added_edge_count"] == 0 + assert result["removed_edge_count"] == 0 + + +def test_via_self_is_a_qualifier_not_an_identity(): + nodes = [_node("app.py:10", "caller"), _node("app.py:50", "callee")] + old = _backend(nodes, [{"from": "app.py:10", "to": "app.py:50"}]) + new = _backend( + nodes, [{"from": "app.py:10", "to": "app.py:50", "via_self": True}] + ) + + result = _diff_backend(old, new) + + assert result["added_edge_count"] == 0 + assert result["removed_edge_count"] == 0 + + +# ─── _diff_backend: unresolved edges ───────────────────── + +def test_unresolved_edges_are_counted_not_enumerated(): + """ + 82% of real edges are unresolved stdlib calls (`append`, `strip`, `get`). + They would drown the signal, so only the tally is reported. + """ + nodes = [_node("app.py:10", "caller")] + old = _backend(nodes, []) + new = _backend( + nodes, + [ + {"from": "app.py:10", "to_fn": "append", "resolved": False}, + {"from": "app.py:10", "to_fn": "strip", "resolved": False}, + ], + ) + + result = _diff_backend(old, new) + + assert result["added_edges"] == [] + assert result["added_edge_count"] == 0 + assert result["unresolved_edges"] == {"from": 0, "to": 2, "delta": 2} + + +def test_split_edges_partitions_resolved_and_unresolved(): + nodes = [_node("app.py:10", "caller"), _node("app.py:50", "callee")] + registry = _backend( + nodes, + [ + {"from": "app.py:10", "to": "app.py:50"}, + {"from": "app.py:10", "to_fn": "append", "resolved": False}, + ], + ) + node_map = {n["id"]: n for n in registry["nodes"]} + + resolved, unresolved = _split_edges(registry, node_map) + + assert len(resolved) == 1 + assert unresolved == 1 + + +# ─── _diff_backend: cap and backward compatibility ─────── + +def test_detail_is_capped_but_counts_stay_exact(): + total = EDGE_DETAIL_CAP + 25 + nodes = [_node("app.py:1", "root")] + [ + _node(f"app.py:{i + 100}", f"fn{i}") for i in range(total) + ] + edges = [{"from": "app.py:1", "to": f"app.py:{i + 100}"} for i in range(total)] + + result = _diff_backend(_backend(nodes, []), _backend(nodes, edges)) + + assert result["added_edge_count"] == total + assert len(result["added_edges"]) == EDGE_DETAIL_CAP + assert result["added_edges_truncated"] is True + + +def test_missing_edges_key_does_not_crash(): + """Snapshots predating edge storage must still diff.""" + result = _diff_backend({"nodes": []}, {"nodes": []}) + + assert result["added_edge_count"] == 0 + assert result["removed_edge_count"] == 0 + assert result["unresolved_edges"] == {"from": 0, "to": 0, "delta": 0} + + +def test_legacy_node_fields_are_unchanged(): + """Consumers (commands/diff.py, dashboard, formatters, MCP) read these.""" + old = _backend([_node("app.py:10", "gone")], []) + new = _backend([_node("app.py:20", "fresh", status="dead")], []) + + result = _diff_backend(old, new) + + for key in ( + "added_nodes", "removed_nodes", "changed_nodes", + "added_count", "removed_count", "changed_count", + "new_dead", "resolved_dead", + ): + assert key in result, f"legacy field {key} disappeared" + + assert result["added_count"] == 1 + assert result["removed_count"] == 1 + assert len(result["new_dead"]) == 1 + + +# ─── summary wiring ────────────────────────────────────── + +def test_summary_exposes_edge_counts(): + with tempfile.TemporaryDirectory() as workspace: + nodes = [_node("app.py:10", "caller"), _node("app.py:50", "callee")] + frontend = {"classes": [], "ids": []} + + save_snapshot(workspace, frontend, _backend(nodes, [])) + save_snapshot( + workspace, + frontend, + _backend(nodes, [{"from": "app.py:10", "to": "app.py:50"}]), + ) + + result = diff_snapshots(workspace) + + assert result["summary"]["edges_added"] == 1 + assert result["summary"]["edges_removed"] == 0 From 0f68bcc7e629c4241c5184dae416060d1c0df6de Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Fri, 17 Jul 2026 11:54:08 +0700 Subject: [PATCH 2/2] refactor(diff): name unresolved tally sides before/after, not from/to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In an edge diff `from`/`to` already mean an edge's endpoints, so reusing them for the old/new snapshot sides of the unresolved tally reads as a bug at a glance. No consumers yet — renaming now is free. Co-Authored-By: Claude Opus 4.8 --- scripts/diff_engine.py | 6 ++++-- tests/test_diff_engine_edges.py | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/scripts/diff_engine.py b/scripts/diff_engine.py index bc2a975d..fc8383ef 100755 --- a/scripts/diff_engine.py +++ b/scripts/diff_engine.py @@ -446,9 +446,11 @@ def _diff_backend(old: Dict, new: Dict) -> Dict: "removed_edges_truncated": removed_truncated, "added_edge_count": len(added_edge_pairs), "removed_edge_count": len(removed_edge_pairs), + # `before`/`after`, not `from`/`to`: those already mean an edge's + # endpoints here, and reusing them for snapshot sides reads as a bug. "unresolved_edges": { - "from": old_unresolved, - "to": new_unresolved, + "before": old_unresolved, + "after": new_unresolved, "delta": new_unresolved - old_unresolved } } diff --git a/tests/test_diff_engine_edges.py b/tests/test_diff_engine_edges.py index ca7ab822..f2fbe7ee 100644 --- a/tests/test_diff_engine_edges.py +++ b/tests/test_diff_engine_edges.py @@ -194,7 +194,7 @@ def test_unresolved_edges_are_counted_not_enumerated(): assert result["added_edges"] == [] assert result["added_edge_count"] == 0 - assert result["unresolved_edges"] == {"from": 0, "to": 2, "delta": 2} + assert result["unresolved_edges"] == {"before": 0, "after": 2, "delta": 2} def test_split_edges_partitions_resolved_and_unresolved(): @@ -236,7 +236,7 @@ def test_missing_edges_key_does_not_crash(): assert result["added_edge_count"] == 0 assert result["removed_edge_count"] == 0 - assert result["unresolved_edges"] == {"from": 0, "to": 0, "delta": 0} + assert result["unresolved_edges"] == {"before": 0, "after": 0, "delta": 0} def test_legacy_node_fields_are_unchanged():