Skip to content

Commit 791d5b1

Browse files
anvansterclaude
andcommitted
fix(pr-review): proportionate risk + count example/bench callers as coverage
Two calibration fixes to codegraph_pr_context: - Risk was `total_direct > 20 || untested > 5 → high`, so any sizeable PR tripped "high" purely on caller count — even when every change was body-only and broke nothing. Base risk instead on `breaking_callers` (callers of signature-changed functions) and the untested *ratio* (untested / functions_touched). Surface `breaking_callers` in JSON, the message, and the blast-radius line. - Callers under `examples/` and `benches/` exercise a function at runtime but were counted as breakable production callers, inflating blast radius and leaving example-driven code flagged as untested. Count them as coverage instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 6e707f1 commit 791d5b1

1 file changed

Lines changed: 43 additions & 13 deletions

File tree

crates/codegraph-server/src/mcp/server.rs

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4046,6 +4046,10 @@ impl McpServer {
40464046
let mut all_tests = Vec::new();
40474047
let mut untested_functions = Vec::new();
40484048
let mut total_direct = 0usize;
4049+
// Callers of signature-changed functions — the ones that can
4050+
// actually break. Body-only changes don't break callers, so
4051+
// they shouldn't drive the risk level or blast radius.
4052+
let mut breaking_callers = 0usize;
40494053
let mut all_affected_files = std::collections::HashSet::new();
40504054
let mut func_details = Vec::new();
40514055

@@ -4114,8 +4118,15 @@ impl McpServer {
41144118
|| cname.to_lowercase().contains("_test")
41154119
|| cfile.contains("/tests/")
41164120
|| cfile.contains("/test_");
4117-
4118-
if is_test {
4121+
// Callers under examples/ (and doctests) exercise the
4122+
// function at runtime — count them as coverage, not as
4123+
// breakable production callers. This is what covers code
4124+
// driven only by eval/example harnesses.
4125+
let is_exercising = !is_test
4126+
&& (cfile.contains("/examples/")
4127+
|| cfile.contains("/benches/"));
4128+
4129+
if is_test || is_exercising {
41194130
has_test_caller = true;
41204131
all_tests.push(serde_json::json!({
41214132
"test": cname, "file": cfile, "covers": func_name,
@@ -4134,6 +4145,9 @@ impl McpServer {
41344145
}
41354146
}
41364147
total_direct += caller_count as usize;
4148+
if change_type == "signature_changed" {
4149+
breaking_callers += caller_count as usize;
4150+
}
41374151

41384152
// #87: Test gap — function has no test callers.
41394153
// Skip functions that ARE tests (they don't need
@@ -4292,10 +4306,29 @@ impl McpServer {
42924306

42934307
drop(graph);
42944308

4295-
// Risk level
4296-
let risk_level = if total_direct > 20 || untested_functions.len() > 5 {
4309+
// Functions actually touched by this PR (denominator for ratios).
4310+
let total_functions: u64 = file_impacts
4311+
.iter()
4312+
.map(|f| f["functions_changed"].as_u64().unwrap_or(0))
4313+
.sum();
4314+
4315+
// Risk is driven by what can actually break — callers of
4316+
// signature-changed functions — and by the share of touched
4317+
// functions left untested, NOT by the raw caller count (which
4318+
// body-only changes inflate, e.g. a widely-called helper whose
4319+
// body changed but signature didn't).
4320+
let untested_ratio = if total_functions > 0 {
4321+
untested_functions.len() as f64 / total_functions as f64
4322+
} else {
4323+
0.0
4324+
};
4325+
let risk_level = if breaking_callers > 25
4326+
|| (untested_functions.len() > 5 && untested_ratio > 0.5)
4327+
{
42974328
"high"
4298-
} else if total_direct > 5 || untested_functions.len() > 2 {
4329+
} else if breaking_callers > 8
4330+
|| (untested_functions.len() > 2 && untested_ratio > 0.25)
4331+
{
42994332
"medium"
43004333
} else {
43014334
"low"
@@ -4315,28 +4348,24 @@ impl McpServer {
43154348
commit_prefix, primary_module
43164349
);
43174350

4318-
let total_functions: u64 = file_impacts
4319-
.iter()
4320-
.map(|f| f["functions_changed"].as_u64().unwrap_or(0))
4321-
.sum();
4322-
43234351
let mut result = serde_json::json!({
43244352
"base_branch": base,
43254353
"changed_files": changed_rel.len(),
43264354
"lines_added": lines_added,
43274355
"lines_removed": lines_removed,
43284356
"functions_touched": total_functions,
43294357
"direct_callers": total_direct,
4358+
"breaking_callers": breaking_callers,
43304359
"related_tests": unique_tests.len(),
43314360
"untested_functions": untested_functions.len(),
43324361
"affected_modules": affected_modules,
43334362
"risk_level": risk_level,
43344363
"commit_hint": commit_hint,
43354364
"files": file_impacts,
43364365
"message": format!(
4337-
"PR changes {} files (+{}/-{}, {} functions). {} direct callers, {} tests, {} untested. Risk: {}.",
4366+
"PR changes {} files (+{}/-{}, {} functions). {} direct callers ({} breaking), {} tests, {} untested. Risk: {}.",
43384367
changed_rel.len(), lines_added, lines_removed, total_functions,
4339-
total_direct, unique_tests.len(), untested_functions.len(), risk_level,
4368+
total_direct, breaking_callers, unique_tests.len(), untested_functions.len(), risk_level,
43404369
),
43414370
});
43424371

@@ -4395,9 +4424,10 @@ impl McpServer {
43954424

43964425
if total_direct > 0 {
43974426
md.push_str(&format!(
4398-
"### Blast radius\n{} direct caller{} affected",
4427+
"### Blast radius\n{} direct caller{} affected ({} breaking)",
43994428
total_direct,
44004429
if total_direct == 1 { "" } else { "s" },
4430+
breaking_callers,
44014431
));
44024432
if !affected_modules.is_empty() {
44034433
let mods: Vec<String> = affected_modules

0 commit comments

Comments
 (0)