Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 81 additions & 8 deletions crates/calm-core/src/analysis/boundaries.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ use rusqlite::Connection;
use serde::{Deserialize, Serialize};

/// One declared architecture rule: files under `from` must not import files
/// under `to`. Matching is a simple path-prefix check on project-relative,
/// forward-slash paths (the same format `import_edges.from_path`/`to_path`
/// already use) — deliberately not glob/regex, so a rule reads exactly like
/// the directory boundary it describes and there's no pattern syntax to get
/// wrong. `from`/`to` are directory prefixes (e.g. `"crates/calm-core/src/indexer/"`),
/// not exact file paths.
/// under `to`. `from`/`to` are project-relative, forward-slash paths (the
/// same format `import_edges.from_path`/`to_path` already use). A pattern
/// containing a glob metacharacter (`*`, `?`, `[`) is matched with `globset`
/// against the whole path (e.g. `"crates/*/src/indexer/**"`); otherwise it's
/// treated as a plain directory prefix via `starts_with` (e.g.
/// `"crates/calm-core/src/indexer/"`) — existing prefix-style rules keep
/// working unchanged.
#[derive(Debug, Clone, Deserialize, Serialize)]
pub struct BoundaryRule {
pub from: String,
Expand All @@ -30,6 +31,34 @@ pub struct BoundaryViolation {
/// is reported per (edge, rule) pair — the same edge can violate more than
/// one rule if rules overlap, which is surfaced rather than deduplicated so
/// each rule's own `reason` is visible.
enum PathMatcher {
Prefix(String),
Glob(globset::GlobMatcher),
}

impl PathMatcher {
/// A pattern containing a glob metacharacter is compiled with `globset`;
/// otherwise (including invalid glob syntax) it falls back to a plain
/// `starts_with` prefix check, so existing prefix-style rules are
/// unaffected and a typo'd glob degrades to its literal prefix rather
/// than silently matching nothing.
fn new(pattern: &str) -> Self {
if pattern.contains(['*', '?', '[']) {
if let Ok(glob) = globset::Glob::new(pattern) {
return PathMatcher::Glob(glob.compile_matcher());
}
}
PathMatcher::Prefix(pattern.to_string())
}

fn matches(&self, path: &str) -> bool {
match self {
PathMatcher::Prefix(prefix) => path.starts_with(prefix.as_str()),
PathMatcher::Glob(matcher) => matcher.is_match(path),
}
}
}

pub fn check_boundaries(
conn: &Connection,
rules: &[BoundaryRule],
Expand All @@ -46,10 +75,15 @@ pub fn check_boundaries(
.filter_map(|r| r.ok())
.collect();

let matchers: Vec<(PathMatcher, PathMatcher)> = rules
.iter()
.map(|rule| (PathMatcher::new(&rule.from), PathMatcher::new(&rule.to)))
.collect();

let mut violations = Vec::new();
for (from_path, to_path) in &edges {
for rule in rules {
if from_path.starts_with(&rule.from) && to_path.starts_with(&rule.to) {
for (rule, (from_matcher, to_matcher)) in rules.iter().zip(&matchers) {
if from_matcher.matches(from_path) && to_matcher.matches(to_path) {
violations.push(BoundaryViolation {
from_path: from_path.clone(),
to_path: to_path.clone(),
Expand Down Expand Up @@ -183,4 +217,43 @@ mod tests {
let violations = check_boundaries(&conn, &rules).unwrap();
assert_eq!(violations.len(), 2);
}

#[test]
fn test_glob_rule_matches_nested_paths() {
let conn = test_conn();
insert_import(&conn, "crates/calm-core/src/indexer/foo.rs", "crates/calm-server/src/tools/orient.rs");
let rules = vec![BoundaryRule {
from: "crates/*/src/indexer/**".into(),
to: "crates/*/src/tools/**".into(),
reason: "indexer must not import server tools".into(),
}];
let violations = check_boundaries(&conn, &rules).unwrap();
assert_eq!(violations.len(), 1);
}

#[test]
fn test_glob_rule_does_not_match_unrelated_path() {
let conn = test_conn();
insert_import(&conn, "crates/calm-core/src/other/foo.rs", "crates/calm-server/src/tools/orient.rs");
let rules = vec![BoundaryRule {
from: "crates/*/src/indexer/**".into(),
to: "crates/*/src/tools/**".into(),
reason: "indexer must not import server tools".into(),
}];
let violations = check_boundaries(&conn, &rules).unwrap();
assert!(violations.is_empty());
}

#[test]
fn test_invalid_glob_falls_back_to_literal_prefix() {
let conn = test_conn();
insert_import(&conn, "a/[unclosed/x.py", "b/y.py");
let rules = vec![BoundaryRule {
from: "a/[unclosed".into(),
to: "b/".into(),
reason: "invalid glob syntax degrades to prefix match".into(),
}];
let violations = check_boundaries(&conn, &rules).unwrap();
assert_eq!(violations.len(), 1);
}
}
2 changes: 1 addition & 1 deletion crates/calm-core/src/analysis/coverage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ impl CoverageData {
}
}

const COVERAGE_SEARCH_PATHS: &[(&str, &str)] = &[
pub const COVERAGE_SEARCH_PATHS: &[(&str, &str)] = &[
("lcov.info", "lcov"),
("coverage/lcov.info", "lcov"),
(".nyc_output/lcov.info", "lcov"),
Expand Down
144 changes: 142 additions & 2 deletions crates/calm-core/src/analysis/hotspot.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,31 @@ use crate::config::HotspotsConfig;
#[derive(Debug, Clone)]
pub struct ChurnInfo {
pub commit_count: i64,
/// Commits whose author looks like a bot account (see `is_bot_author`) —
/// counted separately from `commit_count` so `churn_source` can tell
/// human-driven churn from CI/dependency-bump noise.
pub bot_commit_count: i64,
pub authors: HashSet<String>,
pub last_changed: Option<String>,
}

impl ChurnInfo {
/// "unknown" with no churn data at all (e.g. git unavailable),
/// "human" when no commit came from a bot account, "bot_dominated" when
/// every commit did, "mixed" otherwise.
pub fn churn_source(&self) -> &'static str {
if self.commit_count == 0 {
"unknown"
} else if self.bot_commit_count == 0 {
"human"
} else if self.bot_commit_count >= self.commit_count {
"bot_dominated"
} else {
"mixed"
}
}
}

#[derive(Debug, Clone)]
pub struct ComplexityInfo {
pub symbol_count: i64,
Expand Down Expand Up @@ -41,6 +62,13 @@ pub struct HotspotEntry {
pub language: String,
pub churn: ChurnInfo,
pub complexity: ComplexityInfo,
/// Churn share (0-1) of `hotspot_score`'s numerator, normalized against
/// the busiest candidate file this run — 0.0 when git is unavailable
/// (no churn signal at all, not "no churn").
pub norm_churn: f64,
/// Complexity share (0-1) of `hotspot_score`'s numerator, normalized
/// against the most complex candidate file this run.
pub norm_compl: f64,
pub hotspot_score: f64,
pub risk_level: String,
pub top_symbols: Option<Vec<HotspotSymbol>>,
Expand Down Expand Up @@ -87,6 +115,7 @@ pub fn compute_hotspots(
path.clone(),
ChurnInfo {
commit_count: 0,
bot_commit_count: 0,
authors: HashSet::new(),
last_changed: None,
},
Expand Down Expand Up @@ -141,10 +170,13 @@ pub fn compute_hotspots(
.iter()
.filter_map(|(path, churn)| {
let cm = complexity_map.get(path)?;
// Computed unconditionally (not just when `git_available`) so
// callers can see the churn share even when it didn't factor
// into `hotspot_score` — 0.0 uniformly when git is unavailable,
// since every candidate's `commit_count` is 0 in that branch.
let norm_churn = churn_scores.get(path.as_str()).copied().unwrap_or(0.0) / max_churn;
let norm_compl = compl_scores.get(path.as_str()).copied().unwrap_or(0.0) / max_compl;
let score = if git_available {
let norm_churn =
churn_scores.get(path.as_str()).copied().unwrap_or(0.0) / max_churn;
norm_churn * norm_compl
} else {
norm_compl
Expand All @@ -165,6 +197,8 @@ pub fn compute_hotspots(
language: cm.language.clone(),
churn: churn.clone(),
complexity: cm.clone(),
norm_churn: (norm_churn * 10000.0).round() / 10000.0,
norm_compl: (norm_compl * 10000.0).round() / 10000.0,
hotspot_score: (score * 10000.0).round() / 10000.0,
risk_level: risk_level.to_string(),
top_symbols: None,
Expand Down Expand Up @@ -283,19 +317,31 @@ fn collect_git_churn(project_root: &Path, since: &str) -> (HashMap<String, Churn
for path in &commit.files {
let entry = churn_map.entry(path.clone()).or_insert_with(|| ChurnInfo {
commit_count: 0,
bot_commit_count: 0,
authors: HashSet::new(),
// H-1 fix: last_changed is Option<String>, None instead of ""
last_changed: commit.date.clone(),
});
entry.commit_count += 1;
if let Some(ref author) = commit.author {
entry.authors.insert(author.clone());
if is_bot_author(author) {
entry.bot_commit_count += 1;
}
}
}
}
(churn_map, true)
}

/// GitHub's bot accounts (dependabot[bot], renovate[bot], github-actions[bot],
/// etc.) all carry `[bot]` in their noreply author email (`%ae`), which is
/// what `commits_with_files` captures — this substring check covers them
/// without hardcoding a specific bot allowlist.
fn is_bot_author(author: &str) -> bool {
author.contains("[bot]")
}

fn collect_complexity(conn: &Connection) -> HashMap<String, ComplexityInfo> {
// is_test = 0: test functions structurally call production code, so they
// almost always end up with coreness > 0 despite never being called
Expand Down Expand Up @@ -512,6 +558,100 @@ mod tests {
assert_eq!(output.hotspots[0].churn.commit_count, 2);
}

#[test]
fn test_churn_source_classifies_human_bot_mixed_and_unknown() {
let unknown = ChurnInfo {
commit_count: 0,
bot_commit_count: 0,
authors: HashSet::new(),
last_changed: None,
};
assert_eq!(unknown.churn_source(), "unknown");

let human = ChurnInfo {
commit_count: 3,
bot_commit_count: 0,
authors: HashSet::new(),
last_changed: None,
};
assert_eq!(human.churn_source(), "human");

let bot_dominated = ChurnInfo {
commit_count: 3,
bot_commit_count: 3,
authors: HashSet::new(),
last_changed: None,
};
assert_eq!(bot_dominated.churn_source(), "bot_dominated");

let mixed = ChurnInfo {
commit_count: 4,
bot_commit_count: 1,
authors: HashSet::new(),
last_changed: None,
};
assert_eq!(mixed.churn_source(), "mixed");
}

#[test]
fn test_is_bot_author_matches_bot_suffix() {
assert!(is_bot_author(
"49699333+dependabot[bot]@users.noreply.github.com"
));
assert!(is_bot_author(
"29139614+renovate[bot]@users.noreply.github.com"
));
assert!(!is_bot_author("jane@example.com"));
}

#[test]
fn test_collect_git_churn_tallies_bot_commits_separately() {
let dir = tempfile::tempdir().unwrap();
run_git(dir.path(), &["init", "-q"]);
run_git(dir.path(), &["config", "user.email", "human@example.com"]);
run_git(dir.path(), &["config", "user.name", "Human"]);
std::fs::write(dir.path().join("f.py"), "x = 1\n").unwrap();
run_git(dir.path(), &["add", "f.py"]);
run_git(dir.path(), &["commit", "-q", "-m", "init"]);

run_git(
dir.path(),
&[
"config",
"user.email",
"49699333+dependabot[bot]@users.noreply.github.com",
],
);
run_git(dir.path(), &["config", "user.name", "dependabot[bot]"]);
std::fs::write(dir.path().join("f.py"), "x = 2\n").unwrap();
run_git(dir.path(), &["commit", "-q", "-am", "bump"]);

let (churn_map, git_available) = collect_git_churn(dir.path(), "1 year");
assert!(git_available);
let entry = churn_map.get("f.py").unwrap();
assert_eq!(entry.commit_count, 2);
assert_eq!(entry.bot_commit_count, 1);
assert_eq!(entry.churn_source(), "mixed");
}

#[test]
fn test_norm_compl_and_norm_churn_are_exposed_without_git() {
let conn = setup_db();
let dir = tempfile::tempdir().unwrap();
insert_symbol(&conn, "a.func1", "/a.py", 0, false, 0);
insert_symbol(&conn, "b.func1", "/b.py", 10, true, 5);
insert_symbol(&conn, "b.func2", "/b.py", 2, false, 1);

let config = HotspotsConfig::default();
let output = compute_hotspots(dir.path(), &conn, &config, 10, "6 months ago", 2, false);
assert!(!output.git_available);
// No churn signal at all when git is unavailable.
assert!(output.hotspots.iter().all(|h| h.norm_churn == 0.0));
// b.py is the most complex candidate, so its complexity share is 1.0.
let b = output.hotspots.iter().find(|h| h.path == "/b.py").unwrap();
assert_eq!(b.norm_compl, 1.0);
}

/// Regression: `compute_absolute_hotspot_risk` must NOT saturate to
/// ~1.0 just because one file is *relatively* the busiest in a small,
/// healthy repo — unlike `compute_hotspots`' `hotspot_score` (min-max
Expand Down
Loading
Loading