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" ) 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"]