From 94feec406185f92186b893637e1b062a31bd18e4 Mon Sep 17 00:00:00 2001 From: Wolfvin Date: Fri, 17 Jul 2026 12:56:42 +0700 Subject: [PATCH] fix(diff): key node diff by identity, not line-bearing id (closes #300) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _diff_backend() keyed nodes by n["id"], which embeds a line number, so adding a comment to a file reported every function in it as added AND removed and fired false new_dead / resolved_dead. On the real 4,188-node polyglot workspace a pure line shift with zero real changes produced 1,137 false adds, 1,137 false removes and 260 false dead-code alarms. False new_dead is the dangerous half: an agent reading "this function just became dead" can delete live code. Same class as #293 and #294. Nodes now key on (file, impl_for, fn) via the _node_key/_index_nodes pair factored out of #297's _endpoint_key, so both halves of the diff share one identity scheme. Functions that share (file, impl_for, fn) — closures like `visit` redefined in a scope, already flagged duplicate_define — get an occurrence index in line order, so deleting one of two is still detected while a shift keeps the keys stable. Edge resolution still needs raw ids, so nodes are indexed twice: by id for endpoints, by identity for the diff. Recovers lost signal too: a shifted node was never compared against its old self, so its ref_count/status change vanished. changed_nodes now survives a line shift. Output shape unchanged. Full suite: 19 failures on this branch, 19 on main, identical list. Co-Authored-By: Claude Opus 4.8 --- scripts/diff_engine.py | 75 ++++++++--- tests/test_diff_engine_nodes.py | 227 ++++++++++++++++++++++++++++++++ 2 files changed, 283 insertions(+), 19 deletions(-) create mode 100644 tests/test_diff_engine_nodes.py diff --git a/scripts/diff_engine.py b/scripts/diff_engine.py index fc8383e..03d79e1 100755 --- a/scripts/diff_engine.py +++ b/scripts/diff_engine.py @@ -308,29 +308,62 @@ def _diff_frontend(old: Dict, new: Dict) -> Dict: } -def _endpoint_key(node_id: str, node_map: Dict) -> Tuple[str, str, str]: +def _node_key(node: 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. + Line-independent identity for a node: (file, owner, fn). + + Node ids embed a line number (`engine.py:167`), so keying by raw id treats + a function that merely shifted lines as a different function. `impl_for` + (the owning class/struct) separates same-named methods that share a file. + Values are normalised to strings so a missing owner still sorts against a + present one. """ - 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 + node.get("fn") or node.get("id") or "" ) +def _index_nodes(nodes: List[Dict]) -> Dict[Tuple, Dict]: + """ + Map line-independent identity -> node, disambiguating exact duplicates. + + A handful of functions share (file, owner, fn) — closures like `visit` or + `_walk` redefined in the same scope, which the registry already flags as + `duplicate_define`. Collapsing them would hide a real deletion, so each + gets an occurrence index assigned in line order. A pure line shift moves + them together, leaving the indices — and therefore the keys — stable. + """ + index: Dict[Tuple, Dict] = {} + seen: Dict[Tuple, int] = {} + + for node in sorted( + nodes, + key=lambda n: (n.get("file") or "", n.get("line") or 0, n.get("fn") or "") + ): + base = _node_key(node) + occurrence = seen.get(base, 0) + seen[base] = occurrence + 1 + index[base + (occurrence,)] = node + + return index + + +def _endpoint_key(node_id: str, node_map: Dict) -> Tuple[str, str, str]: + """ + Line-independent identity for an edge endpoint. + + 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) + return _node_key(node) + + def _split_edges( registry: Dict, node_map: Dict @@ -383,8 +416,12 @@ def _format_edges(pairs: Set[Tuple[Tuple, Tuple]]) -> Tuple[List[Dict[str, str]] def _diff_backend(old: Dict, new: Dict) -> Dict: """Diff two backend registries.""" - old_nodes = {n["id"]: n for n in old.get("nodes", [])} - new_nodes = {n["id"]: n for n in new.get("nodes", [])} + # Two views of the same nodes: by raw id for edge endpoint resolution, + # by line-independent identity for the node diff itself. + old_by_id = {n["id"]: n for n in old.get("nodes", [])} + new_by_id = {n["id"]: n for n in new.get("nodes", [])} + old_nodes = _index_nodes(old.get("nodes", [])) + new_nodes = _index_nodes(new.get("nodes", [])) added_nodes = [] removed_nodes = [] @@ -422,8 +459,8 @@ 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) + old_edges, old_unresolved = _split_edges(old, old_by_id) + new_edges, new_unresolved = _split_edges(new, new_by_id) added_edge_pairs = new_edges - old_edges removed_edge_pairs = old_edges - new_edges diff --git a/tests/test_diff_engine_nodes.py b/tests/test_diff_engine_nodes.py new file mode 100644 index 0000000..cf0e966 --- /dev/null +++ b/tests/test_diff_engine_nodes.py @@ -0,0 +1,227 @@ +""" +Tests for line-independent node identity in the snapshot diff (issue #300). + +`_diff_backend()` keyed nodes by `n["id"]`, which embeds a line number, so +adding a comment to a file reported every function in it as added AND removed +and fired false `new_dead` / `resolved_dead`. On the real 4,188-node polyglot +workspace a pure line shift produced 1,137 false adds, 1,137 false removes and +260 false dead-code alarms. + +Covers: +- ``_node_key()`` / ``_index_nodes()`` — line independence, owner separation, + duplicate-define disambiguation. +- ``_diff_backend()`` — no churn on line shift, real adds/removes/dead still + detected, changed_nodes surviving a shift, output shape unchanged. +""" + +from __future__ import annotations + +import os +import sys + +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 + _diff_backend, + _index_nodes, + _node_key, +) + + +def _node(node_id, fn, line, ref_count=1, status="active", file=None, **extra): + node = { + "id": node_id, + "fn": fn, + "file": file if file is not None else node_id.split(":")[0], + "line": line, + "ref_count": ref_count, + "status": status, + } + node.update(extra) + return node + + +def _backend(nodes, edges=None): + return {"nodes": nodes, "edges": edges or []} + + +# ─── identity ──────────────────────────────────────────── + +def test_node_key_ignores_line_number(): + before = _node("app.py:10", "handler", 10) + after = _node("app.py:42", "handler", 42) + + assert _node_key(before) == _node_key(after) + + +def test_node_key_separates_same_name_methods_by_owner(): + a = _node("app.py:10", "copy", 10, impl_for="TaintInfo") + b = _node("app.py:20", "copy", 20, impl_for="TaintState") + + assert _node_key(a) != _node_key(b) + + +def test_node_key_separates_same_name_across_files(): + a = _node("a.py:10", "main", 10) + b = _node("b.py:10", "main", 10) + + assert _node_key(a) != _node_key(b) + + +def test_index_nodes_disambiguates_duplicate_defines(): + """Closures redefined in one scope must not collapse onto one key.""" + nodes = [ + _node("p.py:10", "visit", 10, impl_for="P"), + _node("p.py:50", "visit", 50, impl_for="P", duplicate_define=True), + ] + + index = _index_nodes(nodes) + + assert len(index) == 2 + + +def test_index_nodes_keeps_duplicate_order_stable_under_line_shift(): + before = _index_nodes([ + _node("p.py:10", "visit", 10, impl_for="P"), + _node("p.py:50", "visit", 50, impl_for="P", duplicate_define=True), + ]) + after = _index_nodes([ + _node("p.py:15", "visit", 15, impl_for="P"), + _node("p.py:55", "visit", 55, impl_for="P", duplicate_define=True), + ]) + + assert set(before) == set(after) + + +# ─── the regression ────────────────────────────────────── + +def test_line_shift_reports_no_node_change(): + """Issue #300: a comment at the top of a file changed every node id.""" + old = _backend([ + _node("checkout.py:6", "handle_checkout", 6, ref_count=0, status="dead"), + _node("checkout.py:12", "send_receipt", 12), + ]) + new = _backend([ + _node("checkout.py:11", "handle_checkout", 11, ref_count=0, status="dead"), + _node("checkout.py:17", "send_receipt", 17), + ]) + + result = _diff_backend(old, new) + + assert result["added_nodes"] == [] + assert result["removed_nodes"] == [] + assert result["changed_nodes"] == [] + assert result["added_count"] == 0 + assert result["removed_count"] == 0 + + +def test_already_dead_function_is_not_reported_newly_dead_after_shift(): + """The dangerous one: agents delete code that `new_dead` points at.""" + old = _backend([_node("a.py:1", "entry", 1, ref_count=0, status="dead")]) + new = _backend([_node("a.py:9", "entry", 9, ref_count=0, status="dead")]) + + result = _diff_backend(old, new) + + assert result["new_dead"] == [] + assert result["resolved_dead"] == [] + + +# ─── real signals still land ───────────────────────────── + +def test_genuinely_added_and_removed_nodes_detected(): + old = _backend([_node("a.py:1", "goneFn", 1)]) + new = _backend([_node("a.py:1", "freshFn", 1)]) + + result = _diff_backend(old, new) + + assert [n["name"] for n in result["added_nodes"]] == ["freshFn"] + assert [n["name"] for n in result["removed_nodes"]] == ["goneFn"] + + +def test_genuinely_newly_dead_is_detected(): + old = _backend([_node("a.py:1", "f", 1, ref_count=1, status="active")]) + new = _backend([_node("a.py:9", "f", 9, ref_count=0, status="dead")]) + + result = _diff_backend(old, new) + + assert [n["name"] for n in result["new_dead"]] == ["f"] + + +def test_status_and_refcount_change_survives_a_line_shift(): + """ + Previously a shifted node was never compared against its old self, so its + ref_count/status change vanished. Removing the churn also recovers this. + """ + old = _backend([_node("a.py:1", "f", 1, ref_count=2, status="active")]) + new = _backend([_node("a.py:9", "f", 9, ref_count=1, status="active")]) + + result = _diff_backend(old, new) + + assert len(result["changed_nodes"]) == 1 + assert result["changed_nodes"][0]["ref_count"] == {"from": 2, "to": 1} + + +def test_deleting_one_of_two_duplicate_defines_is_detected(): + old = _backend([ + _node("p.py:10", "visit", 10, impl_for="P"), + _node("p.py:50", "visit", 50, impl_for="P", duplicate_define=True), + ]) + new = _backend([_node("p.py:12", "visit", 12, impl_for="P")]) + + result = _diff_backend(old, new) + + assert [n["name"] for n in result["removed_nodes"]] == ["visit"] + + +def test_function_moved_to_another_file_counts_as_moved(): + """File is part of identity — a cross-file move is a real relocation.""" + old = _backend([_node("a.py:1", "f", 1)]) + new = _backend([_node("b.py:1", "f", 1)]) + + result = _diff_backend(old, new) + + assert [n["name"] for n in result["added_nodes"]] == ["f"] + assert [n["name"] for n in result["removed_nodes"]] == ["f"] + + +# ─── shape ─────────────────────────────────────────────── + +def test_output_shape_unchanged(): + old = _backend([_node("a.py:1", "goneFn", 1)]) + new = _backend([_node("a.py:5", "freshFn", 5, status="dead", ref_count=0)]) + + 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 + + assert set(result["added_nodes"][0]) == {"name", "file", "status"} + assert set(result["removed_nodes"][0]) == {"name", "file"} + + +def test_edge_diff_still_works_through_id_map(): + """Node diffing moved off raw ids; edges must still resolve by id.""" + old = _backend( + [_node("a.py:1", "caller", 1), _node("a.py:9", "callee", 9)], + [], + ) + new = _backend( + [_node("a.py:1", "caller", 1), _node("a.py:9", "callee", 9)], + [{"from": "a.py:1", "to": "a.py:9"}], + ) + + result = _diff_backend(old, new) + + assert result["added_edge_count"] == 1 + assert result["added_edges"][0]["from"] == "caller" + assert result["added_edges"][0]["to"] == "callee"