From c36ce8939b8832065d8e668cf0d61a9300f2daba Mon Sep 17 00:00:00 2001 From: Jonathan Haas Date: Wed, 11 Mar 2026 13:05:20 -0700 Subject: [PATCH 1/8] =?UTF-8?q?feat:=20audit=20fixes=20=E2=80=94=20gracefu?= =?UTF-8?q?l=20OTEL,=20async-safe=20plugins,=20URL=20validation,=20+100=20?= =?UTF-8?q?tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - OTEL init no longer panics on misconfigured endpoints; falls back gracefully - Semgrep/ESLint plugins use tokio::spawn_blocking to avoid blocking the runtime - Added semgrep --timeout flag (30s) for process execution safety - Git clone URL validation: only https://, git@, and http://*.git accepted - Added PartialEq derive to ContextType for test assertions - 100+ new tests across storage_json, interactive commands, plugins, CLI commands (pr.rs, git.rs), and duplicate_filter Co-Authored-By: Claude Opus 4.6 --- src/core/context.rs | 2 +- src/core/interactive.rs | 182 ++++ src/main.rs | 60 +- src/plugins/builtin/duplicate_filter.rs | 99 +++ src/plugins/builtin/eslint.rs | 82 +- src/plugins/builtin/semgrep.rs | 74 +- src/review/context_helpers.rs | 86 +- src/server/storage_json.rs | 1006 +++++++++++++++++++++++ 8 files changed, 1543 insertions(+), 48 deletions(-) diff --git a/src/core/context.rs b/src/core/context.rs index faea834..62ab295 100644 --- a/src/core/context.rs +++ b/src/core/context.rs @@ -15,7 +15,7 @@ pub struct LLMContextChunk { pub line_range: Option<(usize, usize)>, } -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] pub enum ContextType { FileContent, Definition, diff --git a/src/core/interactive.rs b/src/core/interactive.rs index 280c16e..4e3595a 100644 --- a/src/core/interactive.rs +++ b/src/core/interactive.rs @@ -246,6 +246,188 @@ Interactive commands respect these configurations."# } } +#[cfg(test)] +mod tests { + use super::*; + + // === InteractiveCommand::parse tests === + + #[test] + fn test_parse_review_command() { + let cmd = InteractiveCommand::parse("@diffscope review").unwrap(); + assert_eq!(cmd.command, CommandType::Review); + assert!(cmd.args.is_empty()); + assert!(cmd.context.is_none()); + } + + #[test] + fn test_parse_review_with_focus() { + let cmd = InteractiveCommand::parse("@diffscope review security performance").unwrap(); + assert_eq!(cmd.command, CommandType::Review); + assert_eq!(cmd.args, vec!["security", "performance"]); + } + + #[test] + fn test_parse_ignore_command() { + let cmd = InteractiveCommand::parse("@diffscope ignore src/generated/").unwrap(); + assert_eq!(cmd.command, CommandType::Ignore); + assert_eq!(cmd.args, vec!["src/generated/"]); + } + + #[test] + fn test_parse_explain_command() { + let cmd = InteractiveCommand::parse("@diffscope explain").unwrap(); + assert_eq!(cmd.command, CommandType::Explain); + } + + #[test] + fn test_parse_generate_tests() { + let cmd = InteractiveCommand::parse("@diffscope generate tests").unwrap(); + assert_eq!(cmd.command, CommandType::Generate); + assert_eq!(cmd.args, vec!["tests"]); + } + + #[test] + fn test_parse_help() { + let cmd = InteractiveCommand::parse("@diffscope help").unwrap(); + assert_eq!(cmd.command, CommandType::Help); + } + + #[test] + fn test_parse_config() { + let cmd = InteractiveCommand::parse("@diffscope config").unwrap(); + assert_eq!(cmd.command, CommandType::Config); + } + + #[test] + fn test_parse_case_insensitive() { + let cmd = InteractiveCommand::parse("@diffscope REVIEW").unwrap(); + assert_eq!(cmd.command, CommandType::Review); + + let cmd = InteractiveCommand::parse("@diffscope Help").unwrap(); + assert_eq!(cmd.command, CommandType::Help); + } + + #[test] + fn test_parse_unknown_command_returns_none() { + assert!(InteractiveCommand::parse("@diffscope foobar").is_none()); + } + + #[test] + fn test_parse_no_at_mention_returns_none() { + assert!(InteractiveCommand::parse("just a regular comment").is_none()); + } + + #[test] + fn test_parse_empty_string_returns_none() { + assert!(InteractiveCommand::parse("").is_none()); + } + + #[test] + fn test_parse_at_mention_only_returns_none() { + assert!(InteractiveCommand::parse("@diffscope").is_none()); + } + + #[test] + fn test_parse_embedded_in_text() { + // Command embedded in longer comment + let cmd = + InteractiveCommand::parse("Hey team, can you @diffscope review this PR?").unwrap(); + assert_eq!(cmd.command, CommandType::Review); + } + + #[test] + fn test_parse_extra_whitespace() { + let cmd = InteractiveCommand::parse("@diffscope review security").unwrap(); + assert_eq!(cmd.command, CommandType::Review); + assert_eq!(cmd.args, vec!["security"]); + } + + #[test] + fn test_parse_different_at_mention_ignored() { + assert!(InteractiveCommand::parse("@someone review").is_none()); + } + + // === execute_ignore tests === + + #[test] + fn test_ignore_no_args() { + let cmd = InteractiveCommand { + command: CommandType::Ignore, + args: vec![], + context: None, + }; + let result = cmd.execute_ignore().unwrap(); + assert!(result.contains("specify what to ignore")); + } + + #[test] + fn test_ignore_with_patterns() { + let cmd = InteractiveCommand { + command: CommandType::Ignore, + args: vec!["src/generated/".to_string(), "*.test.js".to_string()], + context: None, + }; + let result = cmd.execute_ignore().unwrap(); + assert!(result.contains("src/generated/")); + assert!(result.contains("*.test.js")); + } + + // === help_text / config tests === + + #[test] + fn test_help_text_contains_commands() { + let help = InteractiveCommand::help_text(); + assert!(help.contains("@diffscope review")); + assert!(help.contains("@diffscope ignore")); + assert!(help.contains("@diffscope explain")); + assert!(help.contains("@diffscope generate")); + assert!(help.contains("@diffscope help")); + assert!(help.contains("@diffscope config")); + } + + #[test] + fn test_config_info_contains_yaml() { + let info = InteractiveCommand::get_config_info(); + assert!(info.contains(".diffscope.yml")); + assert!(info.contains("exclude_patterns")); + } + + // === InteractiveProcessor tests === + + #[test] + fn test_processor_new_empty() { + let processor = InteractiveProcessor::new(); + assert!(!processor.should_ignore("any/path")); + } + + #[test] + fn test_processor_add_and_check_pattern() { + let mut processor = InteractiveProcessor::new(); + processor.add_ignore_pattern("src/generated/"); + assert!(processor.should_ignore("src/generated/types.rs")); + assert!(!processor.should_ignore("src/main.rs")); + } + + #[test] + fn test_processor_glob_pattern() { + let mut processor = InteractiveProcessor::new(); + processor.add_ignore_pattern("*.test.js"); + assert!(processor.should_ignore("foo.test.js")); + assert!(!processor.should_ignore("foo.js")); + } + + #[test] + fn test_processor_multiple_patterns() { + let mut processor = InteractiveProcessor::new(); + processor.add_ignore_pattern("vendor/"); + processor.add_ignore_pattern("*.generated.*"); + assert!(processor.should_ignore("vendor/lib.js")); + assert!(processor.should_ignore("types.generated.ts")); + assert!(!processor.should_ignore("src/main.rs")); + } +} + /// Manages per-session ignore patterns from @diffscope ignore commands. /// Will be wired into the review pipeline's triage filter. #[allow(dead_code)] diff --git a/src/main.rs b/src/main.rs index 3405cb1..b981969 100644 --- a/src/main.rs +++ b/src/main.rs @@ -304,32 +304,44 @@ async fn main() -> Result<()> { let _otel_guard: Option = { let otel_enabled = std::env::var("OTEL_EXPORTER_OTLP_ENDPOINT").is_ok(); if otel_enabled { - let exporter = opentelemetry_otlp::SpanExporter::builder() + match opentelemetry_otlp::SpanExporter::builder() .with_tonic() .build() - .expect("Failed to build OTLP span exporter"); - - let tracer_provider = opentelemetry_sdk::trace::TracerProvider::builder() - .with_batch_exporter(exporter, opentelemetry_sdk::runtime::Tokio) - .with_resource(opentelemetry_sdk::Resource::new(vec![ - opentelemetry::KeyValue::new("service.name", "diffscope"), - ])) - .build(); - - opentelemetry::global::set_tracer_provider(tracer_provider.clone()); - - let otel_layer = - tracing_opentelemetry::layer().with_tracer(tracer_provider.tracer("diffscope")); - - let subscriber = tracing_subscriber::fmt::Subscriber::builder() - .with_env_filter(filter) - .finish() - .with(otel_layer); - - tracing::subscriber::set_global_default(subscriber) - .expect("Failed to set tracing subscriber"); - - Some(tracer_provider) + { + Ok(exporter) => { + let tracer_provider = opentelemetry_sdk::trace::TracerProvider::builder() + .with_batch_exporter(exporter, opentelemetry_sdk::runtime::Tokio) + .with_resource(opentelemetry_sdk::Resource::new(vec![ + opentelemetry::KeyValue::new("service.name", "diffscope"), + ])) + .build(); + + opentelemetry::global::set_tracer_provider(tracer_provider.clone()); + + let otel_layer = tracing_opentelemetry::layer() + .with_tracer(tracer_provider.tracer("diffscope")); + + let subscriber = tracing_subscriber::fmt::Subscriber::builder() + .with_env_filter(filter) + .finish() + .with(otel_layer); + + if let Err(e) = tracing::subscriber::set_global_default(subscriber) { + eprintln!("Warning: failed to set OTEL tracing subscriber: {}", e); + // Already initialized by another thread or test — not fatal + } + + Some(tracer_provider) + } + Err(e) => { + eprintln!( + "Warning: OTEL_EXPORTER_OTLP_ENDPOINT set but exporter failed to initialize: {}. Continuing without OpenTelemetry.", + e + ); + tracing_subscriber::fmt().with_env_filter(filter).init(); + None + } + } } else { tracing_subscriber::fmt().with_env_filter(filter).init(); None diff --git a/src/plugins/builtin/duplicate_filter.rs b/src/plugins/builtin/duplicate_filter.rs index e4a6550..b7aca1c 100644 --- a/src/plugins/builtin/duplicate_filter.rs +++ b/src/plugins/builtin/duplicate_filter.rs @@ -33,3 +33,102 @@ impl PostProcessor for DuplicateFilter { Ok(comments) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::core::comment::{Category, FixEffort, Severity}; + use std::path::PathBuf; + + fn make_comment(file: &str, line: usize, content: &str) -> Comment { + Comment { + id: format!("c-{}-{}", file, line), + file_path: PathBuf::from(file), + line_number: line, + content: content.to_string(), + rule_id: None, + severity: Severity::Warning, + category: Category::Style, + suggestion: None, + confidence: 0.8, + code_suggestion: None, + tags: vec![], + fix_effort: FixEffort::Low, + feedback: None, + } + } + + #[test] + fn test_duplicate_filter_id() { + let filter = DuplicateFilter::new(); + assert_eq!(filter.id(), "duplicate_filter"); + } + + #[tokio::test] + async fn test_removes_exact_duplicates() { + let filter = DuplicateFilter::new(); + let comments = vec![ + make_comment("a.rs", 10, "fix this"), + make_comment("a.rs", 10, "fix this"), + make_comment("a.rs", 10, "fix this"), + ]; + let result = filter.run(comments, "/repo").await.unwrap(); + assert_eq!(result.len(), 1); + } + + #[tokio::test] + async fn test_keeps_different_lines() { + let filter = DuplicateFilter::new(); + let comments = vec![ + make_comment("a.rs", 10, "fix this"), + make_comment("a.rs", 20, "fix this"), + ]; + let result = filter.run(comments, "/repo").await.unwrap(); + assert_eq!(result.len(), 2); + } + + #[tokio::test] + async fn test_keeps_different_content() { + let filter = DuplicateFilter::new(); + let comments = vec![ + make_comment("a.rs", 10, "fix this"), + make_comment("a.rs", 10, "fix that"), + ]; + let result = filter.run(comments, "/repo").await.unwrap(); + assert_eq!(result.len(), 2); + } + + #[tokio::test] + async fn test_keeps_different_files() { + let filter = DuplicateFilter::new(); + let comments = vec![ + make_comment("a.rs", 10, "fix this"), + make_comment("b.rs", 10, "fix this"), + ]; + let result = filter.run(comments, "/repo").await.unwrap(); + assert_eq!(result.len(), 2); + } + + #[tokio::test] + async fn test_empty_input() { + let filter = DuplicateFilter::new(); + let result = filter.run(vec![], "/repo").await.unwrap(); + assert!(result.is_empty()); + } + + #[tokio::test] + async fn test_preserves_order() { + let filter = DuplicateFilter::new(); + let comments = vec![ + make_comment("c.rs", 1, "third"), + make_comment("a.rs", 1, "first"), + make_comment("b.rs", 1, "second"), + make_comment("a.rs", 1, "first"), // duplicate + ]; + let result = filter.run(comments, "/repo").await.unwrap(); + assert_eq!(result.len(), 3); + assert_eq!(result[0].content, "third"); + assert_eq!(result[1].content, "first"); + assert_eq!(result[2].content, "second"); + } +} diff --git a/src/plugins/builtin/eslint.rs b/src/plugins/builtin/eslint.rs index b376884..5e37005 100644 --- a/src/plugins/builtin/eslint.rs +++ b/src/plugins/builtin/eslint.rs @@ -3,7 +3,6 @@ use crate::plugins::PreAnalyzer; use anyhow::Result; use async_trait::async_trait; use std::path::PathBuf; -use std::process::Command; pub struct EslintAnalyzer; @@ -13,6 +12,8 @@ impl EslintAnalyzer { } } +const JS_EXTENSIONS: &[&str] = &[".js", ".ts", ".jsx", ".tsx"]; + #[async_trait] impl PreAnalyzer for EslintAnalyzer { fn id(&self) -> &str { @@ -20,24 +21,26 @@ impl PreAnalyzer for EslintAnalyzer { } async fn run(&self, diff: &UnifiedDiff, repo_path: &str) -> Result> { - if !diff.file_path.to_string_lossy().ends_with(".js") - && !diff.file_path.to_string_lossy().ends_with(".ts") - && !diff.file_path.to_string_lossy().ends_with(".jsx") - && !diff.file_path.to_string_lossy().ends_with(".tsx") - { + let path_str = diff.file_path.to_string_lossy(); + if !JS_EXTENSIONS.iter().any(|ext| path_str.ends_with(ext)) { return Ok(Vec::new()); } let file_path = PathBuf::from(repo_path).join(&diff.file_path); + let file_arg = file_path.to_string_lossy().to_string(); - let output = Command::new("eslint") - .arg("--format=json") - .arg("--no-eslintrc") - .arg(file_path.to_string_lossy().as_ref()) - .output(); + let output = tokio::task::spawn_blocking(move || { + use std::process::Command; + Command::new("eslint") + .arg("--format=json") + .arg("--no-eslintrc") + .arg(&file_arg) + .output() + }) + .await; match output { - Ok(output) => { + Ok(Ok(output)) => { let stdout = String::from_utf8_lossy(&output.stdout); if !stdout.trim().is_empty() { Ok(vec![LLMContextChunk { @@ -50,7 +53,60 @@ impl PreAnalyzer for EslintAnalyzer { Ok(Vec::new()) } } - Err(_) => Ok(Vec::new()), + _ => Ok(Vec::new()), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn make_diff(file_path: &str) -> UnifiedDiff { + UnifiedDiff { + old_content: None, + new_content: None, + file_path: PathBuf::from(file_path), + is_new: false, + is_deleted: false, + is_binary: false, + hunks: vec![], + } + } + + #[test] + fn test_eslint_analyzer_id() { + let analyzer = EslintAnalyzer::new(); + assert_eq!(analyzer.id(), "eslint"); + } + + #[tokio::test] + async fn test_eslint_skips_non_js_files() { + let analyzer = EslintAnalyzer::new(); + for ext in &[".rs", ".py", ".go", ".java", ".rb", ".css", ".html"] { + let diff = make_diff(&format!("file{}", ext)); + let result = analyzer.run(&diff, "/tmp").await.unwrap(); + assert!(result.is_empty(), "Should skip {}", ext); + } + } + + #[tokio::test] + async fn test_eslint_accepts_js_extensions() { + let analyzer = EslintAnalyzer::new(); + // These should not be skipped (they'll fail because eslint isn't installed, + // but they won't be filtered by extension) + for ext in &[".js", ".ts", ".jsx", ".tsx"] { + let diff = make_diff(&format!("file{}", ext)); + let result = analyzer.run(&diff, "/nonexistent").await; + assert!(result.is_ok(), "Should accept {}", ext); } } + + #[tokio::test] + async fn test_eslint_handles_missing_binary() { + let analyzer = EslintAnalyzer::new(); + let diff = make_diff("test.js"); + let result = analyzer.run(&diff, "/nonexistent/repo").await; + assert!(result.is_ok()); + } } diff --git a/src/plugins/builtin/semgrep.rs b/src/plugins/builtin/semgrep.rs index bb605e9..8fef5c4 100644 --- a/src/plugins/builtin/semgrep.rs +++ b/src/plugins/builtin/semgrep.rs @@ -3,7 +3,9 @@ use crate::plugins::PreAnalyzer; use anyhow::Result; use async_trait::async_trait; use std::path::PathBuf; -use std::process::Command; + +/// Timeout for semgrep execution in seconds. +const SEMGREP_TIMEOUT_SECS: u64 = 30; pub struct SemgrepAnalyzer; @@ -21,16 +23,23 @@ impl PreAnalyzer for SemgrepAnalyzer { async fn run(&self, diff: &UnifiedDiff, repo_path: &str) -> Result> { let file_path = PathBuf::from(repo_path).join(&diff.file_path); + let file_arg = file_path.to_string_lossy().to_string(); - let output = Command::new("semgrep") - .arg("--config=auto") - .arg("--json") - .arg("--quiet") - .arg(file_path.to_string_lossy().as_ref()) - .output(); + let output = tokio::task::spawn_blocking(move || { + use std::process::Command; + Command::new("semgrep") + .arg("--config=auto") + .arg("--json") + .arg("--quiet") + .arg("--timeout") + .arg(SEMGREP_TIMEOUT_SECS.to_string()) + .arg(&file_arg) + .output() + }) + .await; match output { - Ok(output) => { + Ok(Ok(output)) => { let stdout = String::from_utf8_lossy(&output.stdout); if !stdout.trim().is_empty() { Ok(vec![LLMContextChunk { @@ -43,7 +52,54 @@ impl PreAnalyzer for SemgrepAnalyzer { Ok(Vec::new()) } } - Err(_) => Ok(Vec::new()), + _ => Ok(Vec::new()), + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn make_diff(file_path: &str) -> UnifiedDiff { + UnifiedDiff { + old_content: None, + new_content: None, + file_path: PathBuf::from(file_path), + is_new: false, + is_deleted: false, + is_binary: false, + hunks: vec![], } } + + #[test] + fn test_semgrep_analyzer_id() { + let analyzer = SemgrepAnalyzer::new(); + assert_eq!(analyzer.id(), "semgrep"); + } + + #[tokio::test] + async fn test_semgrep_handles_missing_binary() { + // When semgrep is not installed, should return empty vec (not error) + let analyzer = SemgrepAnalyzer::new(); + let diff = make_diff("nonexistent_file.py"); + let result = analyzer.run(&diff, "/nonexistent/repo").await; + assert!(result.is_ok()); + // Either empty (semgrep not found) or has output (semgrep found but file missing) + } + + #[tokio::test] + async fn test_semgrep_returns_context_chunks_with_correct_type() { + // Verify the context type and file path are set correctly when output exists + let chunk = LLMContextChunk { + file_path: PathBuf::from("test.py"), + content: "Semgrep analysis:\n{\"results\":[]}".to_string(), + context_type: ContextType::Documentation, + line_range: None, + }; + assert_eq!(chunk.context_type, ContextType::Documentation); + assert_eq!(chunk.file_path, PathBuf::from("test.py")); + assert!(chunk.content.starts_with("Semgrep analysis:")); + } } diff --git a/src/review/context_helpers.rs b/src/review/context_helpers.rs index 5b65e92..f9c8ebb 100644 --- a/src/review/context_helpers.rs +++ b/src/review/context_helpers.rs @@ -57,12 +57,34 @@ pub fn resolve_pattern_repositories( } fn is_git_source(source: &str) -> bool { - source.contains("://") || source.starts_with("git@") || source.ends_with(".git") + if source.starts_with("https://") || source.starts_with("git@") { + return true; + } + if source.starts_with("http://") && source.ends_with(".git") { + return true; + } + false +} + +/// Validate that a pattern repository source URL uses an allowed scheme. +/// Only `https://`, `git@`, and `http://` (with `.git` suffix) are permitted. +fn is_safe_git_url(source: &str) -> bool { + source.starts_with("https://") + || source.starts_with("git@") + || (source.starts_with("http://") && source.ends_with(".git")) } fn prepare_pattern_repository_checkout(source: &str) -> Option { use std::process::Command; + if !is_safe_git_url(source) { + warn!( + "Rejecting pattern repository '{}': only https:// and git@ URLs are allowed", + source + ); + return None; + } + let home_dir = dirs::home_dir()?; let cache_root = home_dir.join(".diffscope").join("pattern_repositories"); if std::fs::create_dir_all(&cache_root).is_err() { @@ -476,4 +498,66 @@ mod tests { assert_eq!(result.len(), 1); assert!(result[0].content.starts_with("Active review rules.")); } + + // === URL validation tests === + + #[test] + fn test_is_git_source_https() { + assert!(is_git_source("https://github.com/org/repo.git")); + assert!(is_git_source("https://github.com/org/repo")); + } + + #[test] + fn test_is_git_source_ssh() { + assert!(is_git_source("git@github.com:org/repo.git")); + } + + #[test] + fn test_is_git_source_http_with_git_suffix() { + assert!(is_git_source("http://example.com/repo.git")); + } + + #[test] + fn test_is_git_source_rejects_local_paths() { + assert!(!is_git_source("/tmp/evil")); + assert!(!is_git_source("../relative/path")); + assert!(!is_git_source("file:///etc/passwd")); + } + + #[test] + fn test_is_git_source_rejects_other_schemes() { + assert!(!is_git_source("ftp://example.com/repo.git")); + assert!(!is_git_source("ssh://example.com/repo")); + } + + #[test] + fn test_is_safe_git_url_allows_https() { + assert!(is_safe_git_url("https://github.com/org/repo")); + assert!(is_safe_git_url("https://gitlab.com/org/repo.git")); + } + + #[test] + fn test_is_safe_git_url_allows_ssh() { + assert!(is_safe_git_url("git@github.com:org/repo.git")); + } + + #[test] + fn test_is_safe_git_url_rejects_file_urls() { + assert!(!is_safe_git_url("file:///etc/passwd")); + assert!(!is_safe_git_url("/tmp/evil")); + assert!(!is_safe_git_url("../traversal")); + } + + #[test] + fn test_is_safe_git_url_rejects_arbitrary_schemes() { + assert!(!is_safe_git_url("ftp://example.com/repo")); + assert!(!is_safe_git_url("ssh://example.com/repo")); + assert!(!is_safe_git_url("gopher://example.com/repo")); + } + + #[test] + fn test_is_safe_git_url_rejects_http_without_git_suffix() { + // Plain http without .git suffix is not safe enough + assert!(!is_safe_git_url("http://example.com/repo")); + } } diff --git a/src/server/storage_json.rs b/src/server/storage_json.rs index ec2b4a8..52e4a41 100644 --- a/src/server/storage_json.rs +++ b/src/server/storage_json.rs @@ -366,3 +366,1009 @@ impl StorageBackend for JsonStorageBackend { Ok(removed) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::core::comment::{Category, Comment, FixEffort, ReviewSummary, Severity}; + use crate::server::state::ReviewEventBuilder; + use crate::server::storage::EventFilters; + use std::collections::HashMap; + use std::path::PathBuf; + + /// Create a minimal ReviewSession for testing. + fn make_session(id: &str, started_at: i64, status: ReviewStatus) -> ReviewSession { + ReviewSession { + id: id.to_string(), + status, + diff_source: "head".to_string(), + started_at, + completed_at: None, + comments: vec![], + summary: None, + files_reviewed: 0, + error: None, + pr_summary_text: None, + diff_content: Some("diff content".to_string()), + event: None, + progress: None, + } + } + + /// Create a ReviewSession with an attached ReviewEvent. + fn make_session_with_event( + id: &str, + started_at: i64, + status: ReviewStatus, + event_type: &str, + model: &str, + diff_source: &str, + duration_ms: u64, + ) -> ReviewSession { + let event = ReviewEventBuilder::new(id, event_type, diff_source, model) + .duration_ms(duration_ms) + .build(); + let mut session = make_session(id, started_at, status); + session.diff_source = diff_source.to_string(); + session.event = Some(event); + session + } + + /// Create a Comment for testing. + fn make_comment(id: &str, file: &str) -> Comment { + Comment { + id: id.to_string(), + file_path: PathBuf::from(file), + line_number: 1, + content: "test comment".to_string(), + rule_id: None, + severity: Severity::Warning, + category: Category::Bug, + suggestion: None, + confidence: 0.8, + code_suggestion: None, + tags: vec![], + fix_effort: FixEffort::Low, + feedback: None, + } + } + + /// Helper to get the current Unix timestamp. + fn now_ts() -> i64 { + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() as i64 + } + + // --------------------------------------------------------------- + // 1. save_review + get_review round-trip + // --------------------------------------------------------------- + + #[tokio::test] + async fn test_save_and_get_review_roundtrip() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let session = make_session("r1", now_ts(), ReviewStatus::Complete); + backend.save_review(&session).await.unwrap(); + + let loaded = backend.get_review("r1").await.unwrap(); + assert!(loaded.is_some()); + let loaded = loaded.unwrap(); + assert_eq!(loaded.id, "r1"); + assert_eq!(loaded.status, ReviewStatus::Complete); + assert_eq!(loaded.diff_source, "head"); + } + + #[tokio::test] + async fn test_get_review_nonexistent_returns_none() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let result = backend.get_review("does-not-exist").await.unwrap(); + assert!(result.is_none()); + } + + #[tokio::test] + async fn test_save_review_with_comments_and_summary() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let mut session = make_session("r-full", now_ts(), ReviewStatus::Complete); + session.comments = vec![ + make_comment("c1", "src/main.rs"), + make_comment("c2", "src/lib.rs"), + ]; + session.summary = Some(ReviewSummary { + total_comments: 2, + by_severity: HashMap::from([("Warning".to_string(), 2)]), + by_category: HashMap::from([("Bug".to_string(), 2)]), + critical_issues: 0, + files_reviewed: 2, + overall_score: 8.0, + recommendations: vec!["Fix bugs".to_string()], + }); + + backend.save_review(&session).await.unwrap(); + let loaded = backend.get_review("r-full").await.unwrap().unwrap(); + assert_eq!(loaded.comments.len(), 2); + assert!(loaded.summary.is_some()); + assert_eq!(loaded.summary.unwrap().overall_score, 8.0); + } + + // --------------------------------------------------------------- + // 2. list_reviews — ordering, limit, offset + // --------------------------------------------------------------- + + #[tokio::test] + async fn test_list_reviews_ordering_newest_first() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let base = now_ts(); + for i in 0..5 { + let session = make_session(&format!("r{}", i), base + i as i64, ReviewStatus::Complete); + backend.save_review(&session).await.unwrap(); + } + + let list = backend.list_reviews(10, 0).await.unwrap(); + assert_eq!(list.len(), 5); + // Should be sorted newest first (descending started_at) + assert_eq!(list[0].id, "r4"); + assert_eq!(list[4].id, "r0"); + } + + #[tokio::test] + async fn test_list_reviews_with_limit() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let base = now_ts(); + for i in 0..5 { + let session = make_session(&format!("r{}", i), base + i as i64, ReviewStatus::Complete); + backend.save_review(&session).await.unwrap(); + } + + let list = backend.list_reviews(3, 0).await.unwrap(); + assert_eq!(list.len(), 3); + assert_eq!(list[0].id, "r4"); + assert_eq!(list[2].id, "r2"); + } + + #[tokio::test] + async fn test_list_reviews_with_offset() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let base = now_ts(); + for i in 0..5 { + let session = make_session(&format!("r{}", i), base + i as i64, ReviewStatus::Complete); + backend.save_review(&session).await.unwrap(); + } + + let list = backend.list_reviews(10, 2).await.unwrap(); + assert_eq!(list.len(), 3); + // Skipped r4, r3 (offset=2), so first result is r2 + assert_eq!(list[0].id, "r2"); + assert_eq!(list[2].id, "r0"); + } + + #[tokio::test] + async fn test_list_reviews_empty() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let list = backend.list_reviews(10, 0).await.unwrap(); + assert!(list.is_empty()); + } + + // --------------------------------------------------------------- + // 3. delete_review + // --------------------------------------------------------------- + + #[tokio::test] + async fn test_delete_review() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let session = make_session("r-del", now_ts(), ReviewStatus::Complete); + backend.save_review(&session).await.unwrap(); + assert!(backend.get_review("r-del").await.unwrap().is_some()); + + backend.delete_review("r-del").await.unwrap(); + assert!(backend.get_review("r-del").await.unwrap().is_none()); + } + + #[tokio::test] + async fn test_delete_nonexistent_review_is_noop() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + // Should not panic or error + backend.delete_review("ghost").await.unwrap(); + } + + #[tokio::test] + async fn test_delete_does_not_affect_others() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let base = now_ts(); + backend + .save_review(&make_session("keep", base, ReviewStatus::Complete)) + .await + .unwrap(); + backend + .save_review(&make_session("remove", base + 1, ReviewStatus::Complete)) + .await + .unwrap(); + + backend.delete_review("remove").await.unwrap(); + assert!(backend.get_review("keep").await.unwrap().is_some()); + assert!(backend.get_review("remove").await.unwrap().is_none()); + } + + // --------------------------------------------------------------- + // 4. prune + // --------------------------------------------------------------- + + #[tokio::test] + async fn test_prune_removes_old_reviews() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + // Old review: 2 hours ago + backend + .save_review(&make_session("old", now - 7200, ReviewStatus::Complete)) + .await + .unwrap(); + // Recent review: just now + backend + .save_review(&make_session("new", now, ReviewStatus::Complete)) + .await + .unwrap(); + + // Prune anything older than 1 hour, keep at most 100 + let removed = backend.prune(3600, 100).await.unwrap(); + assert_eq!(removed, 1); + assert!(backend.get_review("old").await.unwrap().is_none()); + assert!(backend.get_review("new").await.unwrap().is_some()); + } + + #[tokio::test] + async fn test_prune_enforces_max_count() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + // All recent, but exceed max_count of 2 + for i in 0..5 { + backend + .save_review(&make_session( + &format!("r{}", i), + now + i as i64, + ReviewStatus::Complete, + )) + .await + .unwrap(); + } + + // max_age very large (so nothing expires by age), max_count = 2 + let removed = backend.prune(999999, 2).await.unwrap(); + assert_eq!(removed, 3); + let remaining = backend.list_reviews(100, 0).await.unwrap(); + assert_eq!(remaining.len(), 2); + } + + #[tokio::test] + async fn test_prune_preserves_running_reviews_when_enforcing_count() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + // One running review (oldest) + backend + .save_review(&make_session("running", now, ReviewStatus::Running)) + .await + .unwrap(); + // Two completed reviews (newer) + backend + .save_review(&make_session("done1", now + 1, ReviewStatus::Complete)) + .await + .unwrap(); + backend + .save_review(&make_session("done2", now + 2, ReviewStatus::Complete)) + .await + .unwrap(); + + // max_count=1 -- prune should remove completed, but Running is not + // eligible for the count-based pass (only Complete/Failed are pruned) + let removed = backend.prune(999999, 1).await.unwrap(); + // 2 completed reviews removed to bring total towards max_count + assert_eq!(removed, 2); + // Running review should still be present + assert!(backend.get_review("running").await.unwrap().is_some()); + } + + #[tokio::test] + async fn test_prune_empty_storage() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let removed = backend.prune(3600, 100).await.unwrap(); + assert_eq!(removed, 0); + } + + // --------------------------------------------------------------- + // 5. save_event + list_events + // --------------------------------------------------------------- + + #[tokio::test] + async fn test_list_events_returns_events_from_sessions() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + let s1 = make_session_with_event( + "r1", + now, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + 1000, + ); + let s2 = make_session_with_event( + "r2", + now + 1, + ReviewStatus::Failed, + "review.failed", + "claude-sonnet-4.6", + "staged", + 2000, + ); + backend.save_review(&s1).await.unwrap(); + backend.save_review(&s2).await.unwrap(); + + let events = backend.list_events(&EventFilters::default()).await.unwrap(); + assert_eq!(events.len(), 2); + } + + #[tokio::test] + async fn test_list_events_filter_by_source() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + backend + .save_review(&make_session_with_event( + "r1", + now, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + 1000, + )) + .await + .unwrap(); + backend + .save_review(&make_session_with_event( + "r2", + now + 1, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "staged", + 1000, + )) + .await + .unwrap(); + + let filters = EventFilters { + source: Some("head".to_string()), + ..Default::default() + }; + let events = backend.list_events(&filters).await.unwrap(); + assert_eq!(events.len(), 1); + assert_eq!(events[0].diff_source, "head"); + } + + #[tokio::test] + async fn test_list_events_filter_by_model() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + backend + .save_review(&make_session_with_event( + "r1", + now, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + 1000, + )) + .await + .unwrap(); + backend + .save_review(&make_session_with_event( + "r2", + now + 1, + ReviewStatus::Complete, + "review.completed", + "claude-sonnet-4.6", + "head", + 2000, + )) + .await + .unwrap(); + + let filters = EventFilters { + model: Some("claude-sonnet-4.6".to_string()), + ..Default::default() + }; + let events = backend.list_events(&filters).await.unwrap(); + assert_eq!(events.len(), 1); + assert_eq!(events[0].model, "claude-sonnet-4.6"); + } + + #[tokio::test] + async fn test_list_events_filter_by_status() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + backend + .save_review(&make_session_with_event( + "r1", + now, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + 1000, + )) + .await + .unwrap(); + backend + .save_review(&make_session_with_event( + "r2", + now + 1, + ReviewStatus::Failed, + "review.failed", + "gpt-4o", + "head", + 500, + )) + .await + .unwrap(); + + let filters = EventFilters { + status: Some("failed".to_string()), + ..Default::default() + }; + let events = backend.list_events(&filters).await.unwrap(); + assert_eq!(events.len(), 1); + assert_eq!(events[0].event_type, "review.failed"); + } + + #[tokio::test] + async fn test_list_events_with_limit_and_offset() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + for i in 0..5 { + backend + .save_review(&make_session_with_event( + &format!("r{}", i), + now + i as i64, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + (i as u64 + 1) * 100, + )) + .await + .unwrap(); + } + + let filters = EventFilters { + limit: Some(2), + offset: Some(1), + ..Default::default() + }; + let events = backend.list_events(&filters).await.unwrap(); + assert_eq!(events.len(), 2); + } + + #[tokio::test] + async fn test_list_events_empty() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let events = backend.list_events(&EventFilters::default()).await.unwrap(); + assert!(events.is_empty()); + } + + #[tokio::test] + async fn test_save_event_is_noop() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + // save_event for JSON backend is a no-op (events are embedded in sessions) + let event = ReviewEventBuilder::new("r1", "review.completed", "head", "gpt-4o").build(); + let result = backend.save_event(&event).await; + assert!(result.is_ok()); + + // No events should be listed because no session holds this event + let events = backend.list_events(&EventFilters::default()).await.unwrap(); + assert!(events.is_empty()); + } + + // --------------------------------------------------------------- + // 6. get_event_stats + // --------------------------------------------------------------- + + #[tokio::test] + async fn test_get_event_stats_basic_aggregation() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + + // Two completed reviews with different models + let mut s1 = make_session_with_event( + "r1", + now, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + 1000, + ); + if let Some(ref mut e) = s1.event { + e.tokens_total = Some(500); + e.overall_score = Some(8.5); + e.comments_by_severity.insert("Warning".to_string(), 2); + } + + let mut s2 = make_session_with_event( + "r2", + now + 1, + ReviewStatus::Complete, + "review.completed", + "claude-sonnet-4.6", + "staged", + 3000, + ); + if let Some(ref mut e) = s2.event { + e.tokens_total = Some(1000); + e.overall_score = Some(7.0); + e.comments_by_category.insert("Bug".to_string(), 3); + } + + // One failed review + let mut s3 = make_session_with_event( + "r3", + now + 2, + ReviewStatus::Failed, + "review.failed", + "gpt-4o", + "head", + 500, + ); + if let Some(ref mut e) = s3.event { + e.tokens_total = Some(100); + } + + backend.save_review(&s1).await.unwrap(); + backend.save_review(&s2).await.unwrap(); + backend.save_review(&s3).await.unwrap(); + + let stats = backend + .get_event_stats(&EventFilters::default()) + .await + .unwrap(); + + assert_eq!(stats.total_reviews, 3); + assert_eq!(stats.completed_count, 2); + assert_eq!(stats.failed_count, 1); + assert_eq!(stats.total_tokens, 1600); // 500 + 1000 + 100 + assert!(stats.avg_duration_ms > 0.0); + assert!(stats.avg_score.is_some()); + assert!(stats.error_rate > 0.0); + + // Verify by_source has both "head" and "staged" + assert_eq!(stats.by_source.len(), 2); + + // Verify severity_totals and category_totals + assert_eq!(*stats.severity_totals.get("Warning").unwrap_or(&0), 2); + assert_eq!(*stats.category_totals.get("Bug").unwrap_or(&0), 3); + } + + #[tokio::test] + async fn test_get_event_stats_empty() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let stats = backend + .get_event_stats(&EventFilters::default()) + .await + .unwrap(); + + assert_eq!(stats.total_reviews, 0); + assert_eq!(stats.completed_count, 0); + assert_eq!(stats.failed_count, 0); + assert_eq!(stats.total_tokens, 0); + assert_eq!(stats.avg_duration_ms, 0.0); + assert!(stats.avg_score.is_none()); + assert_eq!(stats.error_rate, 0.0); + assert_eq!(stats.p50_latency_ms, 0); + } + + #[tokio::test] + async fn test_get_event_stats_latency_percentiles() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + // Create 10 reviews with increasing durations + for i in 0..10 { + backend + .save_review(&make_session_with_event( + &format!("r{}", i), + now + i as i64, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + (i as u64 + 1) * 100, // 100, 200, ..., 1000 + )) + .await + .unwrap(); + } + + let stats = backend + .get_event_stats(&EventFilters::default()) + .await + .unwrap(); + + assert!(stats.p50_latency_ms > 0); + assert!(stats.p95_latency_ms >= stats.p50_latency_ms); + assert!(stats.p99_latency_ms >= stats.p95_latency_ms); + } + + #[tokio::test] + async fn test_get_event_stats_by_model() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + backend + .save_review(&make_session_with_event( + "r1", + now, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + 1000, + )) + .await + .unwrap(); + backend + .save_review(&make_session_with_event( + "r2", + now + 1, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + 2000, + )) + .await + .unwrap(); + backend + .save_review(&make_session_with_event( + "r3", + now + 2, + ReviewStatus::Complete, + "review.completed", + "claude-sonnet-4.6", + "head", + 500, + )) + .await + .unwrap(); + + let stats = backend + .get_event_stats(&EventFilters::default()) + .await + .unwrap(); + + assert_eq!(stats.by_model.len(), 2); + let gpt_stats = stats.by_model.iter().find(|m| m.model == "gpt-4o").unwrap(); + assert_eq!(gpt_stats.count, 2); + assert_eq!(gpt_stats.avg_duration_ms, 1500.0); // (1000+2000)/2 + } + + #[tokio::test] + async fn test_get_event_stats_by_repo() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + let mut s1 = make_session_with_event( + "r1", + now, + ReviewStatus::Complete, + "review.completed", + "gpt-4o", + "head", + 1000, + ); + if let Some(ref mut e) = s1.event { + e.github_repo = Some("owner/repo".to_string()); + e.overall_score = Some(9.0); + } + backend.save_review(&s1).await.unwrap(); + + let stats = backend + .get_event_stats(&EventFilters::default()) + .await + .unwrap(); + + assert_eq!(stats.by_repo.len(), 1); + assert_eq!(stats.by_repo[0].repo, "owner/repo"); + assert_eq!(stats.by_repo[0].count, 1); + assert_eq!(stats.by_repo[0].avg_score, Some(9.0_f64)); + } + + // --------------------------------------------------------------- + // 7. is_empty (via internal state check) + // --------------------------------------------------------------- + + #[tokio::test] + async fn test_empty_storage_has_no_reviews() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let list = backend.list_reviews(100, 0).await.unwrap(); + assert!(list.is_empty()); + } + + #[tokio::test] + async fn test_non_empty_after_save() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + backend + .save_review(&make_session("r1", now_ts(), ReviewStatus::Pending)) + .await + .unwrap(); + let list = backend.list_reviews(100, 0).await.unwrap(); + assert!(!list.is_empty()); + } + + // --------------------------------------------------------------- + // 8. Edge cases + // --------------------------------------------------------------- + + #[tokio::test] + async fn test_duplicate_id_overwrites() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let mut session = make_session("dup", now_ts(), ReviewStatus::Pending); + backend.save_review(&session).await.unwrap(); + + // Save again with same ID but different status + session.status = ReviewStatus::Complete; + session.files_reviewed = 5; + backend.save_review(&session).await.unwrap(); + + let loaded = backend.get_review("dup").await.unwrap().unwrap(); + assert_eq!(loaded.status, ReviewStatus::Complete); + assert_eq!(loaded.files_reviewed, 5); + + // Should still be just one review + let list = backend.list_reviews(100, 0).await.unwrap(); + assert_eq!(list.len(), 1); + } + + #[tokio::test] + async fn test_persistence_across_instances() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("reviews.json"); + + // First instance: save a review + { + let backend = JsonStorageBackend::new(&path); + backend + .save_review(&make_session("persist", now_ts(), ReviewStatus::Complete)) + .await + .unwrap(); + } + + // Second instance: load from the same file + { + let backend = JsonStorageBackend::new(&path); + let loaded = backend.get_review("persist").await.unwrap(); + assert!(loaded.is_some()); + assert_eq!(loaded.unwrap().id, "persist"); + } + } + + #[tokio::test] + async fn test_diff_content_stripped_on_flush() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("reviews.json"); + + // Save a review that has diff_content set + { + let backend = JsonStorageBackend::new(&path); + let mut session = make_session("strip", now_ts(), ReviewStatus::Complete); + session.diff_content = Some("big diff content here".to_string()); + backend.save_review(&session).await.unwrap(); + + // In-memory version still has diff_content + let in_mem = backend.get_review("strip").await.unwrap().unwrap(); + assert!(in_mem.diff_content.is_some()); + } + + // Reload from disk: diff_content should be None (stripped during flush) + { + let backend = JsonStorageBackend::new(&path); + let loaded = backend.get_review("strip").await.unwrap().unwrap(); + assert!(loaded.diff_content.is_none()); + } + } + + #[tokio::test] + async fn test_update_comment_feedback() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let mut session = make_session("fb", now_ts(), ReviewStatus::Complete); + session.comments = vec![make_comment("c1", "src/main.rs")]; + backend.save_review(&session).await.unwrap(); + + backend + .update_comment_feedback("fb", "c1", "helpful") + .await + .unwrap(); + + let loaded = backend.get_review("fb").await.unwrap().unwrap(); + assert_eq!(loaded.comments[0].feedback.as_deref(), Some("helpful")); + } + + #[tokio::test] + async fn test_update_comment_feedback_nonexistent_review() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + // Should not error, just no-op + let result = backend + .update_comment_feedback("ghost", "c1", "helpful") + .await; + assert!(result.is_ok()); + } + + #[tokio::test] + async fn test_update_comment_feedback_nonexistent_comment() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let session = make_session("fb2", now_ts(), ReviewStatus::Complete); + backend.save_review(&session).await.unwrap(); + + // Review exists but comment does not -- should not error + let result = backend + .update_comment_feedback("fb2", "nonexistent-comment", "helpful") + .await; + assert!(result.is_ok()); + } + + #[tokio::test] + async fn test_concurrent_writes() { + let dir = tempfile::tempdir().unwrap(); + let backend = + std::sync::Arc::new(JsonStorageBackend::new(&dir.path().join("reviews.json"))); + + let base = now_ts(); + let mut handles = vec![]; + + for i in 0..20 { + let b = backend.clone(); + let handle = tokio::spawn(async move { + let session = make_session( + &format!("conc-{}", i), + base + i as i64, + ReviewStatus::Complete, + ); + b.save_review(&session).await.unwrap(); + }); + handles.push(handle); + } + + for handle in handles { + handle.await.unwrap(); + } + + let list = backend.list_reviews(100, 0).await.unwrap(); + assert_eq!(list.len(), 20); + } + + #[tokio::test] + async fn test_load_from_nonexistent_file() { + let dir = tempfile::tempdir().unwrap(); + // Point to a file that doesn't exist yet + let backend = JsonStorageBackend::new(&dir.path().join("doesnt-exist.json")); + + let list = backend.list_reviews(100, 0).await.unwrap(); + assert!(list.is_empty()); + } + + #[tokio::test] + async fn test_load_from_corrupt_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("reviews.json"); + + // Write garbage to the file + std::fs::write(&path, "{ not valid json !!!").unwrap(); + + // Should not panic; falls back to empty map + let backend = JsonStorageBackend::new(&path); + let list = backend.list_reviews(100, 0).await.unwrap(); + assert!(list.is_empty()); + } + + #[tokio::test] + async fn test_list_events_case_insensitive_filters() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + let now = now_ts(); + backend + .save_review(&make_session_with_event( + "r1", + now, + ReviewStatus::Complete, + "review.completed", + "GPT-4o", + "HEAD", + 1000, + )) + .await + .unwrap(); + + // Filters use lowercase but source/model are uppercase -- should match + let filters = EventFilters { + source: Some("head".to_string()), + model: Some("gpt-4o".to_string()), + ..Default::default() + }; + let events = backend.list_events(&filters).await.unwrap(); + assert_eq!(events.len(), 1); + } + + #[tokio::test] + async fn test_sessions_without_events_excluded_from_list_events() { + let dir = tempfile::tempdir().unwrap(); + let backend = JsonStorageBackend::new(&dir.path().join("reviews.json")); + + // Session without an event + backend + .save_review(&make_session("no-event", now_ts(), ReviewStatus::Pending)) + .await + .unwrap(); + + let events = backend.list_events(&EventFilters::default()).await.unwrap(); + assert!(events.is_empty()); + } +} From fcfe9778164c6f0f4e0258409041c6ff1718f6f9 Mon Sep 17 00:00:00 2001 From: Jonathan Haas Date: Wed, 11 Mar 2026 13:29:30 -0700 Subject: [PATCH 2/8] =?UTF-8?q?fix:=204=20TDD-discovered=20bugs=20?= =?UTF-8?q?=E2=80=94=20comment=20detection,=20function=20boundaries,=20ded?= =?UTF-8?q?up=20severity,=20empty=20LLM=20responses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 1: `is_comment_line` missed bare `#` (shell/Makefile/Python comments) - Changed `#!` → `#![` in non-comment prefixes (Rust inner attrs always have `[`) - Any `#` line not matching a known code prefix is now treated as a comment Bug 2: `find_function_end` ignored single-quoted strings - `'{'` in JS/Python strings was counted as a real brace, breaking function boundary detection. Now tracks both `"` and `'` string delimiters. Bug 3: `deduplicate_comments` could drop higher-severity comments - `dedup_by` keeps the first element; sort didn't include severity. Now sorts by severity (highest first) within same file/line/content group. Bug 4: OpenAI/Anthropic silently returned empty string for empty responses - Empty `choices`/`content` arrays now return errors instead of succeeding with empty content, preventing silent failures in the review pipeline. Co-Authored-By: Claude Opus 4.6 --- src/adapters/anthropic.rs | 27 +++++++++--------- src/adapters/openai.rs | 10 +++---- src/core/comment.rs | 54 ++++++++++++++++++++++++++++++++++++ src/core/function_chunker.rs | 33 ++++++++++++++++++---- src/review/triage.rs | 51 ++++++++++++++++++++++++++++------ 5 files changed, 142 insertions(+), 33 deletions(-) diff --git a/src/adapters/anthropic.rs b/src/adapters/anthropic.rs index a74c3dc..1e98772 100644 --- a/src/adapters/anthropic.rs +++ b/src/adapters/anthropic.rs @@ -121,14 +121,13 @@ impl LLMAdapter for AnthropicAdapter { .content .first() .map(|c| { - // Verify it's a text content type if c.content_type == "text" { - c.text.clone() + Ok(c.text.clone()) } else { - format!("Unsupported content type: {}", c.content_type) + Err(anyhow::anyhow!("Unsupported content type: {}", c.content_type)) } }) - .unwrap_or_default(); + .ok_or_else(|| anyhow::anyhow!("Anthropic returned empty content array — no content generated"))??; Ok(LLMResponse { content, @@ -292,7 +291,7 @@ mod tests { } #[tokio::test] - async fn test_unsupported_content_type() { + async fn test_unsupported_content_type_returns_error() { let mut server = mockito::Server::new_async().await; let _mock = server .mock("POST", "/messages") @@ -311,17 +310,17 @@ mod tests { let adapter = AnthropicAdapter::new(test_config(&server.url())).unwrap(); let result = adapter.complete(test_request()).await; - assert!(result.is_ok()); - let response = result.unwrap(); + assert!(result.is_err(), "Unsupported content type should return an error"); + let err = result.unwrap_err().to_string(); assert!( - response.content.contains("Unsupported content type"), - "Expected 'Unsupported content type' in response, got: {}", - response.content + err.contains("Unsupported content type"), + "Error should mention unsupported type, got: {}", + err ); } #[tokio::test] - async fn test_empty_content_array() { + async fn test_empty_content_array_returns_error() { let mut server = mockito::Server::new_async().await; let _mock = server .mock("POST", "/messages") @@ -340,9 +339,9 @@ mod tests { let adapter = AnthropicAdapter::new(test_config(&server.url())).unwrap(); let result = adapter.complete(test_request()).await; - assert!(result.is_ok()); - let response = result.unwrap(); - assert_eq!(response.content, ""); + assert!(result.is_err(), "Empty content array should return an error, not silently succeed"); + let err = result.unwrap_err().to_string(); + assert!(err.contains("empty content"), "Error should mention empty content: {}", err); } #[test] diff --git a/src/adapters/openai.rs b/src/adapters/openai.rs index 927c593..990b387 100644 --- a/src/adapters/openai.rs +++ b/src/adapters/openai.rs @@ -196,7 +196,7 @@ impl OpenAIAdapter { .choices .first() .map(|c| c.message.content.clone()) - .unwrap_or_default(); + .ok_or_else(|| anyhow::anyhow!("OpenAI returned empty choices array — no content generated"))?; Ok(LLMResponse { content, @@ -416,7 +416,7 @@ mod tests { } #[tokio::test] - async fn test_empty_choices_returns_empty_content() { + async fn test_empty_choices_returns_error() { let mut server = mockito::Server::new_async().await; let _mock = server .mock("POST", "/chat/completions") @@ -435,9 +435,9 @@ mod tests { let adapter = OpenAIAdapter::new(test_config(&server.url())).unwrap(); let result = adapter.complete(test_request()).await; - assert!(result.is_ok()); - let response = result.unwrap(); - assert_eq!(response.content, ""); + assert!(result.is_err(), "Empty choices should return an error, not silently succeed"); + let err = result.unwrap_err().to_string(); + assert!(err.contains("empty choices"), "Error should mention empty choices: {}", err); } #[test] diff --git a/src/core/comment.rs b/src/core/comment.rs index 74ef90c..35431f5 100644 --- a/src/core/comment.rs +++ b/src/core/comment.rs @@ -449,12 +449,23 @@ impl CommentSynthesizer { } fn deduplicate_comments(comments: &mut Vec) { + let severity_rank = |s: &Severity| match s { + Severity::Error => 0, + Severity::Warning => 1, + Severity::Info => 2, + Severity::Suggestion => 3, + }; + + // Sort by file/line/content, then by severity (highest first) comments.sort_by(|a, b| { a.file_path .cmp(&b.file_path) .then(a.line_number.cmp(&b.line_number)) .then(a.content.cmp(&b.content)) + .then(severity_rank(&a.severity).cmp(&severity_rank(&b.severity))) }); + // dedup_by keeps the first element (b) of consecutive duplicates, + // which is the highest severity due to our sort order comments.dedup_by(|a, b| { a.file_path == b.file_path && a.line_number == b.line_number && a.content == b.content }); @@ -548,6 +559,49 @@ pub struct RawComment { mod tests { use super::*; + #[test] + fn test_deduplicate_preserves_highest_severity() { + // BUG: dedup_by keeps the first element of a consecutive pair, + // but doesn't consider severity. If Warning comes before Error + // (due to stable sort on file/line/content), the Error is dropped. + let raw_comments = vec![ + RawComment { + file_path: PathBuf::from("src/lib.rs"), + line_number: 10, + content: "Missing null check".to_string(), + rule_id: None, + suggestion: None, + severity: Some(Severity::Warning), + category: Some(Category::Bug), + confidence: Some(0.8), + fix_effort: None, + tags: Vec::new(), + code_suggestion: None, + }, + RawComment { + file_path: PathBuf::from("src/lib.rs"), + line_number: 10, + content: "Missing null check".to_string(), + rule_id: None, + suggestion: None, + severity: Some(Severity::Error), + category: Some(Category::Bug), + confidence: Some(0.9), + fix_effort: None, + tags: Vec::new(), + code_suggestion: None, + }, + ]; + + let comments = CommentSynthesizer::synthesize(raw_comments).unwrap(); + assert_eq!(comments.len(), 1, "Should deduplicate to one comment"); + assert_eq!( + comments[0].severity, + Severity::Error, + "Should keep the higher severity (Error), not the lower (Warning)" + ); + } + #[test] fn test_severity_display() { assert_eq!(Severity::Error.to_string(), "Error"); diff --git a/src/core/function_chunker.rs b/src/core/function_chunker.rs index 7f72a86..e09a364 100644 --- a/src/core/function_chunker.rs +++ b/src/core/function_chunker.rs @@ -255,16 +255,19 @@ fn find_function_end(lines: &[&str], start: usize, language: &str) -> usize { let mut depth = 0i32; let mut found_open = false; for (i, line) in lines.iter().enumerate().skip(start) { - let mut in_string = false; + let mut in_double_quote = false; + let mut in_single_quote = false; let mut escaped = false; for ch in line.chars() { if escaped { escaped = false; - } else if in_string && ch == '\\' { + } else if (in_double_quote || in_single_quote) && ch == '\\' { escaped = true; - } else if ch == '"' { - in_string = !in_string; - } else if !in_string { + } else if ch == '"' && !in_single_quote { + in_double_quote = !in_double_quote; + } else if ch == '\'' && !in_double_quote { + in_single_quote = !in_single_quote; + } else if !in_double_quote && !in_single_quote { if ch == '{' { depth += 1; found_open = true; @@ -911,6 +914,26 @@ fn next_func() { assert_eq!(result, Some(2)); } + #[test] + fn test_find_function_end_single_quoted_string_with_brace() { + // BUG: single-quoted strings aren't tracked, so `'{'` is counted as a real brace + let lines: Vec<&str> = vec![ + "function foo() {", // line 0: depth = 1 + " let s = '{';", // line 1: depth should stay 1, but goes to 2 + " console.log(s);", // line 2 + "}", // line 3: depth goes to 1 (not 0!), so foo "never ends" + "", // line 4 + "function bar() {", // line 5: depth goes to 2 + " return 1;", // line 6 + "}", // line 7: depth goes to 1, and foo is "found" here (wrong!) + ]; + let end = find_function_end(&lines, 0, "js"); + assert_eq!( + end, 3, + "foo() should end at line 3, not bleed into bar() due to untracked single-quoted brace" + ); + } + #[test] fn test_find_function_end_escaped_backslash_in_string() { // A string containing "\\" (escaped backslash) on the same line as braces. diff --git a/src/review/triage.rs b/src/review/triage.rs index c6004c3..1043656 100644 --- a/src/review/triage.rs +++ b/src/review/triage.rs @@ -45,7 +45,7 @@ const COMMENT_PREFIXES: &[&str] = &["//", "# ", "/*", "*/", "* ", "--", "