From eba884f35c25a7655890ea5f4a9a862786497479 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 8 Oct 2026 10:03:18 +0700 Subject: [PATCH] refactor(store): single review_store module owns review-history SQL Move all SQL for reviews/findings/finding_events into engine::review_store (replaces engine::db_writer). cora findings, debt tracker and review/scan persistence call it and hold no SQL. Store takes a &Connection so it is testable with in-memory SQLite; best-effort persistence policy is explicit in persist_review_best_effort. Co-Authored-By: Claude Sonnet 5.5 Signed-off-by: ajianaz --- src/commands/findings.rs | 230 +++------ src/commands/review.rs | 38 +- src/commands/scan.rs | 20 +- src/engine/db_writer.rs | 304 ------------ src/engine/debt_tracker.rs | 217 ++++----- src/engine/mod.rs | 2 +- src/engine/review_store.rs | 926 +++++++++++++++++++++++++++++++++++++ 7 files changed, 1080 insertions(+), 657 deletions(-) delete mode 100644 src/engine/db_writer.rs create mode 100644 src/engine/review_store.rs diff --git a/src/commands/findings.rs b/src/commands/findings.rs index 359644f..29bc786 100644 --- a/src/commands/findings.rs +++ b/src/commands/findings.rs @@ -3,6 +3,8 @@ use anyhow::Result; use colored::Colorize; +use crate::engine::review_store::{self, FindingFilter, FindingStats, ReviewStore, Transition}; + /// Exit codes. const EXIT_OK: i32 = 0; const EXIT_NOT_FOUND: i32 = 1; @@ -63,13 +65,11 @@ pub fn execute_findings(action: &FindingsAction) -> Result { // Write actions (dismiss, reopen) use a read-write connection. match action { FindingsAction::List { .. } | FindingsAction::Stats { .. } => { - let conn = match crate::engine::db_writer::open_db_for_read() { - Some(c) => c, - None => { - eprintln!("{}", "Error: could not open cora.db".red()); - return Ok(EXIT_NOT_FOUND); - } + let Ok(conn) = review_store::open_read() else { + eprintln!("{}", "Error: could not open cora.db".red()); + return Ok(EXIT_NOT_FOUND); }; + let store = ReviewStore::new(&conn); match action { FindingsAction::List { all, @@ -77,90 +77,38 @@ pub fn execute_findings(action: &FindingsAction) -> Result { file, json, limit, - } => list_findings(&conn, *all, severity, file, *json, *limit), - FindingsAction::Stats { json } => stats(&conn, *json), + } => { + let filter = FindingFilter { + all: *all, + severity: severity.clone(), + file: file.clone(), + limit: *limit, + }; + list_findings(&store, &filter, *json) + } + FindingsAction::Stats { json } => stats(&store, *json), _ => unreachable!(), } } FindingsAction::Dismiss { id, reason } => { - let conn = match crate::engine::db_writer::open_db_for_write() { - Some(c) => c, - None => { - eprintln!("{}", "Error: could not open cora.db for writing".red()); - return Ok(EXIT_NOT_FOUND); - } + let Ok(conn) = review_store::open_write() else { + eprintln!("{}", "Error: could not open cora.db for writing".red()); + return Ok(EXIT_NOT_FOUND); }; - dismiss(&conn, *id, reason) + dismiss(&ReviewStore::new(&conn), *id, reason.as_deref()) } FindingsAction::Reopen { id } => { - let conn = match crate::engine::db_writer::open_db_for_write() { - Some(c) => c, - None => { - eprintln!("{}", "Error: could not open cora.db for writing".red()); - return Ok(EXIT_NOT_FOUND); - } + let Ok(conn) = review_store::open_write() else { + eprintln!("{}", "Error: could not open cora.db for writing".red()); + return Ok(EXIT_NOT_FOUND); }; - reopen(&conn, *id) + reopen(&ReviewStore::new(&conn), *id) } } } -fn list_findings( - conn: &rusqlite::Connection, - all: bool, - severity: &Option, - file: &Option, - json: bool, - limit: usize, -) -> Result { - let mut sql = String::from( - "SELECT f.id, f.severity, f.file_path, f.line_number, f.title, f.status, - f.fingerprint, r.created_at - FROM findings f - JOIN reviews r ON f.review_id = r.id", - ); - - // Build WHERE clause with parameterized placeholders to prevent SQL injection. - let mut wheres: Vec<&str> = Vec::new(); - let mut params: Vec> = Vec::new(); - - if !all { - wheres.push("f.status = 'open'"); - } - if let Some(s) = severity { - wheres.push("f.severity = ?"); - params.push(Box::new(s.to_uppercase())); - } - if let Some(f) = file { - wheres.push("f.file_path LIKE ?"); - params.push(Box::new(format!("%{f}%"))); - } - - if !wheres.is_empty() { - sql.push_str(" WHERE "); - sql.push_str(&wheres.join(" AND ")); - } - sql.push_str(" ORDER BY f.id DESC LIMIT ?"); - params.push(Box::new(limit as i64)); - - let param_refs: Vec<&dyn rusqlite::ToSql> = params.iter().map(|p| p.as_ref()).collect(); - let mut stmt = conn.prepare(&sql)?; - let rows: Vec = stmt - .query(param_refs.as_slice())? - .mapped(|r| { - Ok(ListRow { - id: r.get(0)?, - severity: r.get(1)?, - file_path: r.get(2)?, - line_number: r.get(3)?, - title: r.get(4)?, - status: r.get(5)?, - fingerprint: r.get(6)?, - created_at: r.get(7)?, - }) - }) - .filter_map(|r| r.ok()) - .collect(); +fn list_findings(store: &ReviewStore<'_>, filter: &FindingFilter, json: bool) -> Result { + let rows = store.list_findings(filter)?; if json { println!("{}", serde_json::to_string_pretty(&rows)?); @@ -208,38 +156,14 @@ fn list_findings( Ok(EXIT_OK) } -fn stats(conn: &rusqlite::Connection, json: bool) -> Result { - let total: i64 = conn - .query_row("SELECT count(*) FROM findings", [], |r| r.get(0)) - .unwrap_or(0); - - let open: i64 = conn - .query_row( - "SELECT count(*) FROM findings WHERE status = 'open'", - [], - |r| r.get(0), - ) - .unwrap_or(0); - - let resolved: i64 = conn - .query_row( - "SELECT count(*) FROM findings WHERE status = 'resolved'", - [], - |r| r.get(0), - ) - .unwrap_or(0); - - let dismissed: i64 = conn - .query_row( - "SELECT count(*) FROM findings WHERE status = 'dismissed'", - [], - |r| r.get(0), - ) - .unwrap_or(0); - - let reviews: i64 = conn - .query_row("SELECT count(*) FROM reviews", [], |r| r.get(0)) - .unwrap_or(0); +fn stats(store: &ReviewStore<'_>, json: bool) -> Result { + let FindingStats { + total, + open, + resolved, + dismissed, + reviews, + } = store.stats()?; if json { let stats = serde_json::json!({ @@ -269,78 +193,32 @@ fn stats(conn: &rusqlite::Connection, json: bool) -> Result { Ok(EXIT_OK) } -fn dismiss(conn: &rusqlite::Connection, id: i64, reason: &Option) -> Result { - let exists: bool = conn - .query_row( - "SELECT status FROM findings WHERE id = ?1", - rusqlite::params![id], - |r| r.get::<_, String>(0), - ) - .is_ok(); - - if !exists { - eprintln!("{}", format!("Finding #{} not found.", id).red()); - return Ok(EXIT_NOT_FOUND); +fn dismiss(store: &ReviewStore<'_>, id: i64, reason: Option<&str>) -> Result { + match store.dismiss(id, reason)? { + Transition::NotFound => { + eprintln!("{}", format!("Finding #{} not found.", id).red()); + Ok(EXIT_NOT_FOUND) + } + _ => { + println!("{} Finding #{} dismissed.", "✓".green(), id); + Ok(EXIT_OK) + } } - - conn.execute( - "UPDATE findings SET status = 'dismissed' WHERE id = ?1", - rusqlite::params![id], - )?; - - let note = reason.as_deref().unwrap_or("Manually dismissed via CLI"); - conn.execute( - "INSERT INTO finding_events (finding_id, event_type, note) VALUES (?1, 'dismissed', ?2)", - rusqlite::params![id, note], - )?; - - println!("{} Finding #{} dismissed.", "✓".green(), id); - Ok(EXIT_OK) } -fn reopen(conn: &rusqlite::Connection, id: i64) -> Result { - let status: Option = conn - .query_row( - "SELECT status FROM findings WHERE id = ?1", - rusqlite::params![id], - |r| r.get(0), - ) - .ok(); - - match status.as_deref() { - Some("open") => { +fn reopen(store: &ReviewStore<'_>, id: i64) -> Result { + match store.reopen(id)? { + Transition::Unchanged => { println!("{}", format!("Finding #{} is already open.", id).yellow()); - return Ok(EXIT_OK); + Ok(EXIT_OK) } - None => { + Transition::NotFound => { eprintln!("{}", format!("Finding #{} not found.", id).red()); - return Ok(EXIT_NOT_FOUND); + Ok(EXIT_NOT_FOUND) + } + Transition::Applied => { + println!("{} Finding #{} reopened.", "✓".green(), id); + Ok(EXIT_OK) } - _ => {} } - - conn.execute( - "UPDATE findings SET status = 'open' WHERE id = ?1", - rusqlite::params![id], - )?; - - conn.execute( - "INSERT INTO finding_events (finding_id, event_type, note) VALUES (?1, 'reopened', 'Manually reopened via CLI')", - rusqlite::params![id], - )?; - - println!("{} Finding #{} reopened.", "✓".green(), id); - Ok(EXIT_OK) -} - -#[derive(serde::Serialize)] -struct ListRow { - id: i64, - severity: String, - file_path: String, - line_number: Option, - title: String, - status: String, - fingerprint: Option, - created_at: String, } diff --git a/src/commands/review.rs b/src/commands/review.rs index 9df526c..bff4001 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -5,8 +5,8 @@ use tracing::debug; use crate::config::schema::Config; use crate::engine::Severity; use crate::engine::chunker; -use crate::engine::db_writer; use crate::engine::quality_gate; +use crate::engine::review_store; use crate::engine::types::ReviewResponse; use crate::formatters::{OutputFormat, formatter_for}; use crate::git; @@ -317,7 +317,7 @@ pub async fn execute_review( let cwd = std::env::current_dir() .map(|p| p.to_string_lossy().to_string()) .unwrap_or_default(); - let record = db_writer::ReviewRecord { + let record = review_store::ReviewRecord { command: cmd, project_root: &cwd, commit_hash: commit.as_deref(), @@ -330,20 +330,8 @@ pub async fn execute_review( tokens, issues: &filtered_response.issues, }; - if db_writer::save_review_to_db(&record).is_none() { - debug!("Failed to save review to cora.db"); - } - - // Auto-resolve findings that no longer appear in this review. - let fps: Vec = filtered_response - .issues - .iter() - .map(db_writer::compute_fingerprint_pub) - .collect(); - let resolved = db_writer::resolve_stale_findings(&cwd, &fps); - if resolved > 0 { - debug!(resolved, "auto-resolved stale findings"); - } + // Best-effort: a history-write failure never fails the run. + review_store::persist_review_best_effort(&record); } let exit_code = if gate_result .as_ref() @@ -717,7 +705,7 @@ async fn execute_chunked_review( let cwd = std::env::current_dir() .map(|p| p.to_string_lossy().to_string()) .unwrap_or_default(); - let record = db_writer::ReviewRecord { + let record = review_store::ReviewRecord { command: cmd, project_root: &cwd, commit_hash: commit.as_deref(), @@ -730,20 +718,8 @@ async fn execute_chunked_review( tokens, issues: &filtered_response.issues, }; - if db_writer::save_review_to_db(&record).is_none() { - debug!("Failed to save review to cora.db"); - } - - // Auto-resolve findings that no longer appear in this review. - let fps: Vec = filtered_response - .issues - .iter() - .map(db_writer::compute_fingerprint_pub) - .collect(); - let resolved = db_writer::resolve_stale_findings(&cwd, &fps); - if resolved > 0 { - debug!(resolved, "auto-resolved stale findings"); - } + // Best-effort: a history-write failure never fails the run. + review_store::persist_review_best_effort(&record); } let exit_code = compute_exit_code( gate_result.as_ref().map(|g| g.status), diff --git a/src/commands/scan.rs b/src/commands/scan.rs index 1dcc73c..8913d64 100644 --- a/src/commands/scan.rs +++ b/src/commands/scan.rs @@ -5,7 +5,7 @@ use colored::Colorize; use tracing::debug; use crate::config::schema::Config; -use crate::engine::db_writer; +use crate::engine::review_store; use crate::engine::scanner::{batch_files, format_batch_for_prompt, walk_project}; use crate::engine::types::TokenUsage; use crate::formatters::{OutputFormat, formatter_for}; @@ -305,7 +305,7 @@ pub async fn execute_scan( let cwd = std::env::current_dir() .map(|p| p.to_string_lossy().to_string()) .unwrap_or_default(); - let record = db_writer::ReviewRecord { + let record = review_store::ReviewRecord { command: "scan", project_root: &cwd, commit_hash: commit.as_deref(), @@ -318,20 +318,8 @@ pub async fn execute_scan( tokens: response.tokens_used.as_ref(), issues: &response.issues, }; - if db_writer::save_review_to_db(&record).is_none() { - debug!("Failed to save scan to cora.db"); - } - - // Auto-resolve findings that no longer appear in this scan. - let fps: Vec = response - .issues - .iter() - .map(db_writer::compute_fingerprint_pub) - .collect(); - let resolved = db_writer::resolve_stale_findings(&cwd, &fps); - if resolved > 0 { - debug!(resolved, "auto-resolved stale findings"); - } + // Best-effort: a history-write failure never fails the run. + review_store::persist_review_best_effort(&record); } if response.should_block && config.hook.mode == "block" { diff --git a/src/engine/db_writer.rs b/src/engine/db_writer.rs deleted file mode 100644 index 12e4888..0000000 --- a/src/engine/db_writer.rs +++ /dev/null @@ -1,304 +0,0 @@ -//! Database writer - persists review/scan results to `cora.db`. -//! -//! Uses the v5 schema tables: `reviews`, `findings`, `finding_events`. -//! All operations are best-effort (non-fatal on error) to never block a review. - -use rusqlite::Connection; - -use crate::engine::Severity; -use crate::engine::types::{ReviewIssue, TokenUsage}; -use crate::index::schema; - -/// Input data for saving a review/scan run to the database. -pub struct ReviewRecord<'a> { - /// "review" or "scan". - pub command: &'a str, - /// Absolute path of the project root (used for project lookup/creation). - pub project_root: &'a str, - /// Git commit hash (short) if available. - pub commit_hash: Option<&'a str>, - /// Git branch name if available. - pub branch: Option<&'a str>, - /// LLM-generated summary text. - pub summary: &'a str, - /// Quality gate status: "passed", "failed", or "disabled". - pub gate_status: &'a str, - /// Number of files scanned/reviewed. - pub files_scanned: usize, - /// Number of lines scanned/reviewed. - pub lines_scanned: usize, - /// Whether the quality gate should block. - pub should_block: bool, - /// Token usage from the LLM call (if any). - pub tokens: Option<&'a TokenUsage>, - /// The issues/findings to persist. - pub issues: &'a [ReviewIssue], -} - -/// Save a review/scan run to `cora.db`. -/// -/// Opens its own connection (the global DB is single-writer safe for our -/// low-frequency writes). Returns the `review_id` on success, or `None` on -/// error (best-effort: caller continues regardless). -pub fn save_review_to_db(record: &ReviewRecord<'_>) -> Option { - let conn = open_db().ok()?; - let project_id = schema::get_or_create_project(&conn, record.project_root).ok()?; - - // Insert the review row. - let (input_tokens, output_tokens, cost_usd) = record - .tokens - .map(|t| { - ( - t.input_tokens as i64, - t.output_tokens as i64, - t.estimated_cost_usd, - ) - }) - .unwrap_or((0, 0, 0.0)); - - conn.execute( - "INSERT INTO reviews - (project_id, command, commit_hash, branch, summary, score, gate_status, - files_scanned, lines_scanned, should_block, input_tokens, output_tokens, cost_usd) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13)", - rusqlite::params![ - project_id, - record.command, - record.commit_hash, - record.branch, - record.summary, - calculate_score(record.issues) as i64, - record.gate_status, - record.files_scanned as i64, - record.lines_scanned as i64, - record.should_block as i64, - input_tokens, - output_tokens, - cost_usd, - ], - ) - .ok()?; - - let review_id = conn.last_insert_rowid(); - - // Insert each finding + an "opened" event. - let mut stmt_findings = conn - .prepare( - "INSERT INTO findings - (review_id, file_path, line_number, severity, issue_type, title, body, - suggested_fix, status, fingerprint) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, 'open', ?9)", - ) - .ok()?; - - let mut stmt_events = conn - .prepare( - "INSERT INTO finding_events (finding_id, event_type, note) - VALUES (?1, 'opened', NULL)", - ) - .ok()?; - - for issue in record.issues { - let fingerprint = compute_fingerprint(issue); - stmt_findings - .execute(rusqlite::params![ - review_id, - issue.file, - issue.line.map(|l| l as i64), - issue.severity.to_string(), - issue.issue_type.as_deref(), - issue.title, - issue.body, - issue.suggested_fix.as_deref(), - fingerprint, - ]) - .ok()?; - - let finding_id = conn.last_insert_rowid(); - stmt_events.execute(rusqlite::params![finding_id]).ok()?; - } - - Some(review_id) -} - -/// Auto-resolve findings from prior reviews that no longer appear. -/// -/// After saving a new review, any `open` findings in the same project whose -/// fingerprint is *not* in the current review's findings are marked `resolved` -/// with an `auto_resolved` event. Findings that *do* reappear are left `open`. -pub fn resolve_stale_findings(project_root: &str, current_fingerprints: &[String]) -> usize { - let Ok(conn) = open_db() else { return 0 }; - let Ok(project_id) = schema::get_or_create_project(&conn, project_root) else { - return 0; - }; - - // Fetch (id, fingerprint) for all open findings in this project. - let mut stmt = match conn.prepare( - "SELECT f.id, f.fingerprint FROM findings f - JOIN reviews r ON f.review_id = r.id - WHERE r.project_id = ?1 - AND f.status = 'open' - AND f.fingerprint IS NOT NULL", - ) { - Ok(s) => s, - Err(_) => return 0, - }; - let mut rows = match stmt.query(rusqlite::params![project_id]) { - Ok(r) => r, - Err(_) => return 0, - }; - - let mut stale_ids: Vec = Vec::new(); - while let Some(row) = rows.next().unwrap_or(None) { - let id: i64 = row.get(0).unwrap_or(0); - let fp: String = row.get(1).unwrap_or_default(); - if !current_fingerprints.contains(&fp) { - stale_ids.push(id); - } - } - - let mut resolved = 0; - let mut stmt_update = conn - .prepare("UPDATE findings SET status = 'resolved' WHERE id = ?1") - .ok(); - let mut stmt_event = conn - .prepare( - "INSERT INTO finding_events (finding_id, event_type, note) - VALUES (?1, 'auto_resolved', 'No longer found in latest review')", - ) - .ok(); - - for id in &stale_ids { - if let (Some(u), Some(e)) = (stmt_update.as_mut(), stmt_event.as_mut()) { - if u.execute(rusqlite::params![id]).is_ok() && e.execute(rusqlite::params![id]).is_ok() - { - resolved += 1; - } - } - } - - resolved -} - -/// Open the global `cora.db` and ensure migrations are up to date. -/// -/// Delegates to the shared index opener so PRAGMAs live in one place. -fn open_db() -> anyhow::Result { - crate::index::open_global_index() -} - -/// Open cora.db in read-only mode (no migrations, no WAL). -/// Returns `None` if the DB doesn't exist or can't be opened. -pub fn open_db_for_read() -> Option { - let db_path = crate::data_dir::graph_db_path(); - if !std::path::Path::new(&db_path).exists() { - return None; - } - Connection::open_with_flags(&db_path, rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY).ok() -} - -/// Open cora.db for read-write, running migrations if needed. -/// Returns `None` on failure (best-effort, never panics). -pub fn open_db_for_write() -> Option { - open_db().ok() -} - -/// Compute a fingerprint for dedup/auto-resolve: `file:line:title_slug`. -/// Public wrapper so callers (review.rs, scan.rs) can compute fingerprints -/// for the current review before calling `resolve_stale_findings`. -pub fn compute_fingerprint_pub(issue: &ReviewIssue) -> String { - compute_fingerprint(issue) -} - -fn compute_fingerprint(issue: &ReviewIssue) -> String { - let line = issue.line.unwrap_or(0); - let title_slug = issue.title.to_lowercase().replace(' ', "_"); - format!("{}:{}:{}", issue.file, line, title_slug) -} - -/// Calculate a quality score 0-100 from issue severities. -/// -/// 100 = no issues. Each finding reduces the score: -/// - critical: -20, major: -10, minor: -3, info: -1 -fn calculate_score(issues: &[ReviewIssue]) -> f64 { - let mut score: f64 = 100.0; - for issue in issues { - let penalty: f64 = match issue.severity { - Severity::Critical => 20.0, - Severity::Major => 10.0, - Severity::Minor => 3.0, - Severity::Info => 1.0, - }; - score -= penalty; - } - score.max(0.0) -} - -#[cfg(test)] -mod tests { - use super::*; - - fn make_issue(file: &str, line: u32, severity: Severity, title: &str) -> ReviewIssue { - ReviewIssue { - file: file.to_string(), - line: Some(line), - severity, - issue_type: Some("security".to_string()), - title: title.to_string(), - body: "test body".to_string(), - suggested_fix: Some("fix it".to_string()), - } - } - - #[test] - fn test_fingerprint_format() { - let issue = make_issue("src/main.rs", 42, Severity::Critical, "SQL Injection"); - let fp = compute_fingerprint(&issue); - assert_eq!(fp, "src/main.rs:42:sql_injection"); - } - - #[test] - fn test_fingerprint_no_line() { - let mut issue = make_issue("src/lib.rs", 0, Severity::Minor, "Unused Import"); - issue.line = None; - let fp = compute_fingerprint(&issue); - assert_eq!(fp, "src/lib.rs:0:unused_import"); - } - - #[test] - fn test_score_no_issues() { - let issues: Vec = vec![]; - assert_eq!(calculate_score(&issues), 100.0); - } - - #[test] - fn test_score_with_critical() { - let issues = vec![make_issue("a.rs", 1, Severity::Critical, "x")]; - assert_eq!(calculate_score(&issues), 80.0); - } - - #[test] - fn test_score_floor_zero() { - let issues = vec![ - make_issue("a.rs", 1, Severity::Critical, "x"), - make_issue("a.rs", 2, Severity::Critical, "y"), - make_issue("a.rs", 3, Severity::Critical, "z"), - make_issue("a.rs", 4, Severity::Critical, "w"), - make_issue("a.rs", 5, Severity::Critical, "v"), - make_issue("a.rs", 6, Severity::Critical, "u"), - ]; - assert_eq!(calculate_score(&issues), 0.0); - } - - #[test] - fn test_score_mixed() { - let issues = vec![ - make_issue("a.rs", 1, Severity::Critical, "c"), - make_issue("b.rs", 2, Severity::Major, "m"), - make_issue("c.rs", 3, Severity::Minor, "n"), - make_issue("d.rs", 4, Severity::Info, "i"), - ]; - // 100 - 20 - 10 - 3 - 1 = 66 - assert_eq!(calculate_score(&issues), 66.0); - } -} diff --git a/src/engine/debt_tracker.rs b/src/engine/debt_tracker.rs index be11e92..6631cad 100644 --- a/src/engine/debt_tracker.rs +++ b/src/engine/debt_tracker.rs @@ -4,6 +4,7 @@ //! Provides aggregation and trend analysis across multiple reviews. use crate::engine::quality_gate::GateResult; +use crate::engine::review_store::ReviewRow; use crate::engine::types::{ReviewIssue, Severity}; use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; @@ -488,141 +489,57 @@ pub fn aggregate(snapshots: &[DebtSnapshot]) -> DebtReport { /// Load review snapshots from `cora.db` for the given project root. /// -/// This is the preferred data source (SoT). Falls back gracefully if the DB -/// doesn't exist or has no reviews. +/// This is the preferred data source (SoT). Falls back gracefully (empty) if +/// the DB doesn't exist or has no reviews; the SQL lives in +/// [`crate::engine::review_store`]. /// /// Converts the DB's 0-100 score to the 0-10 scale used by `DebtSnapshot`. pub fn load_snapshots_from_db(project_root: &str) -> Vec { - let Some(conn) = crate::engine::db_writer::open_db_for_read() else { - return Vec::new(); - }; - - let canonical = match std::path::Path::new(project_root).canonicalize() { - Ok(p) => p.to_string_lossy().to_string(), - Err(_) => project_root.to_string(), - }; - - // Get project ID - let project_id: i64 = match conn.query_row( - "SELECT id FROM projects WHERE root_path = ?1", - rusqlite::params![canonical], - |row| row.get(0), - ) { - Ok(id) => id, - Err(_) => return Vec::new(), - }; - - // Load all reviews for this project, ordered by created_at - let mut stmt = match conn.prepare( - "SELECT id, commit_hash, branch, files_scanned, lines_scanned, - score, gate_status, created_at - FROM reviews WHERE project_id = ?1 ORDER BY created_at ASC", - ) { - Ok(s) => s, - Err(_) => return Vec::new(), - }; - - let mut rows = match stmt.query(rusqlite::params![project_id]) { - Ok(r) => r, - Err(_) => return Vec::new(), - }; - - let mut snapshots = Vec::new(); - while let Some(row) = rows.next().unwrap_or(None) { - let review_id: i64 = row.get(0).unwrap_or(0); - let commit: Option = row.get(1).unwrap_or(None); - let branch: Option = row.get(2).unwrap_or(None); - let files_scanned: i64 = row.get(3).unwrap_or(0); - let lines_scanned: i64 = row.get(4).unwrap_or(0); - let db_score: f64 = row.get(5).unwrap_or(100.0); - let gate_status: String = row.get(6).unwrap_or_else(|_| "disabled".to_string()); - let created_at: String = row.get(7).unwrap_or_default(); - - // Parse timestamp - let timestamp = DateTime::parse_from_rfc3339(&created_at) - .map(|dt| dt.with_timezone(&Utc)) - .or_else(|_| { - chrono::NaiveDateTime::parse_from_str(&created_at, "%Y-%m-%d %H:%M:%S") - .map(|ndt| ndt.and_utc()) - }) - .unwrap_or_else(|_| Utc::now()); - - // Load findings for this review - let findings = load_findings_for_review(&conn, review_id); - let categories = load_categories_for_review(&conn, review_id); - - // Convert DB score (0-100) to DebtSnapshot scale (0-10) - let quality_score = db_score / 10.0; - - snapshots.push(DebtSnapshot { - timestamp, - commit, - branch, - files_reviewed: files_scanned as usize, - lines_reviewed: if lines_scanned > 0 { - Some(lines_scanned as usize) - } else { - None - }, - findings, - categories, - quality_score, - gate_status, - duration_ms: None, // not stored in DB - }); - } - - snapshots + snapshots_from_rows(crate::engine::review_store::load_debt_rows(project_root)) } -/// Load severity → count map for findings of a specific review. -fn load_findings_for_review(conn: &rusqlite::Connection, review_id: i64) -> HashMap { - let mut findings: HashMap = HashMap::new(); - let mut stmt = match conn.prepare( - "SELECT severity, COUNT(*) as cnt FROM findings - WHERE review_id = ?1 AND status = 'open' - GROUP BY severity", - ) { - Ok(s) => s, - Err(_) => return findings, - }; - let mut rows = match stmt.query(rusqlite::params![review_id]) { - Ok(r) => r, - Err(_) => return findings, - }; - while let Some(row) = rows.next().unwrap_or(None) { - let severity: String = row.get::<_, String>(0).unwrap_or_default().to_lowercase(); - let count: usize = row.get::<_, i64>(1).unwrap_or(0) as usize; - *findings.entry(severity).or_insert(0) += count; - } - findings -} +/// Convert stored review rows into debt snapshots (pure; no I/O). +pub fn snapshots_from_rows(rows: Vec) -> Vec { + rows.into_iter() + .map(|row| { + // Parse timestamp + let timestamp = DateTime::parse_from_rfc3339(&row.created_at) + .map(|dt| dt.with_timezone(&Utc)) + .or_else(|_| { + chrono::NaiveDateTime::parse_from_str(&row.created_at, "%Y-%m-%d %H:%M:%S") + .map(|ndt| ndt.and_utc()) + }) + .unwrap_or_else(|_| Utc::now()); + + let mut findings: HashMap = HashMap::new(); + for (severity, count) in row.open_by_severity { + *findings.entry(severity.to_lowercase()).or_insert(0) += count; + } + let mut categories: HashMap = HashMap::new(); + for (issue_type, count) in row.open_by_issue_type { + let cat = normalize_category(&issue_type); + *categories.entry(cat.to_string()).or_insert(0) += count; + } -/// Load category → count map for findings of a specific review. -fn load_categories_for_review( - conn: &rusqlite::Connection, - review_id: i64, -) -> HashMap { - let mut categories: HashMap = HashMap::new(); - let mut stmt = match conn.prepare( - "SELECT issue_type, COUNT(*) as cnt FROM findings - WHERE review_id = ?1 AND status = 'open' AND issue_type IS NOT NULL - GROUP BY issue_type", - ) { - Ok(s) => s, - Err(_) => return categories, - }; - let mut rows = match stmt.query(rusqlite::params![review_id]) { - Ok(r) => r, - Err(_) => return categories, - }; - while let Some(row) = rows.next().unwrap_or(None) { - let issue_type: String = row.get::<_, String>(0).unwrap_or_default(); - let count: usize = row.get::<_, i64>(1).unwrap_or(0) as usize; - let cat = normalize_category(&issue_type); - *categories.entry(cat.to_string()).or_insert(0) += count; - } - categories + DebtSnapshot { + timestamp, + commit: row.commit_hash, + branch: row.branch, + files_reviewed: row.files_scanned as usize, + lines_reviewed: if row.lines_scanned > 0 { + Some(row.lines_scanned as usize) + } else { + None + }, + findings, + categories, + // Convert DB score (0-100) to DebtSnapshot scale (0-10) + quality_score: row.score / 10.0, + gate_status: row.gate_status, + duration_ms: None, // not stored in DB + } + }) + .collect() } // ─── Debt config ─── @@ -867,6 +784,48 @@ mod tests { assert_eq!(counts.get("style"), Some(&1)); } + // ─── snapshots from the review store ─── + + #[test] + fn snapshots_from_store_rows() { + use crate::engine::review_store::{ReviewRecord, ReviewStore}; + let conn = rusqlite::Connection::open_in_memory().unwrap(); + crate::index::schema::run_migrations(&conn).unwrap(); + let store = ReviewStore::new(&conn); + let issues = vec![ + make_issue(Severity::Critical, "injection"), + make_issue(Severity::Minor, "style"), + ]; + store + .record_review(&ReviewRecord { + command: "review", + project_root: "/nonexistent/debt-proj", + commit_hash: Some("abc"), + branch: Some("main"), + summary: "", + gate_status: "failed", + files_scanned: 2, + lines_scanned: 0, + should_block: true, + tokens: None, + issues: &issues, + }) + .unwrap(); + + let snaps = snapshots_from_rows(store.reviews_for_root("/nonexistent/debt-proj").unwrap()); + assert_eq!(snaps.len(), 1); + let s = &snaps[0]; + assert_eq!(s.commit.as_deref(), Some("abc")); + assert_eq!(s.files_reviewed, 2); + assert_eq!(s.lines_reviewed, None); + assert_eq!(s.gate_status, "failed"); + assert!((s.quality_score - 7.7).abs() < 1e-9); // 100-20-3 = 77 -> 7.7 + assert_eq!(s.findings.get("critical"), Some(&1)); + assert_eq!(s.findings.get("minor"), Some(&1)); + assert_eq!(s.categories.get("security"), Some(&1)); + assert_eq!(s.categories.get("style"), Some(&1)); + } + // ─── count_by_severity ─── #[test] diff --git a/src/engine/mod.rs b/src/engine/mod.rs index b159f88..9b1dd5d 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -3,7 +3,6 @@ pub mod cache; pub mod chunker; pub mod comment_sanitizer; pub mod context; -pub mod db_writer; pub mod debt_tracker; pub mod deterministic; pub mod diff_parser; @@ -18,6 +17,7 @@ pub mod path_match; pub mod profiles; pub mod quality_gate; pub mod review; +pub mod review_store; pub mod rules; pub mod scanner; pub mod secret_patterns; diff --git a/src/engine/review_store.rs b/src/engine/review_store.rs new file mode 100644 index 0000000..03b275f --- /dev/null +++ b/src/engine/review_store.rs @@ -0,0 +1,926 @@ +//! Review-history store: the one module that owns SQL for the `reviews`, +//! `findings` and `finding_events` tables (schema lives in `index/schema.rs`). +//! +//! # Shape +//! +//! [`ReviewStore`] wraps a borrowed [`Connection`], so every operation is +//! testable against an in-memory SQLite database. Commands (`cora findings`, +//! `cora debt`, review/scan persistence) call it and contain no SQL. +//! +//! # Error policy (decided here, in one place) +//! +//! * **Store methods return `Result`.** Callers that can act on a failure +//! (the `cora findings` CLI) surface it. +//! * **Opening** ([`open_read`], [`open_write`]) returns `Result` too; the CLI +//! prints its "could not open cora.db" message on `Err`. +//! * **Persisting a review is best-effort.** [`persist_review_best_effort`] is +//! the single place that swallows write errors (logging at `debug`), so a +//! failure to write history can never fail or block a review/scan. +//! * The read path used only for reporting ([`load_debt_rows`]) degrades to an +//! empty result, the same as a missing database. + +use std::path::Path; + +use anyhow::{Context, Result}; +use rusqlite::{Connection, OptionalExtension}; +use tracing::debug; + +use crate::engine::Severity; +use crate::engine::types::{ReviewIssue, TokenUsage}; +use crate::index::schema; + +/// Input data for saving a review/scan run to the database. +pub struct ReviewRecord<'a> { + /// "review" or "scan". + pub command: &'a str, + /// Absolute path of the project root (used for project lookup/creation). + pub project_root: &'a str, + /// Git commit hash (short) if available. + pub commit_hash: Option<&'a str>, + /// Git branch name if available. + pub branch: Option<&'a str>, + /// LLM-generated summary text. + pub summary: &'a str, + /// Quality gate status: "passed", "failed", or "disabled". + pub gate_status: &'a str, + /// Number of files scanned/reviewed. + pub files_scanned: usize, + /// Number of lines scanned/reviewed. + pub lines_scanned: usize, + /// Whether the quality gate should block. + pub should_block: bool, + /// Token usage from the LLM call (if any). + pub tokens: Option<&'a TokenUsage>, + /// The issues/findings to persist. + pub issues: &'a [ReviewIssue], +} + +/// Filters for [`ReviewStore::list_findings`]. +#[derive(Debug, Clone)] +pub struct FindingFilter { + /// Include resolved/dismissed findings (default: open only). + pub all: bool, + /// Exact severity match; compared upper-cased. + pub severity: Option, + /// Substring match on the file path. + pub file: Option, + /// Maximum number of rows. + pub limit: usize, +} + +/// One row of `cora findings list`. +#[derive(Debug, Clone, serde::Serialize)] +pub struct FindingRow { + pub id: i64, + pub severity: String, + pub file_path: String, + pub line_number: Option, + pub title: String, + pub status: String, + pub fingerprint: Option, + pub created_at: String, +} + +/// Counts for `cora findings stats`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct FindingStats { + pub total: i64, + pub open: i64, + pub resolved: i64, + pub dismissed: i64, + pub reviews: i64, +} + +/// Result of a manual status transition. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Transition { + /// No finding with that id. + NotFound, + /// Finding exists but is already in the target state; nothing written. + Unchanged, + /// Status updated and an audit event written. + Applied, +} + +/// A `reviews` row as needed by the debt tracker. +#[derive(Debug, Clone)] +pub struct ReviewRow { + pub id: i64, + pub commit_hash: Option, + pub branch: Option, + pub files_scanned: i64, + pub lines_scanned: i64, + /// 0-100 score as stored. + pub score: f64, + pub gate_status: String, + pub created_at: String, + /// Open findings per severity (severity as stored -> count). + pub open_by_severity: Vec<(String, usize)>, + /// Open findings per raw `issue_type` -> count. + pub open_by_issue_type: Vec<(String, usize)>, +} + +/// Outcome of [`persist_review_best_effort`]. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] +pub struct PersistOutcome { + /// `Some(review_id)` when the review row was written. + pub review_id: Option, + /// Number of stale findings auto-resolved. + pub auto_resolved: usize, +} + +// ─── Connection acquisition ─── + +/// Open the global `cora.db` read-only (no migrations, no PRAGMAs). +/// Errors if the database does not exist or cannot be opened. +pub fn open_read() -> Result { + open_read_at(&crate::data_dir::graph_db_path()) +} + +/// Read-only open of an arbitrary path (see [`open_read`]). +pub fn open_read_at(db_path: &Path) -> Result { + if !db_path.exists() { + anyhow::bail!("{} does not exist", db_path.display()); + } + Connection::open_with_flags(db_path, rusqlite::OpenFlags::SQLITE_OPEN_READ_ONLY) + .with_context(|| format!("opening {} read-only", db_path.display())) +} + +/// Open the global `cora.db` read-write, running migrations if needed. +pub fn open_write() -> Result { + crate::index::open_global_index() +} + +// ─── Best-effort persistence (the single swallow-errors policy) ─── + +/// Persist a review/scan and auto-resolve findings that no longer appear. +/// +/// Never fails: any error is logged at `debug` and reflected in the returned +/// [`PersistOutcome`]. Persisting history must not fail or block a review. +pub fn persist_review_best_effort(record: &ReviewRecord<'_>) -> PersistOutcome { + match open_write() { + Ok(conn) => persist_on(&conn, record), + Err(e) => { + debug!(error = %e, "could not open cora.db; review history not saved"); + PersistOutcome::default() + } + } +} + +/// Best-effort persistence on a given connection (testable without `$HOME`). +pub fn persist_on(conn: &Connection, record: &ReviewRecord<'_>) -> PersistOutcome { + let store = ReviewStore::new(conn); + + let review_id = match store.record_review(record) { + Ok(id) => Some(id), + Err(e) => { + debug!(error = %e, "failed to save {} to cora.db", record.command); + None + } + }; + + // Resolution runs even if the insert failed (as it always has). + let fps: Vec = record.issues.iter().map(compute_fingerprint).collect(); + let auto_resolved = match store.resolve_stale(record.project_root, &fps) { + Ok(n) => n, + Err(e) => { + debug!(error = %e, "failed to auto-resolve stale findings"); + 0 + } + }; + if auto_resolved > 0 { + debug!(resolved = auto_resolved, "auto-resolved stale findings"); + } + + PersistOutcome { + review_id, + auto_resolved, + } +} + +/// Read review history for the debt tracker. Degrades to an empty list when +/// the database is missing/unreadable or the project has no reviews. +pub fn load_debt_rows(project_root: &str) -> Vec { + let Ok(conn) = open_read() else { + return Vec::new(); + }; + match ReviewStore::new(&conn).reviews_for_root(project_root) { + Ok(rows) => rows, + Err(e) => { + debug!(error = %e, "could not read review history"); + Vec::new() + } + } +} + +// ─── Store ─── + +/// All SQL for review history, over a borrowed connection. +pub struct ReviewStore<'a> { + conn: &'a Connection, +} + +impl<'a> ReviewStore<'a> { + pub fn new(conn: &'a Connection) -> Self { + Self { conn } + } + + /// Insert a review row plus one `open` finding (and `opened` event) per + /// issue, atomically. Returns the new `review_id`. + pub fn record_review(&self, record: &ReviewRecord<'_>) -> Result { + let project_id = schema::get_or_create_project(self.conn, record.project_root)?; + + let (input_tokens, output_tokens, cost_usd) = record + .tokens + .map(|t| { + ( + t.input_tokens as i64, + t.output_tokens as i64, + t.estimated_cost_usd, + ) + }) + .unwrap_or((0, 0, 0.0)); + + let tx = self.conn.unchecked_transaction()?; + + tx.execute( + "INSERT INTO reviews + (project_id, command, commit_hash, branch, summary, score, gate_status, + files_scanned, lines_scanned, should_block, input_tokens, output_tokens, cost_usd) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12, ?13)", + rusqlite::params![ + project_id, + record.command, + record.commit_hash, + record.branch, + record.summary, + calculate_score(record.issues) as i64, + record.gate_status, + record.files_scanned as i64, + record.lines_scanned as i64, + record.should_block as i64, + input_tokens, + output_tokens, + cost_usd, + ], + )?; + let review_id = tx.last_insert_rowid(); + + { + let mut stmt_findings = tx.prepare( + "INSERT INTO findings + (review_id, file_path, line_number, severity, issue_type, title, body, + suggested_fix, status, fingerprint) + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, 'open', ?9)", + )?; + let mut stmt_events = tx.prepare( + "INSERT INTO finding_events (finding_id, event_type, note) + VALUES (?1, 'opened', NULL)", + )?; + + for issue in record.issues { + stmt_findings.execute(rusqlite::params![ + review_id, + issue.file, + issue.line.map(|l| l as i64), + issue.severity.to_string(), + issue.issue_type.as_deref(), + issue.title, + issue.body, + issue.suggested_fix.as_deref(), + compute_fingerprint(issue), + ])?; + stmt_events.execute(rusqlite::params![tx.last_insert_rowid()])?; + } + } + + tx.commit()?; + Ok(review_id) + } + + /// Auto-resolve `open` findings of the project whose fingerprint is not in + /// `current_fingerprints`, writing an `auto_resolved` event for each. + /// Returns how many findings were resolved. + pub fn resolve_stale( + &self, + project_root: &str, + current_fingerprints: &[String], + ) -> Result { + let project_id = schema::get_or_create_project(self.conn, project_root)?; + + let candidates: Vec<(i64, String)> = { + let mut stmt = self.conn.prepare( + "SELECT f.id, f.fingerprint FROM findings f + JOIN reviews r ON f.review_id = r.id + WHERE r.project_id = ?1 + AND f.status = 'open' + AND f.fingerprint IS NOT NULL", + )?; + stmt.query_map(rusqlite::params![project_id], |r| { + Ok((r.get(0)?, r.get(1)?)) + })? + .collect::>()? + }; + + let tx = self.conn.unchecked_transaction()?; + let mut resolved = 0; + { + let mut update = tx.prepare("UPDATE findings SET status = 'resolved' WHERE id = ?1")?; + let mut event = tx.prepare( + "INSERT INTO finding_events (finding_id, event_type, note) + VALUES (?1, 'auto_resolved', 'No longer found in latest review')", + )?; + for (id, fp) in &candidates { + if current_fingerprints.contains(fp) { + continue; + } + update.execute(rusqlite::params![id])?; + event.execute(rusqlite::params![id])?; + resolved += 1; + } + } + tx.commit()?; + Ok(resolved) + } + + /// List findings (newest id first) joined with their review's timestamp. + pub fn list_findings(&self, filter: &FindingFilter) -> Result> { + let mut sql = String::from( + "SELECT f.id, f.severity, f.file_path, f.line_number, f.title, f.status, + f.fingerprint, r.created_at + FROM findings f + JOIN reviews r ON f.review_id = r.id", + ); + + // Parameterized placeholders only (no user text in the SQL string). + let mut wheres: Vec<&str> = Vec::new(); + let mut params: Vec> = Vec::new(); + + if !filter.all { + wheres.push("f.status = 'open'"); + } + if let Some(s) = &filter.severity { + wheres.push("f.severity = ?"); + params.push(Box::new(s.to_uppercase())); + } + if let Some(f) = &filter.file { + wheres.push("f.file_path LIKE ?"); + params.push(Box::new(format!("%{f}%"))); + } + if !wheres.is_empty() { + sql.push_str(" WHERE "); + sql.push_str(&wheres.join(" AND ")); + } + sql.push_str(" ORDER BY f.id DESC LIMIT ?"); + params.push(Box::new(filter.limit as i64)); + + let param_refs: Vec<&dyn rusqlite::ToSql> = params.iter().map(|p| p.as_ref()).collect(); + let mut stmt = self.conn.prepare(&sql)?; + let rows = stmt + .query_map(param_refs.as_slice(), |r| { + Ok(FindingRow { + id: r.get(0)?, + severity: r.get(1)?, + file_path: r.get(2)?, + line_number: r.get(3)?, + title: r.get(4)?, + status: r.get(5)?, + fingerprint: r.get(6)?, + created_at: r.get(7)?, + }) + })? + .collect::>>()?; + Ok(rows) + } + + /// Aggregate counts over all findings and reviews. + pub fn stats(&self) -> Result { + let count = |sql: &str| -> Result { Ok(self.conn.query_row(sql, [], |r| r.get(0))?) }; + Ok(FindingStats { + total: count("SELECT count(*) FROM findings")?, + open: count("SELECT count(*) FROM findings WHERE status = 'open'")?, + resolved: count("SELECT count(*) FROM findings WHERE status = 'resolved'")?, + dismissed: count("SELECT count(*) FROM findings WHERE status = 'dismissed'")?, + reviews: count("SELECT count(*) FROM reviews")?, + }) + } + + /// Current status of a finding, or `None` if it does not exist. + pub fn finding_status(&self, id: i64) -> Result> { + Ok(self + .conn + .query_row( + "SELECT status FROM findings WHERE id = ?1", + rusqlite::params![id], + |r| r.get(0), + ) + .optional()?) + } + + /// Mark a finding dismissed and write a `dismissed` event. Dismissing an + /// already-dismissed (or resolved) finding is allowed and re-recorded. + pub fn dismiss(&self, id: i64, reason: Option<&str>) -> Result { + if self.finding_status(id)?.is_none() { + return Ok(Transition::NotFound); + } + let note = reason.unwrap_or("Manually dismissed via CLI"); + self.transition(id, "dismissed", "dismissed", note)?; + Ok(Transition::Applied) + } + + /// Reopen a resolved/dismissed finding and write a `reopened` event. + /// An already-open finding is left untouched ([`Transition::Unchanged`]). + pub fn reopen(&self, id: i64) -> Result { + match self.finding_status(id)?.as_deref() { + None => Ok(Transition::NotFound), + Some("open") => Ok(Transition::Unchanged), + Some(_) => { + self.transition(id, "open", "reopened", "Manually reopened via CLI")?; + Ok(Transition::Applied) + } + } + } + + /// Status update + audit event in one transaction. + fn transition(&self, id: i64, status: &str, event: &str, note: &str) -> Result<()> { + let tx = self.conn.unchecked_transaction()?; + tx.execute( + "UPDATE findings SET status = ?2 WHERE id = ?1", + rusqlite::params![id, status], + )?; + tx.execute( + "INSERT INTO finding_events (finding_id, event_type, note) VALUES (?1, ?2, ?3)", + rusqlite::params![id, event, note], + )?; + tx.commit()?; + Ok(()) + } + + /// All reviews of the project at `project_root` (oldest first) with their + /// open-finding breakdowns. Empty when the project is unknown. + pub fn reviews_for_root(&self, project_root: &str) -> Result> { + let canonical = match Path::new(project_root).canonicalize() { + Ok(p) => p.to_string_lossy().to_string(), + Err(_) => project_root.to_string(), + }; + + let project_id: Option = self + .conn + .query_row( + "SELECT id FROM projects WHERE root_path = ?1", + rusqlite::params![canonical], + |r| r.get(0), + ) + .optional()?; + let Some(project_id) = project_id else { + return Ok(Vec::new()); + }; + + let mut stmt = self.conn.prepare( + "SELECT id, commit_hash, branch, files_scanned, lines_scanned, + score, gate_status, created_at + FROM reviews WHERE project_id = ?1 ORDER BY created_at ASC", + )?; + let mut rows: Vec = stmt + .query_map(rusqlite::params![project_id], |r| { + Ok(ReviewRow { + id: r.get(0)?, + commit_hash: r.get(1)?, + branch: r.get(2)?, + files_scanned: r.get(3)?, + lines_scanned: r.get(4)?, + score: r.get(5)?, + gate_status: r.get(6)?, + created_at: r.get(7)?, + open_by_severity: Vec::new(), + open_by_issue_type: Vec::new(), + }) + })? + .collect::>()?; + + for row in &mut rows { + row.open_by_severity = self.open_counts( + "SELECT severity, COUNT(*) FROM findings + WHERE review_id = ?1 AND status = 'open' + GROUP BY severity", + row.id, + )?; + row.open_by_issue_type = self.open_counts( + "SELECT issue_type, COUNT(*) FROM findings + WHERE review_id = ?1 AND status = 'open' AND issue_type IS NOT NULL + GROUP BY issue_type", + row.id, + )?; + } + Ok(rows) + } + + fn open_counts(&self, sql: &str, review_id: i64) -> Result> { + let mut stmt = self.conn.prepare(sql)?; + let counts = stmt + .query_map(rusqlite::params![review_id], |r| { + let key: Option = r.get(0)?; + let n: i64 = r.get(1)?; + Ok((key.unwrap_or_default(), n as usize)) + })? + .collect::>()?; + Ok(counts) + } +} + +// ─── Pure helpers ─── + +/// Compute a fingerprint for dedup/auto-resolve: `file:line:title_slug`. +pub fn compute_fingerprint(issue: &ReviewIssue) -> String { + let line = issue.line.unwrap_or(0); + let title_slug = issue.title.to_lowercase().replace(' ', "_"); + format!("{}:{}:{}", issue.file, line, title_slug) +} + +/// Calculate a quality score 0-100 from issue severities. +/// +/// 100 = no issues. Each finding reduces the score: +/// - critical: -20, major: -10, minor: -3, info: -1 +fn calculate_score(issues: &[ReviewIssue]) -> f64 { + let mut score: f64 = 100.0; + for issue in issues { + let penalty: f64 = match issue.severity { + Severity::Critical => 20.0, + Severity::Major => 10.0, + Severity::Minor => 3.0, + Severity::Info => 1.0, + }; + score -= penalty; + } + score.max(0.0) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn mem() -> Connection { + let conn = Connection::open_in_memory().unwrap(); + conn.execute_batch("PRAGMA foreign_keys=ON;").unwrap(); + schema::run_migrations(&conn).unwrap(); + conn + } + + fn make_issue(file: &str, line: u32, severity: Severity, title: &str) -> ReviewIssue { + ReviewIssue { + file: file.to_string(), + line: Some(line), + severity, + issue_type: Some("security".to_string()), + title: title.to_string(), + body: "test body".to_string(), + suggested_fix: Some("fix it".to_string()), + } + } + + fn record<'a>(root: &'a str, issues: &'a [ReviewIssue]) -> ReviewRecord<'a> { + ReviewRecord { + command: "review", + project_root: root, + commit_hash: Some("abc123"), + branch: Some("main"), + summary: "sum", + gate_status: "passed", + files_scanned: 3, + lines_scanned: 40, + should_block: false, + tokens: None, + issues, + } + } + + fn events(conn: &Connection, finding: i64) -> Vec<(String, Option)> { + let mut s = conn + .prepare( + "SELECT event_type, note FROM finding_events WHERE finding_id = ?1 ORDER BY id", + ) + .unwrap(); + s.query_map([finding], |r| Ok((r.get(0)?, r.get(1)?))) + .unwrap() + .map(|r| r.unwrap()) + .collect() + } + + fn filter(all: bool) -> FindingFilter { + FindingFilter { + all, + severity: None, + file: None, + limit: 50, + } + } + + #[test] + fn test_fingerprint_format() { + let issue = make_issue("src/main.rs", 42, Severity::Critical, "SQL Injection"); + assert_eq!(compute_fingerprint(&issue), "src/main.rs:42:sql_injection"); + let mut issue = make_issue("src/lib.rs", 0, Severity::Minor, "Unused Import"); + issue.line = None; + assert_eq!(compute_fingerprint(&issue), "src/lib.rs:0:unused_import"); + } + + #[test] + fn test_score() { + assert_eq!(calculate_score(&[]), 100.0); + let issues = vec![ + make_issue("a.rs", 1, Severity::Critical, "c"), + make_issue("b.rs", 2, Severity::Major, "m"), + make_issue("c.rs", 3, Severity::Minor, "n"), + make_issue("d.rs", 4, Severity::Info, "i"), + ]; + assert_eq!(calculate_score(&issues), 66.0); + let many: Vec<_> = (0..6) + .map(|i| make_issue("a.rs", i, Severity::Critical, "x")) + .collect(); + assert_eq!(calculate_score(&many), 0.0); + } + + #[test] + fn record_review_round_trip() { + let conn = mem(); + let store = ReviewStore::new(&conn); + let issues = vec![ + make_issue("src/a.rs", 10, Severity::Critical, "Bad Thing"), + make_issue("src/b.rs", 20, Severity::Minor, "Meh"), + ]; + let id = store.record_review(&record("/proj", &issues)).unwrap(); + + let (cmd, commit, score, gate, files): (String, String, i64, String, i64) = conn + .query_row( + "SELECT command, commit_hash, score, gate_status, files_scanned + FROM reviews WHERE id = ?1", + [id], + |r| Ok((r.get(0)?, r.get(1)?, r.get(2)?, r.get(3)?, r.get(4)?)), + ) + .unwrap(); + assert_eq!( + (cmd.as_str(), commit.as_str(), score, gate.as_str(), files), + ("review", "abc123", 77, "passed", 3) + ); + + let rows = store.list_findings(&filter(false)).unwrap(); + assert_eq!(rows.len(), 2); + assert_eq!(rows[0].file_path, "src/b.rs"); // newest id first + assert_eq!( + rows[1].fingerprint.as_deref(), + Some("src/a.rs:10:bad_thing") + ); + assert_eq!(rows[1].severity, "critical"); // stored lowercase + assert_eq!(rows[1].status, "open"); + for r in &rows { + assert_eq!(events(&conn, r.id), vec![("opened".to_string(), None)]); + } + } + + #[test] + fn record_review_is_atomic() { + let conn = mem(); + conn.execute_batch("DROP TABLE finding_events").unwrap(); + let issues = vec![make_issue("a.rs", 1, Severity::Info, "x")]; + assert!( + ReviewStore::new(&conn) + .record_review(&record("/p", &issues)) + .is_err() + ); + let n: i64 = conn + .query_row("SELECT count(*) FROM reviews", [], |r| r.get(0)) + .unwrap(); + assert_eq!(n, 0, "failed record must roll back the review row"); + } + + #[test] + fn list_filters() { + let conn = mem(); + let store = ReviewStore::new(&conn); + let issues = vec![ + make_issue("src/a.rs", 1, Severity::Critical, "one"), + make_issue("lib/b.rs", 2, Severity::Minor, "two"), + make_issue("src/c.rs", 3, Severity::Minor, "three"), + ]; + store.record_review(&record("/p", &issues)).unwrap(); + store.dismiss(3, None).unwrap(); + + assert_eq!(store.list_findings(&filter(false)).unwrap().len(), 2); + assert_eq!(store.list_findings(&filter(true)).unwrap().len(), 3); + + let mut f = filter(true); + f.file = Some("src/".into()); + assert_eq!(store.list_findings(&f).unwrap().len(), 2); + f.all = false; // open only: #3 is dismissed + let rows = store.list_findings(&f).unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].title, "one"); + + // Legacy behaviour kept on purpose (refactor, not a fix): severities are + // stored lowercase but the filter upper-cases its argument, so + // `--severity` never matches. Tracked separately; this pins the status quo. + let mut f = filter(true); + f.severity = Some("minor".into()); + assert_eq!(store.list_findings(&f).unwrap().len(), 0); + + let mut f = filter(true); + f.limit = 1; + let rows = store.list_findings(&f).unwrap(); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].id, 3); + } + + #[test] + fn dismiss_and_reopen_transitions() { + let conn = mem(); + let store = ReviewStore::new(&conn); + let issues = vec![make_issue("a.rs", 1, Severity::Major, "t")]; + store.record_review(&record("/p", &issues)).unwrap(); + + assert_eq!(store.dismiss(99, None).unwrap(), Transition::NotFound); + assert_eq!(store.reopen(99).unwrap(), Transition::NotFound); + // already open: nothing written + assert_eq!(store.reopen(1).unwrap(), Transition::Unchanged); + assert_eq!(events(&conn, 1).len(), 1); + + assert_eq!( + store.dismiss(1, Some("wontfix")).unwrap(), + Transition::Applied + ); + assert_eq!( + store.finding_status(1).unwrap().as_deref(), + Some("dismissed") + ); + assert_eq!( + events(&conn, 1).last().unwrap(), + &("dismissed".to_string(), Some("wontfix".to_string())) + ); + + // dismissing again re-records (unchanged legacy behaviour), default note + assert_eq!(store.dismiss(1, None).unwrap(), Transition::Applied); + assert_eq!( + events(&conn, 1).last().unwrap().1.as_deref(), + Some("Manually dismissed via CLI") + ); + + assert_eq!(store.reopen(1).unwrap(), Transition::Applied); + assert_eq!(store.finding_status(1).unwrap().as_deref(), Some("open")); + assert_eq!( + events(&conn, 1).last().unwrap(), + &( + "reopened".to_string(), + Some("Manually reopened via CLI".to_string()) + ) + ); + } + + #[test] + fn reopen_after_auto_resolve() { + let conn = mem(); + let store = ReviewStore::new(&conn); + let issues = vec![make_issue("a.rs", 1, Severity::Major, "t")]; + store.record_review(&record("/p", &issues)).unwrap(); + assert_eq!(store.resolve_stale("/p", &[]).unwrap(), 1); + assert_eq!(store.reopen(1).unwrap(), Transition::Applied); + assert_eq!(store.stats().unwrap().open, 1); + } + + #[test] + fn resolve_stale_only_missing_fingerprints_in_project() { + let conn = mem(); + let store = ReviewStore::new(&conn); + let a = make_issue("a.rs", 1, Severity::Major, "keep"); + let b = make_issue("b.rs", 2, Severity::Major, "gone"); + store.record_review(&record("/p", &[a.clone(), b])).unwrap(); + // another project's finding must be untouched + store + .record_review(&record( + "/other", + &[make_issue("z.rs", 9, Severity::Minor, "z")], + )) + .unwrap(); + + let n = store + .resolve_stale("/p", &[compute_fingerprint(&a)]) + .unwrap(); + assert_eq!(n, 1); + assert_eq!(store.finding_status(1).unwrap().as_deref(), Some("open")); + assert_eq!( + store.finding_status(2).unwrap().as_deref(), + Some("resolved") + ); + assert_eq!(store.finding_status(3).unwrap().as_deref(), Some("open")); + assert_eq!( + events(&conn, 2).last().unwrap(), + &( + "auto_resolved".to_string(), + Some("No longer found in latest review".to_string()) + ) + ); + } + + #[test] + fn stats_counts() { + let conn = mem(); + let store = ReviewStore::new(&conn); + let zero = FindingStats { + total: 0, + open: 0, + resolved: 0, + dismissed: 0, + reviews: 0, + }; + assert_eq!(store.stats().unwrap(), zero); + let issues = vec![ + make_issue("a.rs", 1, Severity::Major, "a"), + make_issue("b.rs", 2, Severity::Major, "b"), + make_issue("c.rs", 3, Severity::Major, "c"), + ]; + store.record_review(&record("/p", &issues)).unwrap(); + store.dismiss(1, None).unwrap(); + store + .resolve_stale("/p", &[compute_fingerprint(&issues[2])]) + .unwrap(); + assert_eq!( + store.stats().unwrap(), + FindingStats { + total: 3, + open: 1, + resolved: 1, + dismissed: 1, + reviews: 1 + } + ); + } + + #[test] + fn debt_reads_against_store() { + let conn = mem(); + let store = ReviewStore::new(&conn); + let i1 = make_issue("a.rs", 1, Severity::Critical, "a"); + let mut i2 = make_issue("b.rs", 2, Severity::Minor, "b"); + i2.issue_type = None; + let i3 = make_issue("c.rs", 3, Severity::Critical, "c"); + // nonexistent path: reviews_for_root falls back to the raw string + store + .record_review(&record("/nonexistent/proj", &[i1, i2, i3])) + .unwrap(); + store.dismiss(3, None).unwrap(); // dismissed findings are not counted + store + .record_review(&record("/nonexistent/proj", &[])) + .unwrap(); + + let rows = store.reviews_for_root("/nonexistent/proj").unwrap(); + assert_eq!(rows.len(), 2); + let r = &rows[0]; + assert_eq!((r.files_scanned, r.lines_scanned, r.score), (3, 40, 57.0)); + assert_eq!(r.commit_hash.as_deref(), Some("abc123")); + let mut sev = r.open_by_severity.clone(); + sev.sort(); + assert_eq!( + sev, + vec![("critical".to_string(), 1), ("minor".to_string(), 1)] + ); + assert_eq!(r.open_by_issue_type, vec![("security".to_string(), 1)]); + assert!(rows[1].open_by_severity.is_empty()); + + assert!(store.reviews_for_root("/unknown").unwrap().is_empty()); + } + + #[test] + fn persist_records_and_auto_resolves() { + let conn = mem(); + let old = make_issue("a.rs", 1, Severity::Major, "old"); + let r1 = persist_on(&conn, &record("/p", std::slice::from_ref(&old))); + assert_eq!(r1.review_id, Some(1)); + assert_eq!(r1.auto_resolved, 0); + + let new = make_issue("b.rs", 2, Severity::Minor, "new"); + let r2 = persist_on(&conn, &record("/p", &[new])); + assert_eq!(r2.review_id, Some(2)); + assert_eq!(r2.auto_resolved, 1); + let store = ReviewStore::new(&conn); + assert_eq!( + store.finding_status(1).unwrap().as_deref(), + Some("resolved") + ); + assert_eq!(store.finding_status(2).unwrap().as_deref(), Some("open")); + } + + #[test] + fn persist_is_best_effort_when_write_fails() { + // Broken schema: persisting must not panic or error, just report nothing. + let conn = mem(); + conn.execute_batch("DROP TABLE finding_events; DROP TABLE findings;") + .unwrap(); + let issues = vec![make_issue("a.rs", 1, Severity::Info, "x")]; + let out = persist_on(&conn, &record("/p", &issues)); + assert_eq!(out, PersistOutcome::default()); + } + + #[test] + fn open_read_missing_db_errors() { + let dir = tempfile::tempdir().unwrap(); + assert!(open_read_at(&dir.path().join("nope.db")).is_err()); + } +}