From a5ec3f95dbe2da4aefd35da08257adba9f3fe487 Mon Sep 17 00:00:00 2001 From: mochan Date: Wed, 10 Jun 2026 20:21:00 +0530 Subject: [PATCH 1/2] fix(review): exclude dataflow nodes from diff + skip moved symbols in removed-referenced The wave-2 PRs tripped the self-review gate with two false-positive classes: 1. VARIABLE/PARAMETER nodes (new in the data-edges work) embed line numbers in their qualnames, so any line shift makes every local appear removed+added. A clean typescript.py refactor produced 12+ removed-referenced criticals on its own local variables. The differ now excludes these kinds and DATA_* edges from review diffs - they are dataflow plumbing, not reviewable API surface. 2. removed-referenced fired on symbols that moved modules (esc, pyvisHref, mermaidThemeVars: app.js -> ui/helpers.js) even though every caller was updated. The rule now skips removed nodes whose terminal name reappears in the added set with the same kind. Genuine deletions with surviving callers still fire (control test added). Gate: 746 tests pass, ruff + mypy --strict clean, self-lint clean. Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 12 +++++++++ codegraph/review/differ.py | 11 +++++++- codegraph/review/rules.py | 11 ++++++++ tests/test_review_differ.py | 54 +++++++++++++++++++++++++++++++++++++ tests/test_review_rules.py | 52 +++++++++++++++++++++++++++++++++++ 5 files changed, 139 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 62a2932..18a23a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -119,6 +119,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **Review diff no longer churns on dataflow nodes; moved symbols no + longer flag as removed.** The graph differ now excludes `VARIABLE` / + `PARAMETER` nodes and `DATA_*` edges from review diffs — their + qualnames embed line numbers (`pkg.fn.`), so any edit that + shifted lines made every local variable appear removed+added and fired + false-positive `removed-referenced` criticals (12+ on a clean + typescript.py refactor). Separately, `removed-referenced` now skips + symbols whose terminal name reappears in the added set with the same + kind — moving `esc`/`pyvisHref` from `app.js` to `ui/helpers.js` with + every caller updated was flagged critical three times. Genuine + deletions with surviving callers still fire. (Both surfaced by the + self-review gate on the wave-2 dashboard/resolver PRs.) - **`codegraph init` now writes a robust `.mcp.json`** — uses the absolute path to the `codegraph` binary (resolved from the running interpreter's bin dir, then `shutil.which`, then bare fallback), passes an explicit diff --git a/codegraph/review/differ.py b/codegraph/review/differ.py index 4630e67..0d93f22 100644 --- a/codegraph/review/differ.py +++ b/codegraph/review/differ.py @@ -6,6 +6,13 @@ import networkx as nx +# Sub-symbol granularity introduced by the dataflow pass. Variable and +# parameter qualnames embed line numbers (``pkg.fn.``), so any +# edit that shifts lines makes every local appear removed+added. Review +# operates on API-level symbols; dataflow plumbing is excluded outright. +_EXCLUDED_NODE_KINDS = frozenset({"VARIABLE", "PARAMETER"}) +_EXCLUDED_EDGE_KINDS = frozenset({"DATA_ASSIGN", "DATA_ARG", "DATA_RETURN"}) + @dataclass class NodeChange: @@ -55,7 +62,7 @@ def _kind_str(value: object) -> str: def _node_key(attrs: dict[str, Any]) -> _NodeKey | None: qualname = str(attrs.get("qualname") or "") kind = _kind_str(attrs.get("kind")) - if not qualname or not kind: + if not qualname or not kind or kind in _EXCLUDED_NODE_KINDS: return None return (qualname, kind) @@ -99,6 +106,8 @@ def _edge_keys( keys: set[tuple[str, str, str]] = set() for src, dst, data in graph.edges(data=True): kind = _kind_str(data.get("kind")) + if kind in _EXCLUDED_EDGE_KINDS: + continue src_qn = id_map.get(src, src) dst_qn = id_map.get(dst, dst) keys.add((src_qn, dst_qn, kind)) diff --git a/codegraph/review/rules.py b/codegraph/review/rules.py index c34e2bb..392b15c 100644 --- a/codegraph/review/rules.py +++ b/codegraph/review/rules.py @@ -275,9 +275,20 @@ def evaluate_rules( ) ) elif when == "removed_referenced": + # A removed symbol whose terminal name reappears in the added + # set (same kind) was moved, not deleted — callers reference the + # new location. PR #73 flagged esc/pyvisHref as critical after + # they moved from app.js to ui/helpers.js with all callers + # updated. + moved = { + (c.qualname.rsplit(".", 1)[-1], c.kind) + for c in diff.added_nodes + } for change in diff.removed_nodes: if not _node_matches(rule, change): continue + if (change.qualname.rsplit(".", 1)[-1], change.kind) in moved: + continue old_id = _find_node_id(change.qualname, change.kind, old_graph) if old_id is None: continue diff --git a/tests/test_review_differ.py b/tests/test_review_differ.py index b4bbea4..f346e18 100644 --- a/tests/test_review_differ.py +++ b/tests/test_review_differ.py @@ -174,3 +174,57 @@ def test_line_shift_AND_signature_change_records_both_in_details() -> None: details = diff.modified_nodes[0].details or {} assert "signature" in details assert "line_start" in details + + +def _add_node( + g: nx.MultiDiGraph, + qn: str, + *, + kind: str, + file: str = "pkg/foo.py", + line: int = 1, +) -> None: + g.add_node( + f"node::{qn}", + qualname=qn, + kind=kind, + file=file, + line_start=line, + signature="", + ) + + +def test_variable_and_parameter_nodes_are_ignored() -> None: + """Regression test: dataflow VARIABLE/PARAMETER qualnames embed line + numbers (``pkg.fn.``), so a pure line shift makes every local + look removed+added. PR #74 got 12+ false-positive ``removed-referenced`` + criticals on refactored locals in typescript.py.""" + old_g: nx.MultiDiGraph = nx.MultiDiGraph() + new_g: nx.MultiDiGraph = nx.MultiDiGraph() + _add_func(old_g, "pkg.fn", signature="fn()") + _add_func(new_g, "pkg.fn", signature="fn()") + _add_node(old_g, "pkg.fn.", kind="VARIABLE", line=42) + _add_node(old_g, "pkg.fn.", kind="PARAMETER", line=40) + _add_node(new_g, "pkg.fn.", kind="VARIABLE", line=57) + _add_node(new_g, "pkg.fn.", kind="PARAMETER", line=55) + diff = diff_graphs(old_g, new_g) + assert diff.added_nodes == [] + assert diff.removed_nodes == [] + assert diff.modified_nodes == [] + + +def test_data_edges_are_ignored() -> None: + """DATA_* edges churn with every line edit alongside their var nodes; + they are dataflow plumbing, not reviewable API surface.""" + old_g: nx.MultiDiGraph = nx.MultiDiGraph() + new_g: nx.MultiDiGraph = nx.MultiDiGraph() + _add_func(old_g, "pkg.fn", signature="fn()") + _add_func(new_g, "pkg.fn", signature="fn()") + _add_node(new_g, "pkg.fn.", kind="VARIABLE", line=57) + new_g.add_edge( + "node::pkg.fn", "node::pkg.fn.", + key="DATA_ASSIGN", kind="DATA_ASSIGN", + ) + diff = diff_graphs(old_g, new_g) + assert diff.added_edges == [] + assert diff.removed_edges == [] diff --git a/tests/test_review_rules.py b/tests/test_review_rules.py index 35820fb..7c0b863 100644 --- a/tests/test_review_rules.py +++ b/tests/test_review_rules.py @@ -101,3 +101,55 @@ def test_evaluate_rules_with_custom_rule( ) assert findings assert all(f.rule_id == "any-add" for f in findings) + + +def _add_fn( + g: nx.MultiDiGraph, + qn: str, + *, + file: str = "pkg/foo.py", + line: int = 1, +) -> None: + g.add_node( + f"node::{qn}", + qualname=qn, + kind="FUNCTION", + file=file, + line_start=line, + signature=f"{qn.rsplit('.', 1)[-1]}()", + ) + + +def test_removed_referenced_skips_moved_symbol() -> None: + """Regression test: a symbol moved to another module (removed at one + qualname, added at another with the same terminal name and kind) must + not fire ``removed-referenced``. PR #73 flagged esc/pyvisHref/ + mermaidThemeVars as critical after they moved from app.js to + ui/helpers.js with every caller updated.""" + old_g: nx.MultiDiGraph = nx.MultiDiGraph() + new_g: nx.MultiDiGraph = nx.MultiDiGraph() + _add_fn(old_g, "pkg.app.esc", file="app.js") + _add_fn(old_g, "pkg.app.render", file="app.js", line=10) + old_g.add_edge("node::pkg.app.render", "node::pkg.app.esc", key="CALLS", kind="CALLS") + # esc moved to helpers; caller render survives. + _add_fn(new_g, "pkg.ui.helpers.esc", file="ui/helpers.js") + _add_fn(new_g, "pkg.app.render", file="app.js", line=10) + diff = diff_graphs(old_g, new_g) + findings = evaluate_rules(diff, new_graph=new_g, old_graph=old_g) + assert not [f for f in findings if f.rule_id == "removed-referenced"], ( + "moved symbol must not be reported as removed-referenced" + ) + + +def test_removed_referenced_still_fires_for_real_removal() -> None: + """Control: genuinely deleting a symbol that callers still reference + keeps firing critical.""" + old_g: nx.MultiDiGraph = nx.MultiDiGraph() + new_g: nx.MultiDiGraph = nx.MultiDiGraph() + _add_fn(old_g, "pkg.app.esc", file="app.js") + _add_fn(old_g, "pkg.app.render", file="app.js", line=10) + old_g.add_edge("node::pkg.app.render", "node::pkg.app.esc", key="CALLS", kind="CALLS") + _add_fn(new_g, "pkg.app.render", file="app.js", line=10) + diff = diff_graphs(old_g, new_g) + findings = evaluate_rules(diff, new_graph=new_g, old_graph=old_g) + assert [f for f in findings if f.rule_id == "removed-referenced"] From 1a18d328ce836eae9a8f7ebc5555002377549be1 Mon Sep 17 00:00:00 2001 From: mochan Date: Wed, 10 Jun 2026 20:25:54 +0530 Subject: [PATCH 2/2] ci(review): stop baseline step clobbering the PR-head install The baseline build ran 'pip install -e .' inside the origin/main worktree, silently replacing the PR-head editable install from the previous step. Every subsequent step - the PR graph build, the diff, and the rule evaluation - ran main's codegraph, so PR changes to the differ/rules/parsers never took effect in their own review. This PR's differ fix was itself flagged by the exact var-node noise it removes. Both graphs are now built with the PR-head install, which also keeps diffs apples-to-apples when a PR changes parser output. Co-Authored-By: Claude Fable 5 --- .github/workflows/pr-review.yml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index 6765494..afa5a03 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -39,9 +39,15 @@ jobs: BASELINE_DIR="${RUNNER_TEMP}/cg-baseline-checkout" rm -rf "$BASELINE_DIR" git worktree add "$BASELINE_DIR" origin/main + # Both graphs are built with the PR-head codegraph (installed in + # the previous step). Re-installing from the baseline worktree + # here would silently clobber the editable install, making the + # diff AND the rule evaluation run main's code — PR changes to + # the differ/rules/parsers would never take effect in their own + # review, and parser changes would dominate the diff as + # added/removed-node noise. ( cd "$BASELINE_DIR" - pip install -e . --quiet codegraph build --no-incremental codegraph baseline save --output "$GITHUB_WORKSPACE/.codegraph/baseline.db" )