diff --git a/CHANGELOG.md b/CHANGELOG.md index 18a23a4..553d2f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -131,6 +131,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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.) + Move detection is symmetric since the follow-up: `new-untested-hotspot` + and `new-dead-code` likewise skip ADDED symbols whose terminal name + exists in the removed set — a relocated function is not new code, so + its pre-existing coverage state doesn't gate the PR that moved it + (`formatQn` was flagged as a "new" untested hotspot by the same + app.js split). - **`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/rules.py b/codegraph/review/rules.py index 392b15c..07962c6 100644 --- a/codegraph/review/rules.py +++ b/codegraph/review/rules.py @@ -235,6 +235,20 @@ def evaluate_rules( findings: list[Finding] = [] cycle_introduced = _introduces_cycle(new_graph, old_graph, cycle_cache) + # Move detection: a symbol removed at one qualname and added at another + # with the same terminal name and kind was relocated, not deleted/created. + # Removed-side rules skip when the name reappears in the added set; + # added-side "new code" rules skip when it came from the removed set. + added_names = { + (c.qualname.rsplit(".", 1)[-1], c.kind) for c in diff.added_nodes + } + removed_names = { + (c.qualname.rsplit(".", 1)[-1], c.kind) for c in diff.removed_nodes + } + + def _is_moved(change: NodeChange, counterpart: set[tuple[str, str]]) -> bool: + return (change.qualname.rsplit(".", 1)[-1], change.kind) in counterpart + for rule in rules: when = rule.when if when == "added_node": @@ -275,19 +289,12 @@ 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 - } + # PR #73 flagged esc/pyvisHref as critical after they moved + # from app.js to ui/helpers.js with all callers updated. for change in diff.removed_nodes: if not _node_matches(rule, change): continue - if (change.qualname.rsplit(".", 1)[-1], change.kind) in moved: + if _is_moved(change, added_names): continue old_id = _find_node_id(change.qualname, change.kind, old_graph) if old_id is None: @@ -324,6 +331,8 @@ def evaluate_rules( continue if change.kind not in ("FUNCTION", "METHOD"): continue + if _is_moved(change, removed_names): + continue new_id = _find_node_id(change.qualname, change.kind, new_graph) if new_id is None: continue @@ -347,6 +356,11 @@ def evaluate_rules( NodeKind.METHOD.value, ): continue + # A moved symbol is not NEW code — its (lack of) test + # coverage predates the PR. PR #73 flagged formatQn as a + # new untested hotspot after it moved app.js -> helpers.js. + if _is_moved(change, removed_names): + continue new_id = _find_node_id(change.qualname, change.kind, new_graph) if new_id is None: continue diff --git a/tests/test_review_rules.py b/tests/test_review_rules.py index 7c0b863..d7b0e31 100644 --- a/tests/test_review_rules.py +++ b/tests/test_review_rules.py @@ -153,3 +153,57 @@ def test_removed_referenced_still_fires_for_real_removal() -> None: 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"] + + +def test_new_untested_hotspot_skips_moved_symbol() -> None: + """Regression test: a symbol that MOVED modules is not NEW code — its + test-coverage state predates the PR. PR #73 flagged formatQn as a new + untested hotspot after it moved from app.js to ui/helpers.js.""" + old_g: nx.MultiDiGraph = nx.MultiDiGraph() + new_g: nx.MultiDiGraph = nx.MultiDiGraph() + _add_fn(old_g, "pkg.app.formatQn", file="app.js") + _add_fn(new_g, "pkg.ui.helpers.formatQn", file="ui/helpers.js") + # Give the moved symbol >= 5 non-test callers in the new graph. + for i in range(5): + caller = f"pkg.views.v{i}.render" + _add_fn(old_g, caller, file=f"views/v{i}.js") + _add_fn(new_g, caller, file=f"views/v{i}.js") + new_g.add_edge( + f"node::{caller}", "node::pkg.ui.helpers.formatQn", + key="CALLS", kind="CALLS", + ) + 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 == "new-untested-hotspot"], ( + "moved symbol must not be reported as a new untested hotspot" + ) + + +def test_new_untested_hotspot_still_fires_for_genuinely_new() -> None: + """Control: a genuinely new high-fan-in untested function still fires.""" + old_g: nx.MultiDiGraph = nx.MultiDiGraph() + new_g: nx.MultiDiGraph = nx.MultiDiGraph() + _add_fn(new_g, "pkg.core.hotNew", file="core.py") + for i in range(5): + caller = f"pkg.views.v{i}.render" + _add_fn(old_g, caller, file=f"views/v{i}.py") + _add_fn(new_g, caller, file=f"views/v{i}.py") + new_g.add_edge( + f"node::{caller}", "node::pkg.core.hotNew", + key="CALLS", kind="CALLS", + ) + 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 == "new-untested-hotspot"] + + +def test_new_dead_code_skips_moved_symbol() -> None: + """A moved symbol with zero callers was already dead before the PR; + new-dead-code only reports genuinely new unreachable code.""" + old_g: nx.MultiDiGraph = nx.MultiDiGraph() + new_g: nx.MultiDiGraph = nx.MultiDiGraph() + _add_fn(old_g, "pkg.app.orphan", file="app.js") + _add_fn(new_g, "pkg.ui.helpers.orphan", file="ui/helpers.js") + 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 == "new-dead-code"]