From 34164b837102e2676acc2a5467f0b0cfaf841c6e Mon Sep 17 00:00:00 2001 From: Michael Denyer Date: Fri, 12 Jun 2026 20:07:23 +0100 Subject: [PATCH] feat: optional change-frequency (churn) term in risk scoring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Risk scores were 100% structural (flows, communities, test coverage, security keywords, caller count). Hotspot research (Tornhill) shows defect risk correlates with structural complexity x change frequency — the temporal half was missing. Add compute_file_churn(): per-file commit counts over a trailing window (CRG_CHURN_WINDOW_DAYS, default 90 days), parsed natively from git log --numstat with no new dependencies. compute_risk_score() takes an optional churn_counts mapping contributing up to 0.15 (saturating at 10 commits). Opt-in via analyze_changes(include_churn=True) or detect-changes --churn; default-off so existing structural-only scores and published benchmark numbers are unchanged. --- code_review_graph/changes.py | 115 +++++++++++++++++++++- code_review_graph/cli.py | 9 ++ docs/COMMANDS.md | 1 + tests/test_changes.py | 183 +++++++++++++++++++++++++++++++++++ 4 files changed, 303 insertions(+), 5 deletions(-) diff --git a/code_review_graph/changes.py b/code_review_graph/changes.py index 77dc25ad..855c5bc4 100644 --- a/code_review_graph/changes.py +++ b/code_review_graph/changes.py @@ -167,7 +167,83 @@ def _parse_unified_diff(diff_text: str) -> dict[str, list[tuple[int, int]]]: # --------------------------------------------------------------------------- -# 2. map_changes_to_nodes +# 2. compute_file_churn +# --------------------------------------------------------------------------- + +# Commits within the churn window at which the churn risk term saturates. +_CHURN_SATURATION = 10.0 +_CHURN_WEIGHT = 0.15 + +# Match "addeddeletedpath" numstat lines ("-" counts for binary files). +_NUMSTAT_LINE = re.compile(r"^(?:\d+|-)\t(?:\d+|-)\t(.+)$") + + +def _parse_numstat(log_text: str) -> dict[str, int]: + """Parse ``git log --numstat`` output into per-file commit counts. + + Each numstat line represents one file touched by one commit, so + counting lines per path yields commits-per-file. + """ + counts: dict[str, int] = {} + for line in log_text.splitlines(): + match = _NUMSTAT_LINE.match(line) + if match: + path = match.group(1) + counts[path] = counts.get(path, 0) + 1 + return counts + + +def compute_file_churn(repo_root: str, window_days: int | None = None) -> dict[str, int]: + """Count commits touching each file over a trailing window. + + The temporal half of hotspot analysis (Tornhill): defect risk + correlates with structural complexity x change frequency. Uses + native ``git log --numstat`` parsing — no external dependencies. + Renames are not followed (``--no-renames``); churn accrues to the + path as it existed in each commit. + + Args: + repo_root: Absolute path to the repository root. + window_days: Trailing window in days. Defaults to the + ``CRG_CHURN_WINDOW_DAYS`` environment variable (90). + + Returns: + Mapping of repo-relative forward-slash paths to the number of + commits touching them within the window. Empty dict on error + (not a git repo, git unavailable, timeout) or when the window + is not positive. + """ + if window_days is None: + window_days = int(os.environ.get("CRG_CHURN_WINDOW_DAYS", "90")) + if window_days <= 0: + return {} + try: + result = subprocess.run( + [ + "git", "-c", "core.quotepath=off", "log", + f"--since={window_days} days ago", "--numstat", + "--no-renames", "--format=", + ], + capture_output=True, + stdin=subprocess.DEVNULL, + text=True, + encoding="utf-8", + errors="replace", + cwd=repo_root, + timeout=_GIT_TIMEOUT, + ) + if result.returncode != 0: + logger.warning("git log failed (rc=%d): %s", result.returncode, result.stderr[:200]) + return {} + except (OSError, subprocess.SubprocessError) as exc: + logger.warning("git log error: %s", exc) + return {} + + return _parse_numstat(result.stdout) + + +# --------------------------------------------------------------------------- +# 3. map_changes_to_nodes # --------------------------------------------------------------------------- @@ -212,11 +288,15 @@ def map_changes_to_nodes( # --------------------------------------------------------------------------- -# 3. compute_risk_score +# 4. compute_risk_score # --------------------------------------------------------------------------- -def compute_risk_score(store: GraphStore, node: GraphNode) -> float: +def compute_risk_score( + store: GraphStore, + node: GraphNode, + churn_counts: dict[str, int] | None = None, +) -> float: """Compute a risk score (0.0 - 1.0) for a single node. Scoring factors: @@ -225,6 +305,9 @@ def compute_risk_score(store: GraphStore, node: GraphNode) -> float: - Test coverage: 0.30 (untested) scaling down to 0.05 (5+ TESTED_BY edges) - Security sensitivity: 0.20 if name matches security keywords - Caller count: callers / 20, capped at 0.10 + - Change frequency (opt-in): commits touching the file / 10, capped + at 0.15. Only applied when *churn_counts* is provided (see + :func:`compute_file_churn`). """ score = 0.0 @@ -266,11 +349,16 @@ def compute_risk_score(store: GraphStore, node: GraphNode) -> float: caller_count = len(caller_edges) score += min(caller_count / 20.0, 0.10) + # --- Change frequency (opt-in, cap 0.15) --- + if churn_counts and node.file_path: + commit_count = churn_counts.get(node.file_path, 0) + score += min(commit_count / _CHURN_SATURATION, 1.0) * _CHURN_WEIGHT + return round(min(max(score, 0.0), 1.0), 4) # --------------------------------------------------------------------------- -# 4. analyze_changes +# 5. analyze_changes # --------------------------------------------------------------------------- @@ -280,6 +368,7 @@ def analyze_changes( changed_ranges: dict[str, list[tuple[int, int]]] | None = None, repo_root: str | None = None, base: str = "HEAD~1", + include_churn: bool = False, ) -> dict[str, Any]: """Analyze changes and produce risk-scored review guidance. @@ -291,6 +380,11 @@ def analyze_changes( (Git or SVN). repo_root: Repository root (for git/svn diff). base: Git ref or SVN revision range to diff against. + include_churn: When True and ``repo_root`` is given, add a change + frequency term to each node's risk score, sourced from + ``git log --numstat`` over the trailing churn window + (``CRG_CHURN_WINDOW_DAYS``, default 90 days). Off by default + so structural-only scores are unchanged. Returns: Dict with ``summary``, ``risk_score``, ``changed_functions``, @@ -332,10 +426,21 @@ def analyze_changes( if funcs_truncated: changed_funcs = changed_funcs[:_max_funcs] + # Optional churn term: key counts by both the repo-relative path git + # reports and the absolute native path, so lookups succeed however + # the graph stored node file paths (mirrors the diff-key remap above). + churn_counts: dict[str, int] | None = None + if include_churn and repo_root is not None: + churn_counts = {} + root_path = Path(repo_root) + for key, count in compute_file_churn(repo_root).items(): + churn_counts[key] = count + churn_counts[str(root_path / key)] = count + # Compute per-node risk scores. node_risks: list[dict[str, Any]] = [] for node in changed_funcs: - risk = compute_risk_score(store, node) + risk = compute_risk_score(store, node, churn_counts) node_risks.append({ **node_to_dict(node), "risk_score": risk, diff --git a/code_review_graph/cli.py b/code_review_graph/cli.py index 102ad8a8..c6de4b5a 100644 --- a/code_review_graph/cli.py +++ b/code_review_graph/cli.py @@ -635,6 +635,14 @@ def main() -> None: "full JSON. Read-only against the existing graph.", ) detect_cmd.add_argument("--repo", default=None, help="Repository root (auto-detected)") + detect_cmd.add_argument( + "--churn", + action="store_true", + help="Add a change-frequency (churn) term to risk scores, counting " + "commits per file over the last 90 days via git log " + "(CRG_CHURN_WINDOW_DAYS to adjust). Off by default, so " + "structural-only scores are unchanged.", + ) detect_cmd.add_argument( "--verify", action="store_true", @@ -1208,6 +1216,7 @@ def main() -> None: changed, repo_root=str(repo_root), base=base, + include_churn=getattr(args, "churn", False), ) original_tokens = estimate_file_tokens(repo_root, changed) attach_context_savings( diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 893ed352..836dc129 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -325,6 +325,7 @@ code-review-graph detect-changes # Risk-scored change analysis code-review-graph detect-changes --base HEAD~3 # Custom base ref code-review-graph detect-changes --brief # Compact panel with token-savings estimate code-review-graph detect-changes --brief --verify # ...and cross-check vs tiktoken +code-review-graph detect-changes --churn # Add change-frequency (churn) term to risk scores # detect-changes vs update --brief — which one? # • detect-changes --brief: read-only. Asks "what's the impact of my current diff --git a/tests/test_changes.py b/tests/test_changes.py index 34dfdbb3..522a7555 100644 --- a/tests/test_changes.py +++ b/tests/test_changes.py @@ -1,12 +1,15 @@ """Tests for change impact analysis (changes.py).""" +import subprocess import tempfile from pathlib import Path from unittest.mock import patch from code_review_graph.changes import ( + _parse_numstat, _parse_unified_diff, analyze_changes, + compute_file_churn, compute_risk_score, map_changes_to_nodes, parse_git_diff_ranges, @@ -630,3 +633,183 @@ def test_explicit_changed_ranges_not_remapped(self, tmp_path): # No remapping: keys passed through exactly as the caller gave them. assert list(captured["ranges"]) == ["app.py"] assert any(f["name"] == "rel_func" for f in result["changed_functions"]) + + +def _git(repo: Path, *args: str) -> subprocess.CompletedProcess[str]: + """Run a git command inside *repo* and return the result.""" + return subprocess.run( + ["git", *args], + capture_output=True, + text=True, + cwd=str(repo), + timeout=10, + ) + + +class TestFileChurn: + """compute_file_churn: per-file commit counts from git log --numstat.""" + + def test_parse_numstat_counts_commits_per_file(self): + log_text = ( + "3\t1\tsrc/app.py\n" + "10\t0\tsrc/util.py\n" + "\n" + "2\t2\tsrc/app.py\n" + ) + assert _parse_numstat(log_text) == {"src/app.py": 2, "src/util.py": 1} + + def test_parse_numstat_counts_binary_files(self): + log_text = "-\t-\tlogo.png\n5\t0\tapp.py\n" + assert _parse_numstat(log_text) == {"logo.png": 1, "app.py": 1} + + def test_parse_numstat_ignores_non_numstat_lines(self): + log_text = "commit abc123\nAuthor: someone\n\n1\t1\tapp.py\n" + assert _parse_numstat(log_text) == {"app.py": 1} + + def test_compute_file_churn_counts_commits(self, tmp_path): + repo = tmp_path / "repo" + repo.mkdir() + _git(repo, "init") + _git(repo, "config", "user.email", "test@test.com") + _git(repo, "config", "user.name", "Test") + + app = repo / "app.py" + app.write_text("a = 1\n") + _git(repo, "add", "app.py") + _git(repo, "commit", "-m", "one") + + app.write_text("a = 2\n") + util = repo / "util.py" + util.write_text("b = 1\n") + _git(repo, "add", ".") + _git(repo, "commit", "-m", "two") + + counts = compute_file_churn(str(repo)) + assert counts["app.py"] == 2 + assert counts["util.py"] == 1 + + def test_compute_file_churn_non_git_dir_returns_empty(self, tmp_path): + assert compute_file_churn(str(tmp_path)) == {} + + def test_compute_file_churn_subprocess_error_returns_empty(self): + with patch( + "code_review_graph.changes.subprocess.run", + side_effect=OSError("git not found"), + ): + assert compute_file_churn("/tmp") == {} + + def test_compute_file_churn_nonpositive_window_returns_empty(self, tmp_path): + assert compute_file_churn(str(tmp_path), window_days=0) == {} + + +class TestRiskScoreChurn: + """Opt-in churn term in compute_risk_score.""" + + def setup_method(self): + self.tmp = tempfile.NamedTemporaryFile(suffix=".db", delete=False) + self.store = GraphStore(self.tmp.name) + + def teardown_method(self): + self.store.close() + Path(self.tmp.name).unlink(missing_ok=True) + + def _add_func(self, name: str, path: str = "app.py") -> None: + node = NodeInfo( + kind="Function", name=name, file_path=path, + line_start=1, line_end=10, language="python", + ) + self.store.upsert_node(node, file_hash="abc") + self.store.commit() + + def test_churn_counts_increase_risk(self): + self._add_func("hot_func") + node = self.store.get_node("app.py::hot_func") + assert node is not None + base = compute_risk_score(self.store, node) + churned = compute_risk_score(self.store, node, churn_counts={"app.py": 5}) + assert churned > base + + def test_churn_term_saturates(self): + """The churn term maxes out at 0.15 regardless of commit count.""" + self._add_func("hot_func") + node = self.store.get_node("app.py::hot_func") + assert node is not None + base = compute_risk_score(self.store, node) + saturated = compute_risk_score(self.store, node, churn_counts={"app.py": 10}) + extreme = compute_risk_score(self.store, node, churn_counts={"app.py": 1000}) + assert extreme == saturated + assert abs((saturated - base) - 0.15) < 1e-9 + + def test_no_churn_is_backward_compatible(self): + """Omitting churn_counts leaves the structural score unchanged.""" + self._add_func("stable_func") + node = self.store.get_node("app.py::stable_func") + assert node is not None + base = compute_risk_score(self.store, node) + assert compute_risk_score(self.store, node, churn_counts=None) == base + assert compute_risk_score(self.store, node, churn_counts={}) == base + assert compute_risk_score(self.store, node, churn_counts={"other.py": 50}) == base + + +class TestAnalyzeChangesChurn: + """Opt-in churn plumbing through analyze_changes.""" + + def setup_method(self): + self.tmp = tempfile.NamedTemporaryFile(suffix=".db", delete=False) + self.store = GraphStore(self.tmp.name) + + def teardown_method(self): + self.store.close() + Path(self.tmp.name).unlink(missing_ok=True) + + def _add_func_at(self, path: str) -> None: + node = NodeInfo( + kind="Function", name="greet", file_path=path, + line_start=1, line_end=10, language="python", + ) + self.store.upsert_node(node, file_hash="abc") + self.store.commit() + + def test_churn_not_computed_by_default(self, tmp_path): + self._add_func_at("app.py") + with patch("code_review_graph.changes.compute_file_churn") as mock_churn: + analyze_changes( + self.store, + changed_files=["app.py"], + changed_ranges={"app.py": [(1, 10)]}, + repo_root=str(tmp_path), + ) + mock_churn.assert_not_called() + + def test_include_churn_raises_risk_scores(self, tmp_path): + self._add_func_at("app.py") + kwargs = { + "changed_files": ["app.py"], + "changed_ranges": {"app.py": [(1, 10)]}, + "repo_root": str(tmp_path), + } + baseline = analyze_changes(self.store, **kwargs) + with patch( + "code_review_graph.changes.compute_file_churn", + return_value={"app.py": 10}, + ): + churned = analyze_changes(self.store, include_churn=True, **kwargs) + assert churned["risk_score"] > baseline["risk_score"] + assert abs(churned["risk_score"] - baseline["risk_score"] - 0.15) < 1e-9 + + def test_include_churn_matches_absolute_node_paths(self, tmp_path): + """Repo-relative churn keys must match graphs storing absolute paths.""" + abs_path = str(tmp_path / "src" / "app.py") + self._add_func_at(abs_path) + kwargs = { + "changed_files": [abs_path], + "changed_ranges": {abs_path: [(1, 10)]}, + "repo_root": str(tmp_path), + } + baseline = analyze_changes(self.store, **kwargs) + with patch( + "code_review_graph.changes.compute_file_churn", + return_value={"src/app.py": 10}, + ): + churned = analyze_changes(self.store, include_churn=True, **kwargs) + assert churned["risk_score"] > baseline["risk_score"]