From 624ce682eeecc61673928bc80008aef98d9cfba8 Mon Sep 17 00:00:00 2001 From: Aji Anaz Date: Wed, 10 Jun 2026 23:58:00 +0700 Subject: [PATCH 1/3] feat: add debt tracker module for cumulative review history (Phase 1 of #206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - New src/engine/debt_tracker.rs with DebtSnapshot, DebtReport, DebtConfig - Auto-save review snapshots to .cora/history/ after each review - Category mapping from LLM issue_type to tracking groups - Quality score calculation from severity distribution - Trend analysis (improving/stable/worsening) across snapshots - Config section 'debt:' in .cora.yaml (enabled, history_dir, retention_days) - Best-effort snapshot save — never fails review - 32 unit tests covering all debt_tracker logic - Hook integrated in execute_review and execute_chunked_review - All 485 tests pass, clippy clean, fmt clean --- src/commands/review.rs | 52 ++ src/config/schema.rs | 9 + src/engine/debt_tracker.rs | 976 +++++++++++++++++++++++++++++++++++++ src/engine/mod.rs | 1 + 4 files changed, 1038 insertions(+) create mode 100644 src/engine/debt_tracker.rs diff --git a/src/commands/review.rs b/src/commands/review.rs index 307a846e..6265b114 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -224,6 +224,21 @@ pub async fn execute_review( None }; + // 8b. Save debt tracking snapshot (best-effort, never fails review) + if config.debt.enabled { + let (commit, branch) = get_git_context(); + let snapshot = crate::engine::debt_tracker::DebtSnapshot::from_review( + &filtered_response.issues, + gate_result.as_ref(), + commit, + branch, + 0, // files_reviewed not tracked per-review yet + None, + None, // duration not passed through yet + ); + crate::engine::debt_tracker::save_snapshot(&snapshot, config.debt.history_dir.as_deref()); + } + // 9. Return exit code let exit_code = if gate_result .as_ref() @@ -259,6 +274,28 @@ pub async fn execute_review( Ok(ReviewResult { exit_code, output }) } +/// Helper to get git commit and branch context for debt tracking. +/// Returns (Some(commit), Some(branch)) if in a git repo, (None, None) otherwise. +fn get_git_context() -> (Option, Option) { + let commit = std::process::Command::new("git") + .args(["rev-parse", "--short", "HEAD"]) + .output() + .ok() + .and_then(|o| { + if o.status.success() { + String::from_utf8(o.stdout) + .ok() + .map(|s| s.trim().to_string()) + } else { + None + } + }); + + let branch = crate::git::get_current_branch().ok(); + + (commit, branch) +} + /// Get the diff based on the provided options. fn get_diff(opts: &ReviewOptions, _config: &Config) -> Result { if let Some(ref diff_file) = opts.diff_file { @@ -514,6 +551,21 @@ async fn execute_chunked_review( None }; + // 8b. Save debt tracking snapshot (best-effort, never fails review) + if config.debt.enabled { + let (commit, branch) = get_git_context(); + let snapshot = crate::engine::debt_tracker::DebtSnapshot::from_review( + &filtered_response.issues, + gate_result.as_ref(), + commit, + branch, + chunks.iter().map(|c| c.file_count).sum(), + None, + None, + ); + crate::engine::debt_tracker::save_snapshot(&snapshot, config.debt.history_dir.as_deref()); + } + // 9. Return exit code let exit_code = if gate_result .as_ref() diff --git a/src/config/schema.rs b/src/config/schema.rs index 185a937e..51e79e01 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -47,6 +47,8 @@ pub struct Config { pub context_chain: crate::engine::context::types::ContextConfig, /// File bundling configuration for scan/review grouping. pub bundling: BundlingConfig, + /// Debt tracking configuration. + pub debt: crate::engine::debt_tracker::DebtConfig, /// Active quality profile (resolved from .cora.yaml). pub profile: Option, } @@ -130,6 +132,7 @@ impl Default for Config { rules_config: RulesConfig::default(), context_chain: crate::engine::context::types::ContextConfig::default(), bundling: BundlingConfig::default(), + debt: crate::engine::debt_tracker::DebtConfig::default(), profile: None, } } @@ -177,6 +180,8 @@ pub struct CoraFile { pub rules_engine: Option, #[serde(skip_serializing_if = "Option::is_none")] pub bundling: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub debt: Option, #[serde(default)] pub profile: Option, } @@ -466,6 +471,10 @@ impl CoraFile { config.bundling.coalesce_by_language = v; } } + // Merge debt tracking config + if let Some(debt) = &self.debt { + config.debt = debt.clone(); + } // Resolve profile — load built-in or custom, merge into config if let Some(profile_ref) = &self.profile { match crate::engine::profiles::resolve_profile(profile_ref) { diff --git a/src/engine/debt_tracker.rs b/src/engine/debt_tracker.rs new file mode 100644 index 00000000..bf887f28 --- /dev/null +++ b/src/engine/debt_tracker.rs @@ -0,0 +1,976 @@ +//! Debt Tracker — aggregates review findings across runs and tracks quality trends. +//! +//! Stores lightweight JSON snapshots in `.cora/history/` after each review. +//! Provides aggregation and trend analysis across multiple reviews. + +use crate::engine::quality_gate::GateResult; +use crate::engine::types::{ReviewIssue, Severity}; +use chrono::{DateTime, Utc}; +use serde::{Deserialize, Serialize}; +use std::collections::HashMap; +use std::path::PathBuf; +use tracing::{debug, warn}; + +// ─── Default config values ─── + +const DEFAULT_HISTORY_DIR: &str = ".cora/history"; +const DEFAULT_RETENTION_DAYS: u64 = 90; + +// ─── Category mapping ─── + +/// Map raw `issue_type` strings from LLM output to normalized tracking categories. +fn normalize_category(raw: &str) -> &'static str { + let lower = raw.to_lowercase(); + match lower.as_str() { + "security" | "vulnerability" | "injection" | "xss" | "csrf" | "sqli" => "security", + "bug" | "bugs" | "logic" | "correctness" | "bug_risk" => "bug_risk", + "performance" | "perf" | "memory" | "speed" | "n+1" => "performance", + "error" | "panic" | "unwrap" | "error_handling" => "error_handling", + "complexity" | "cognitive" | "complex" => "complexity", + "style" | "naming" | "formatting" => "style", + "best_practice" | "best-practice" | "bestpractice" => "best_practice", + _ => "other", + } +} + +/// Build category → count map from a list of issues. +fn count_by_category(issues: &[ReviewIssue]) -> HashMap { + let mut counts = HashMap::new(); + for issue in issues { + let cat = issue + .issue_type + .as_deref() + .map(normalize_category) + .unwrap_or("other"); + *counts.entry(cat.to_string()).or_insert(0) += 1; + } + counts +} + +/// Build severity → count map from a list of issues. +fn count_by_severity(issues: &[ReviewIssue]) -> HashMap { + let mut counts = HashMap::new(); + for issue in issues { + let sev = issue.severity.to_string(); + *counts.entry(sev).or_insert(0) += 1; + } + counts +} + +// ─── DebtSnapshot — one file per review run ─── + +/// A single review snapshot stored in `.cora/history/`. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct DebtSnapshot { + /// ISO-8601 timestamp of the review. + pub timestamp: DateTime, + /// Git commit hash (short). + pub commit: Option, + /// Git branch name. + pub branch: Option, + /// Number of files reviewed. + pub files_reviewed: usize, + /// Number of lines reviewed (if known). + pub lines_reviewed: Option, + /// Finding counts by severity. + pub findings: HashMap, + /// Finding counts by category. + pub categories: HashMap, + /// Quality score 0-10 (derived from severity distribution). + pub quality_score: f64, + /// Quality gate status: "passed", "failed", or "disabled". + pub gate_status: String, + /// Review duration in milliseconds. + pub duration_ms: Option, +} + +impl DebtSnapshot { + /// Build a snapshot from review results. + /// + /// `files_reviewed` and `lines_reviewed` are best-effort (may be 0/None). + /// `duration_ms` is the wall-clock time for the review call. + pub fn from_review( + issues: &[ReviewIssue], + gate_result: Option<&GateResult>, + commit: Option, + branch: Option, + files_reviewed: usize, + lines_reviewed: Option, + duration_ms: Option, + ) -> Self { + let findings = count_by_severity(issues); + let categories = count_by_category(issues); + let quality_score = calculate_quality_score(issues); + let gate_status = gate_result + .map(|g| g.status.to_string().to_lowercase()) + .unwrap_or_else(|| "disabled".to_string()); + + Self { + timestamp: Utc::now(), + commit, + branch, + files_reviewed, + lines_reviewed, + findings, + categories, + quality_score, + gate_status, + duration_ms, + } + } +} + +/// Calculate a quality score from 0-10 based on issue severities. +/// +/// 10 = no issues. Each finding reduces the score: +/// - critical: -2.0 +/// - major: -1.0 +/// - minor: -0.3 +/// - info: -0.1 +fn calculate_quality_score(issues: &[ReviewIssue]) -> f64 { + let mut score: f64 = 10.0; + for issue in issues { + let penalty: f64 = match issue.severity { + Severity::Critical => 2.0, + Severity::Major => 1.0, + Severity::Minor => 0.3, + Severity::Info => 0.1, + }; + score -= penalty; + } + if score < 0.0 { 0.0 } else { score } +} + +// ─── DebtReport — aggregated across snapshots ─── + +/// Aggregated debt report from multiple snapshots. +#[allow(dead_code)] +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct DebtReport { + /// Number of reviews analyzed. + pub reviews_analyzed: usize, + /// Total findings across all reviews. + pub total_findings: usize, + /// Change in total findings vs previous period (negative = improving). + pub change_from_previous: i64, + /// Overall trend: "improving", "stable", or "worsening". + pub trend: String, + /// Average quality score across reviews. + pub quality_score_avg: f64, + /// Change in quality score vs previous period. + pub quality_score_change: f64, + /// Aggregated findings by severity. + pub findings: HashMap, + /// Per-category breakdown with trend. + pub categories: Vec, + /// Earliest snapshot timestamp. + pub period_start: Option>, + /// Latest snapshot timestamp. + pub period_end: Option>, +} + +/// Per-category report with trend indicator. +#[allow(dead_code)] +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct CategoryReport { + /// Category name. + pub name: String, + /// Total count across all reviews. + pub count: usize, + /// Change vs previous period. + pub change: i64, + /// Trend: "improving", "stable", "worsening". + pub trend: String, +} + +// ─── Trend calculation ─── + +/// Trend direction based on change value. +/// Trend direction based on change value. +#[allow(dead_code)] +fn trend_from_change(change: i64) -> &'static str { + if change < -1 { + "improving" + } else if change > 1 { + "worsening" + } else { + "stable" + } +} + +// ─── File I/O ─── + +/// Generate a filename for a snapshot: `YYYY-MM-DD_HHMMSS_{short_hash}.json` +fn snapshot_filename(snapshot: &DebtSnapshot) -> String { + let date = snapshot.timestamp.format("%Y-%m-%d_%H%M%S"); + let hash = snapshot.commit.as_deref().unwrap_or("unknown"); + // Take first 7 chars of commit hash + let short_hash = if hash.len() > 7 { &hash[..7] } else { hash }; + format!("{date}_{short_hash}.json") +} + +/// Resolve the history directory path. +/// Checks for `.cora/history` relative to CWD. +fn history_dir(custom_dir: Option<&str>) -> PathBuf { + if let Some(dir) = custom_dir { + PathBuf::from(dir) + } else { + PathBuf::from(DEFAULT_HISTORY_DIR) + } +} + +/// Save a snapshot to the history directory. +/// +/// Creates the directory if it doesn't exist. Best-effort — errors are logged +/// but don't fail the review. +pub fn save_snapshot(snapshot: &DebtSnapshot, custom_dir: Option<&str>) { + let dir = history_dir(custom_dir); + + if let Err(e) = std::fs::create_dir_all(&dir) { + warn!("failed to create history directory {}: {e}", dir.display()); + return; + } + + let filename = snapshot_filename(snapshot); + let path = dir.join(&filename); + + match serde_json::to_string_pretty(snapshot) { + Ok(json) => { + if let Err(e) = std::fs::write(&path, json) { + warn!("failed to write snapshot {}: {e}", path.display()); + } else { + debug!("saved debt snapshot: {}", path.display()); + } + } + Err(e) => { + warn!("failed to serialize snapshot: {e}"); + } + } +} + +/// Load all snapshots from the history directory. +/// +/// Returns snapshots sorted by timestamp (oldest first). +/// Malformed files are skipped with a warning. +/// Load all snapshots from the history directory. +/// +/// Returns snapshots sorted by timestamp (oldest first). +/// Malformed files are skipped with a warning. +#[allow(dead_code)] +pub fn load_snapshots(custom_dir: Option<&str>) -> Vec { + let dir = history_dir(custom_dir); + + if !dir.is_dir() { + return Vec::new(); + } + + let mut snapshots = Vec::new(); + + if let Ok(entries) = std::fs::read_dir(&dir) { + for entry in entries.flatten() { + let path = entry.path(); + if path.extension().is_some_and(|ext| ext == "json") { + match std::fs::read_to_string(&path) { + Ok(content) => match serde_json::from_str::(&content) { + Ok(snapshot) => snapshots.push(snapshot), + Err(e) => { + warn!("skipping malformed snapshot {}: {e}", path.display()); + } + }, + Err(e) => { + warn!("failed to read snapshot {}: {e}", path.display()); + } + } + } + } + } + + // Sort oldest first + snapshots.sort_by_key(|s| s.timestamp); + snapshots +} + +/// Clean up snapshots older than `retention_days`. +/// +/// Returns the number of files removed. +/// Clean up snapshots older than `retention_days`. +/// +/// Returns the number of files removed. +#[allow(dead_code)] +pub fn cleanup_old_snapshots(custom_dir: Option<&str>, retention_days: u64) -> usize { + let dir = history_dir(custom_dir); + + if !dir.is_dir() { + return 0; + } + + let cutoff = Utc::now() - chrono::Duration::days(retention_days as i64); + let mut removed = 0; + + if let Ok(entries) = std::fs::read_dir(&dir) { + for entry in entries.flatten() { + let path = entry.path(); + if path.extension().is_some_and(|ext| ext == "json") { + if let Ok(content) = std::fs::read_to_string(&path) { + if let Ok(snapshot) = serde_json::from_str::(&content) { + if snapshot.timestamp < cutoff && std::fs::remove_file(&path).is_ok() { + removed += 1; + debug!("removed old snapshot: {}", path.display()); + } + } + } + } + } + } + + removed +} + +// ─── Aggregation ─── + +/// Aggregate snapshots into a report. +/// +/// If there are enough snapshots, splits into "recent" and "previous" halves +/// to calculate trends. Otherwise, reports totals with "stable" trends. +/// Aggregate snapshots into a report. +/// +/// If there are enough snapshots, splits into "recent" and "previous" halves +/// to calculate trends. Otherwise, reports totals with "stable" trends. +#[allow(dead_code)] +pub fn aggregate(snapshots: &[DebtSnapshot]) -> DebtReport { + if snapshots.is_empty() { + return DebtReport { + reviews_analyzed: 0, + total_findings: 0, + change_from_previous: 0, + trend: "stable".to_string(), + quality_score_avg: 0.0, + quality_score_change: 0.0, + findings: HashMap::new(), + categories: Vec::new(), + period_start: None, + period_end: None, + }; + } + + let period_start = snapshots.first().map(|s| s.timestamp); + let period_end = snapshots.last().map(|s| s.timestamp); + + // Calculate totals across all snapshots + let mut total_findings = 0; + let mut all_findings: HashMap = HashMap::new(); + let mut all_categories: HashMap = HashMap::new(); + let mut quality_scores: Vec = Vec::new(); + + for snapshot in snapshots { + let snapshot_total: usize = snapshot.findings.values().sum(); + total_findings += snapshot_total; + + for (sev, count) in &snapshot.findings { + *all_findings.entry(sev.clone()).or_insert(0) += count; + } + + for (cat, count) in &snapshot.categories { + *all_categories.entry(cat.clone()).or_insert(0) += count; + } + + quality_scores.push(snapshot.quality_score); + } + + let quality_score_avg = quality_scores.iter().sum::() / quality_scores.len() as f64; + + // Split for trend calculation (half-half) + let mid = snapshots.len() / 2; + let (previous, recent): (&[_], &[_]) = if snapshots.len() >= 2 { + (&snapshots[..mid.max(1)], &snapshots[mid.max(1)..]) + } else { + (&[], snapshots) + }; + + let previous_total: usize = previous + .iter() + .map(|s| s.findings.values().sum::()) + .sum(); + let recent_total: usize = recent + .iter() + .map(|s| s.findings.values().sum::()) + .sum(); + + let change_from_previous = recent_total as i64 - previous_total as i64; + let trend = trend_from_change(change_from_previous).to_string(); + + let previous_avg_quality = if previous.is_empty() { + quality_score_avg + } else { + previous.iter().map(|s| s.quality_score).sum::() / previous.len() as f64 + }; + let quality_score_change = quality_score_avg - previous_avg_quality; + + // Per-category with trend + let mut category_reports = Vec::new(); + let mut previous_categories: HashMap = HashMap::new(); + for snap in previous { + for (cat, count) in &snap.categories { + *previous_categories.entry(cat.clone()).or_insert(0) += count; + } + } + + // Collect all unique category names + let mut all_cat_names: Vec = all_categories.keys().cloned().collect(); + all_cat_names.sort(); + + for cat_name in &all_cat_names { + let count = all_categories.get(cat_name).copied().unwrap_or(0); + let prev_count = previous_categories.get(cat_name).copied().unwrap_or(0); + let change = count as i64 - prev_count as i64; + + category_reports.push(CategoryReport { + name: cat_name.clone(), + count, + change, + trend: trend_from_change(change).to_string(), + }); + } + + DebtReport { + reviews_analyzed: snapshots.len(), + total_findings, + change_from_previous, + trend, + quality_score_avg, + quality_score_change, + findings: all_findings, + categories: category_reports, + period_start, + period_end, + } +} + +// ─── Debt config ─── + +/// Debt tracking configuration — parsed from `.cora.yaml` under `debt`. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct DebtConfig { + /// Enable debt tracking (default: true). + #[serde(default = "default_true")] + pub enabled: bool, + + /// Directory to store history snapshots (default: `.cora/history`). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub history_dir: Option, + + /// Auto-cleanup snapshots older than N days (default: 90). + #[serde(default = "default_retention_days")] + pub retention_days: u64, +} + +fn default_true() -> bool { + true +} + +impl Default for DebtConfig { + fn default() -> Self { + Self { + enabled: true, + history_dir: None, + retention_days: DEFAULT_RETENTION_DAYS, + } + } +} + +fn default_retention_days() -> u64 { + DEFAULT_RETENTION_DAYS +} + +impl DebtConfig { + /// Get the effective history directory. + #[allow(dead_code)] + pub fn history_dir(&self) -> &str { + self.history_dir.as_deref().unwrap_or(DEFAULT_HISTORY_DIR) + } + + /// Get the effective retention days. + #[allow(dead_code)] + pub fn retention_days(&self) -> u64 { + self.retention_days + } +} + +// ─── Tests ─── + +#[cfg(test)] +mod tests { + use super::*; + + fn make_issue(severity: Severity, issue_type: &str) -> ReviewIssue { + ReviewIssue { + file: "test.rs".to_string(), + line: Some(1), + severity, + issue_type: Some(issue_type.to_string()), + title: "test issue".to_string(), + body: String::new(), + suggested_fix: None, + } + } + + // ─── normalize_category ─── + + #[test] + fn normalize_security() { + assert_eq!(normalize_category("security"), "security"); + assert_eq!(normalize_category("vulnerability"), "security"); + assert_eq!(normalize_category("injection"), "security"); + assert_eq!(normalize_category("Security"), "security"); + } + + #[test] + fn normalize_bug_risk() { + assert_eq!(normalize_category("bug"), "bug_risk"); + assert_eq!(normalize_category("bugs"), "bug_risk"); + assert_eq!(normalize_category("logic"), "bug_risk"); + assert_eq!(normalize_category("Bug"), "bug_risk"); + } + + #[test] + fn normalize_performance() { + assert_eq!(normalize_category("performance"), "performance"); + assert_eq!(normalize_category("perf"), "performance"); + assert_eq!(normalize_category("memory"), "performance"); + } + + #[test] + fn normalize_error_handling() { + assert_eq!(normalize_category("error"), "error_handling"); + assert_eq!(normalize_category("panic"), "error_handling"); + assert_eq!(normalize_category("unwrap"), "error_handling"); + assert_eq!(normalize_category("error_handling"), "error_handling"); + } + + #[test] + fn normalize_best_practice() { + assert_eq!(normalize_category("best_practice"), "best_practice"); + assert_eq!(normalize_category("best-practice"), "best_practice"); + assert_eq!(normalize_category("bestpractice"), "best_practice"); + } + + #[test] + fn normalize_unknown() { + assert_eq!(normalize_category("something_else"), "other"); + assert_eq!(normalize_category("random"), "other"); + } + + // ─── count_by_category ─── + + #[test] + fn count_categories_empty() { + let counts = count_by_category(&[]); + assert!(counts.is_empty()); + } + + #[test] + fn count_categories_mixed() { + let issues = vec![ + make_issue(Severity::Critical, "security"), + make_issue(Severity::Major, "security"), + make_issue(Severity::Major, "performance"), + make_issue(Severity::Minor, "bug"), + make_issue(Severity::Info, "style"), + ]; + let counts = count_by_category(&issues); + assert_eq!(counts.get("security"), Some(&2)); + assert_eq!(counts.get("performance"), Some(&1)); + assert_eq!(counts.get("bug_risk"), Some(&1)); + assert_eq!(counts.get("style"), Some(&1)); + } + + // ─── count_by_severity ─── + + #[test] + fn count_severities() { + let issues = vec![ + make_issue(Severity::Critical, "security"), + make_issue(Severity::Critical, "bug"), + make_issue(Severity::Major, "performance"), + make_issue(Severity::Minor, "style"), + make_issue(Severity::Info, "suggestion"), + ]; + let counts = count_by_severity(&issues); + assert_eq!(counts.get("critical"), Some(&2)); + assert_eq!(counts.get("major"), Some(&1)); + assert_eq!(counts.get("minor"), Some(&1)); + assert_eq!(counts.get("info"), Some(&1)); + } + + // ─── calculate_quality_score ─── + + #[test] + fn quality_score_no_issues() { + assert!((calculate_quality_score(&[]) - 10.0).abs() < f64::EPSILON); + } + + #[test] + fn quality_score_one_critical() { + let issues = vec![make_issue(Severity::Critical, "security")]; + assert!((calculate_quality_score(&issues) - 8.0).abs() < f64::EPSILON); + } + + #[test] + fn quality_score_one_major() { + let issues = vec![make_issue(Severity::Major, "bug")]; + assert!((calculate_quality_score(&issues) - 9.0).abs() < f64::EPSILON); + } + + #[test] + fn quality_score_one_minor() { + let issues = vec![make_issue(Severity::Minor, "style")]; + assert!((calculate_quality_score(&issues) - 9.7).abs() < f64::EPSILON); + } + + #[test] + fn quality_score_one_info() { + let issues = vec![make_issue(Severity::Info, "suggestion")]; + assert!((calculate_quality_score(&issues) - 9.9).abs() < f64::EPSILON); + } + + #[test] + fn quality_score_clamped_at_zero() { + let issues: Vec = (0..6) + .map(|_| make_issue(Severity::Critical, "security")) + .collect(); + assert!((calculate_quality_score(&issues) - 0.0).abs() < f64::EPSILON); + } + + // ─── DebtSnapshot::from_review ─── + + #[test] + fn snapshot_from_empty_review() { + let snap = DebtSnapshot::from_review( + &[], + None, + Some("abc1234".to_string()), + Some("main".to_string()), + 5, + Some(200), + Some(1500), + ); + assert!(snap.findings.is_empty()); + assert!(snap.categories.is_empty()); + assert!((snap.quality_score - 10.0).abs() < f64::EPSILON); + assert_eq!(snap.gate_status, "disabled"); + assert_eq!(snap.commit.as_deref(), Some("abc1234")); + assert_eq!(snap.branch.as_deref(), Some("main")); + assert_eq!(snap.files_reviewed, 5); + assert_eq!(snap.lines_reviewed, Some(200)); + assert_eq!(snap.duration_ms, Some(1500)); + } + + #[test] + fn snapshot_from_review_with_issues() { + let issues = vec![ + make_issue(Severity::Critical, "security"), + make_issue(Severity::Major, "performance"), + make_issue(Severity::Minor, "bug"), + ]; + let snap = DebtSnapshot::from_review( + &issues, + None, + Some("def5678".to_string()), + None, + 3, + None, + None, + ); + assert_eq!(snap.findings.get("critical"), Some(&1)); + assert_eq!(snap.findings.get("major"), Some(&1)); + assert_eq!(snap.findings.get("minor"), Some(&1)); + assert_eq!(snap.categories.get("security"), Some(&1)); + assert_eq!(snap.categories.get("performance"), Some(&1)); + assert_eq!(snap.categories.get("bug_risk"), Some(&1)); + } + + #[test] + fn snapshot_with_gate_result() { + use crate::engine::quality_gate::{GateStatus, SeverityCounts}; + + let gate = crate::engine::quality_gate::GateResult { + status: GateStatus::Fail, + checks: vec![], + severity_counts: SeverityCounts::default(), + category_counts: HashMap::new(), + total_findings: 1, + }; + let snap = DebtSnapshot::from_review( + &[make_issue(Severity::Critical, "security")], + Some(&gate), + None, + None, + 1, + None, + None, + ); + assert_eq!(snap.gate_status, "failed"); + } + + // ─── DebtSnapshot serialization ─── + + #[test] + fn snapshot_json_roundtrip() { + let snap = DebtSnapshot::from_review( + &[make_issue(Severity::Major, "performance")], + None, + Some("abc1234".to_string()), + Some("develop".to_string()), + 10, + Some(500), + Some(3000), + ); + let json = serde_json::to_string(&snap).unwrap(); + let back: DebtSnapshot = serde_json::from_str(&json).unwrap(); + assert_eq!(back.commit, snap.commit); + assert_eq!(back.branch, snap.branch); + assert_eq!(back.files_reviewed, snap.files_reviewed); + assert_eq!(back.lines_reviewed, snap.lines_reviewed); + assert!((back.quality_score - snap.quality_score).abs() < f64::EPSILON); + assert_eq!(back.gate_status, snap.gate_status); + } + + // ─── snapshot_filename ─── + + #[test] + fn snapshot_filename_format() { + let snap = DebtSnapshot::from_review( + &[], + None, + Some("abcdef1234567890".to_string()), + None, + 0, + None, + None, + ); + let name = snapshot_filename(&snap); + // Should be like: 2026-06-10_123456_abcdef1.json + assert!(name.starts_with("2026")); + assert!(name.contains("_abcdef1")); + assert!(name.ends_with(".json")); + } + + #[test] + fn snapshot_filename_no_commit() { + let snap = DebtSnapshot::from_review(&[], None, None, None, 0, None, None); + let name = snapshot_filename(&snap); + assert!(name.contains("unknown")); + } + + // ─── trend_from_change ─── + + #[test] + fn trend_improving() { + assert_eq!(trend_from_change(-5), "improving"); + assert_eq!(trend_from_change(-2), "improving"); + } + + #[test] + fn trend_stable() { + assert_eq!(trend_from_change(0), "stable"); + assert_eq!(trend_from_change(1), "stable"); + assert_eq!(trend_from_change(-1), "stable"); + } + + #[test] + fn trend_worsening() { + assert_eq!(trend_from_change(2), "worsening"); + assert_eq!(trend_from_change(10), "worsening"); + } + + // ─── aggregate ─── + + #[test] + fn aggregate_empty() { + let report = aggregate(&[]); + assert_eq!(report.reviews_analyzed, 0); + assert_eq!(report.total_findings, 0); + assert_eq!(report.trend, "stable"); + } + + #[test] + fn aggregate_single_snapshot() { + let issues = vec![ + make_issue(Severity::Critical, "security"), + make_issue(Severity::Major, "bug"), + ]; + let snap = DebtSnapshot::from_review(&issues, None, None, None, 1, None, None); + let report = aggregate(&[snap]); + assert_eq!(report.reviews_analyzed, 1); + assert_eq!(report.total_findings, 2); + } + + #[test] + fn aggregate_multiple_snapshots_with_trend() { + // Create 4 snapshots with improving trend + let mut snapshots = Vec::new(); + + // Earlier: 3 critical issues + let mut snap1 = DebtSnapshot::from_review( + &[ + make_issue(Severity::Critical, "security"), + make_issue(Severity::Critical, "security"), + make_issue(Severity::Critical, "security"), + ], + None, + None, + None, + 1, + None, + None, + ); + snap1.timestamp = Utc::now() - chrono::Duration::days(4); + + // Later: 1 minor issue + let mut snap2 = DebtSnapshot::from_review( + &[make_issue(Severity::Minor, "style")], + None, + None, + None, + 1, + None, + None, + ); + snap2.timestamp = Utc::now() - chrono::Duration::days(3); + + let mut snap3 = DebtSnapshot::from_review( + &[make_issue(Severity::Minor, "style")], + None, + None, + None, + 1, + None, + None, + ); + snap3.timestamp = Utc::now() - chrono::Duration::days(2); + + let mut snap4 = DebtSnapshot::from_review( + &[make_issue(Severity::Info, "suggestion")], + None, + None, + None, + 1, + None, + None, + ); + snap4.timestamp = Utc::now() - chrono::Duration::days(1); + + snapshots.push(snap1); + snapshots.push(snap2); + snapshots.push(snap3); + snapshots.push(snap4); + + let report = aggregate(&snapshots); + assert_eq!(report.reviews_analyzed, 4); + assert_eq!(report.total_findings, 6); // 3 + 1 + 1 + 1 + } + + // ─── DebtConfig defaults ─── + + #[test] + fn debt_config_default() { + let config = DebtConfig::default(); + assert!(config.enabled); + assert!(config.history_dir.is_none()); + assert_eq!(config.retention_days, 90); + } + + #[test] + fn debt_config_history_dir_default() { + let config = DebtConfig::default(); + assert_eq!(config.history_dir(), ".cora/history"); + } + + #[test] + fn debt_config_custom_history_dir() { + let config = DebtConfig { + history_dir: Some("custom/dir".to_string()), + ..Default::default() + }; + assert_eq!(config.history_dir(), "custom/dir"); + } + + // ─── save/load roundtrip ─── + + #[test] + fn save_and_load_snapshots() { + let tmp_dir = tempfile::tempdir().unwrap(); + let dir_str = tmp_dir.path().to_string_lossy().to_string(); + + let snap1 = DebtSnapshot::from_review( + &[make_issue(Severity::Critical, "security")], + None, + Some("aaa1111".to_string()), + Some("main".to_string()), + 2, + Some(100), + Some(500), + ); + + let snap2 = DebtSnapshot::from_review( + &[make_issue(Severity::Minor, "style")], + None, + Some("bbb2222".to_string()), + Some("develop".to_string()), + 1, + None, + Some(300), + ); + + save_snapshot(&snap1, Some(&dir_str)); + save_snapshot(&snap2, Some(&dir_str)); + + let loaded = load_snapshots(Some(&dir_str)); + assert_eq!(loaded.len(), 2); + // Should be sorted by timestamp + assert!(loaded[0].timestamp <= loaded[1].timestamp); + } + + // ─── cleanup ─── + + #[test] + fn cleanup_removes_old_snapshots() { + let tmp_dir = tempfile::tempdir().unwrap(); + let dir_str = tmp_dir.path().to_string_lossy().to_string(); + + // Create an old snapshot + let mut old_snap = DebtSnapshot::from_review( + &[make_issue(Severity::Critical, "security")], + None, + Some("old1111".to_string()), + None, + 1, + None, + None, + ); + old_snap.timestamp = Utc::now() - chrono::Duration::days(100); + + // Create a recent snapshot + let recent_snap = DebtSnapshot::from_review( + &[make_issue(Severity::Minor, "style")], + None, + Some("new2222".to_string()), + None, + 1, + None, + None, + ); + + save_snapshot(&old_snap, Some(&dir_str)); + save_snapshot(&recent_snap, Some(&dir_str)); + + // Cleanup with 90-day retention + let removed = cleanup_old_snapshots(Some(&dir_str), 90); + assert_eq!(removed, 1); + + // Only the recent one should remain + let remaining = load_snapshots(Some(&dir_str)); + assert_eq!(remaining.len(), 1); + assert_eq!(remaining[0].commit.as_deref(), Some("new2222")); + } +} diff --git a/src/engine/mod.rs b/src/engine/mod.rs index a57bca6b..5dd236b0 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -2,6 +2,7 @@ pub mod bundling; pub mod cache; pub mod chunker; pub mod context; +pub mod debt_tracker; pub mod diff_parser; pub mod language_analyzer; pub mod llm; From 53f32aaff3064770bf553c74949da0ca44b93993 Mon Sep 17 00:00:00 2001 From: Aji Anaz Date: Thu, 11 Jun 2026 00:00:49 +0700 Subject: [PATCH 2/3] fix: address cora review findings on debt tracker - Snapshot filename: add nanosecond suffix to prevent same-second collision - Git context: cache with LazyLock to avoid spawning git per review - Config merge: field-by-field debt merge instead of wholesale replace - All 485 tests pass, clippy clean --- src/commands/review.rs | 37 ++++++++++++++++++++++--------------- src/config/schema.rs | 10 +++++++++- src/engine/debt_tracker.rs | 13 +++++++++++-- 3 files changed, 42 insertions(+), 18 deletions(-) diff --git a/src/commands/review.rs b/src/commands/review.rs index 6265b114..a8eaadc7 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -276,24 +276,31 @@ pub async fn execute_review( /// Helper to get git commit and branch context for debt tracking. /// Returns (Some(commit), Some(branch)) if in a git repo, (None, None) otherwise. +/// Results are cached per process to avoid spawning git on every review. fn get_git_context() -> (Option, Option) { - let commit = std::process::Command::new("git") - .args(["rev-parse", "--short", "HEAD"]) - .output() - .ok() - .and_then(|o| { - if o.status.success() { - String::from_utf8(o.stdout) - .ok() - .map(|s| s.trim().to_string()) - } else { - None - } - }); + use std::sync::LazyLock; + + static CONTEXT: LazyLock<(Option, Option)> = LazyLock::new(|| { + let commit = std::process::Command::new("git") + .args(["rev-parse", "--short", "HEAD"]) + .output() + .ok() + .and_then(|o| { + if o.status.success() { + String::from_utf8(o.stdout) + .ok() + .map(|s| s.trim().to_string()) + } else { + None + } + }); + + let branch = crate::git::get_current_branch().ok(); - let branch = crate::git::get_current_branch().ok(); + (commit, branch) + }); - (commit, branch) + (CONTEXT.0.clone(), CONTEXT.1.clone()) } /// Get the diff based on the provided options. diff --git a/src/config/schema.rs b/src/config/schema.rs index 51e79e01..1398991f 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -473,7 +473,15 @@ impl CoraFile { } // Merge debt tracking config if let Some(debt) = &self.debt { - config.debt = debt.clone(); + if !debt.enabled { + config.debt.enabled = false; + } + if let Some(ref dir) = debt.history_dir { + config.debt.history_dir = Some(dir.clone()); + } + if debt.retention_days != 90 { + config.debt.retention_days = debt.retention_days; + } } // Resolve profile — load built-in or custom, merge into config if let Some(profile_ref) = &self.profile { diff --git a/src/engine/debt_tracker.rs b/src/engine/debt_tracker.rs index bf887f28..f718a417 100644 --- a/src/engine/debt_tracker.rs +++ b/src/engine/debt_tracker.rs @@ -206,7 +206,9 @@ fn snapshot_filename(snapshot: &DebtSnapshot) -> String { let hash = snapshot.commit.as_deref().unwrap_or("unknown"); // Take first 7 chars of commit hash let short_hash = if hash.len() > 7 { &hash[..7] } else { hash }; - format!("{date}_{short_hash}.json") + // Add sub-second nanos to prevent same-second collision + let nanos = snapshot.timestamp.timestamp_subsec_nanos(); + format!("{date}_{short_hash}_{:09}.json", nanos) } /// Resolve the history directory path. @@ -749,10 +751,17 @@ mod tests { None, ); let name = snapshot_filename(&snap); - // Should be like: 2026-06-10_123456_abcdef1.json + // Should be like: 2026-06-10_123456_abcdef1_000000000.json assert!(name.starts_with("2026")); assert!(name.contains("_abcdef1")); assert!(name.ends_with(".json")); + // Should have 3 underscore-separated segments before .json + let stem = name.trim_end_matches(".json"); + let parts: Vec<&str> = stem.split('_').collect(); + assert!( + parts.len() >= 3, + "expected at least 3 parts, got: {parts:?}" + ); } #[test] From 1fc076fa4e2d44557a88bbf0b4584030d696d488 Mon Sep 17 00:00:00 2001 From: Aji Anaz Date: Thu, 11 Jun 2026 00:02:09 +0700 Subject: [PATCH 3/3] fix: content-hash snapshot filenames and explicit debt config merge - Use content-based hash (findings + categories + timestamp) for unique filenames - Config merge: always apply all debt fields from .cora.yaml (allows explicit reset) --- src/config/schema.rs | 12 ++++-------- src/engine/debt_tracker.rs | 22 +++++++++++++++++----- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/config/schema.rs b/src/config/schema.rs index 1398991f..705a0f22 100644 --- a/src/config/schema.rs +++ b/src/config/schema.rs @@ -473,15 +473,11 @@ impl CoraFile { } // Merge debt tracking config if let Some(debt) = &self.debt { - if !debt.enabled { - config.debt.enabled = false; - } - if let Some(ref dir) = debt.history_dir { - config.debt.history_dir = Some(dir.clone()); - } - if debt.retention_days != 90 { - config.debt.retention_days = debt.retention_days; + config.debt.enabled = debt.enabled; + if debt.history_dir.is_some() { + config.debt.history_dir.clone_from(&debt.history_dir); } + config.debt.retention_days = debt.retention_days; } // Resolve profile — load built-in or custom, merge into config if let Some(profile_ref) = &self.profile { diff --git a/src/engine/debt_tracker.rs b/src/engine/debt_tracker.rs index f718a417..aeebcb59 100644 --- a/src/engine/debt_tracker.rs +++ b/src/engine/debt_tracker.rs @@ -200,15 +200,27 @@ fn trend_from_change(change: i64) -> &'static str { // ─── File I/O ─── -/// Generate a filename for a snapshot: `YYYY-MM-DD_HHMMSS_{short_hash}.json` +/// Generate a filename for a snapshot using a content-based hash for uniqueness. +/// Format: `YYYY-MM-DD_{short_hash}_{content_hash}.json` fn snapshot_filename(snapshot: &DebtSnapshot) -> String { - let date = snapshot.timestamp.format("%Y-%m-%d_%H%M%S"); + let date = snapshot.timestamp.format("%Y-%m-%d"); let hash = snapshot.commit.as_deref().unwrap_or("unknown"); // Take first 7 chars of commit hash let short_hash = if hash.len() > 7 { &hash[..7] } else { hash }; - // Add sub-second nanos to prevent same-second collision - let nanos = snapshot.timestamp.timestamp_subsec_nanos(); - format!("{date}_{short_hash}_{:09}.json", nanos) + // Content-based uniqueness: hash the findings + categories + timestamp + use std::hash::{Hash, Hasher}; + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + snapshot.timestamp.hash(&mut hasher); + for (k, v) in &snapshot.findings { + k.hash(&mut hasher); + v.hash(&mut hasher); + } + for (k, v) in &snapshot.categories { + k.hash(&mut hasher); + v.hash(&mut hasher); + } + let content_hash = format!("{:08x}", hasher.finish()); + format!("{date}_{short_hash}_{content_hash}.json") } /// Resolve the history directory path.