diff --git a/.github/actions/cora-review/action.yml b/.github/actions/cora-review/action.yml index c6590f3..e7b91aa 100644 --- a/.github/actions/cora-review/action.yml +++ b/.github/actions/cora-review/action.yml @@ -124,7 +124,9 @@ runs: INPUT_BASE: ${{ inputs.base-branch }} INPUT_SEVERITY: ${{ inputs.severity }} run: | - cora review \ + # Set higher diff limit for CI (default 50K is too small for large PRs) + printf 'hook:\n max_diff_size: 200000\n' > .cora-ci.yaml + CORA_CONFIG=".cora-ci.yaml" cora review \ --base "$INPUT_BASE" \ --format sarif \ --severity "$INPUT_SEVERITY" \ diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 36c0f4e..2d309f5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -47,6 +47,15 @@ jobs: - uses: Swatinem/rust-cache@v2 - run: cargo clippy --all-targets -- -D warnings + audit: + name: Security Audit + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: rustsec/audit-check@858dc40f52ca2b8570b7a997c1c4e35c6fc9a432 + with: + token: ${{ secrets.GITHUB_TOKEN }} + test: name: Test runs-on: ubuntu-latest diff --git a/Cargo.lock b/Cargo.lock index 3b58d54..337ae96 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -11,6 +11,15 @@ dependencies = [ "memchr", ] +[[package]] +name = "android_system_properties" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "819e7219dbd41043ac279b19830f2efc897156490d7fd6ea916720117ee66311" +dependencies = [ + "libc", +] + [[package]] name = "anstream" version = "1.0.0" @@ -162,6 +171,20 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "613afe47fcd5fac7ccf1db93babcb082c5994d996f20b8b159f2ad1658eb5724" +[[package]] +name = "chrono" +version = "0.4.44" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c673075a2e0e5f4a1dde27ce9dee1ea4558c7ffe648f576438a20ca1d2acc4b0" +dependencies = [ + "iana-time-zone", + "js-sys", + "num-traits", + "serde", + "wasm-bindgen", + "windows-link", +] + [[package]] name = "clap" version = "4.6.1" @@ -244,6 +267,7 @@ version = "0.1.8" dependencies = [ "anyhow", "assert_cmd", + "chrono", "clap", "clap_complete", "colored", @@ -251,6 +275,7 @@ dependencies = [ "futures-util", "git2", "glob", + "ignore", "indicatif", "predicates", "regex", @@ -265,9 +290,14 @@ dependencies = [ "toml", "tracing", "tracing-subscriber", - "walkdir", ] +[[package]] +name = "core-foundation-sys" +version = "0.8.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" + [[package]] name = "cpufeatures" version = "0.2.17" @@ -277,6 +307,31 @@ dependencies = [ "libc", ] +[[package]] +name = "crossbeam-deque" +version = "0.8.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9dd111b7b7f7d55b72c0a6ae361660ee5853c9af73f70c3c2ef6858b950e2e51" +dependencies = [ + "crossbeam-epoch", + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-epoch" +version = "0.9.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5b82ac4a3c2ca9c3460964f020e1402edd5753411d7737aa39c3714ad1b5420e" +dependencies = [ + "crossbeam-utils", +] + +[[package]] +name = "crossbeam-utils" +version = "0.8.21" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d0a5c400df2834b80a4c3327b3aad3a4c4cd4de0629063962b03235697506a28" + [[package]] name = "crypto-common" version = "0.1.7" @@ -522,6 +577,19 @@ version = "0.3.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0cc23270f6e1808e30a928bdc84dea0b9b4136a8bc82338574f23baf47bbd280" +[[package]] +name = "globset" +version = "0.4.18" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52dfc19153a48bde0cbd630453615c8151bce3a5adfac7a0aebfbf0a1e1f57e3" +dependencies = [ + "aho-corasick", + "bstr", + "log", + "regex-automata", + "regex-syntax", +] + [[package]] name = "hashbrown" version = "0.15.5" @@ -641,6 +709,30 @@ dependencies = [ "tracing", ] +[[package]] +name = "iana-time-zone" +version = "0.1.65" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e31bc9ad994ba00e440a8aa5c9ef0ec67d5cb5e5cb0cc7f8b744a35b389cc470" +dependencies = [ + "android_system_properties", + "core-foundation-sys", + "iana-time-zone-haiku", + "js-sys", + "log", + "wasm-bindgen", + "windows-core", +] + +[[package]] +name = "iana-time-zone-haiku" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f31827a206f56af32e590ba56d5d2d085f558508192593743f16b2306495269f" +dependencies = [ + "cc", +] + [[package]] name = "icu_collections" version = "2.1.1" @@ -749,6 +841,22 @@ dependencies = [ "icu_properties", ] +[[package]] +name = "ignore" +version = "0.4.25" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3d782a365a015e0f5c04902246139249abf769125006fbe7649e2ee88169b4a" +dependencies = [ + "crossbeam-deque", + "globset", + "log", + "memchr", + "regex-automata", + "same-file", + "walkdir", + "winapi-util", +] + [[package]] name = "indexmap" version = "2.14.0" @@ -2076,12 +2184,65 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "windows-core" +version = "0.62.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8e83a14d34d0623b51dce9581199302a221863196a1dde71a7663a4c2be9deb" +dependencies = [ + "windows-implement", + "windows-interface", + "windows-link", + "windows-result", + "windows-strings", +] + +[[package]] +name = "windows-implement" +version = "0.60.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "053e2e040ab57b9dc951b72c264860db7eb3b0200ba345b4e4c3b14f67855ddf" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "windows-interface" +version = "0.59.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f316c4a2570ba26bbec722032c4099d8c8bc095efccdc15688708623367e358" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "windows-link" version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-result" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7781fa89eaf60850ac3d2da7af8e5242a5ea78d1a11c49bf2910bb5a73853eb5" +dependencies = [ + "windows-link", +] + +[[package]] +name = "windows-strings" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7837d08f69c77cf6b07689544538e017c1bfcf57e34b4c0ff58e6c2cd3b37091" +dependencies = [ + "windows-link", +] + [[package]] name = "windows-sys" version = "0.52.0" diff --git a/Cargo.toml b/Cargo.toml index 095f86f..6f61ebd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,7 +36,7 @@ indicatif = "0.18" # File system dirs = "6" glob = "0.3" -walkdir = "2" +ignore = "0.4" # Error handling anyhow = "1" @@ -52,6 +52,7 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] } regex = "1" futures-util = "0.3" sha2 = "0.10" +chrono = { version = "0.4.44", features = ["serde"] } [dev-dependencies] assert_cmd = "2" diff --git a/src/commands/review.rs b/src/commands/review.rs index bd0d6b5..1fbfee2 100644 --- a/src/commands/review.rs +++ b/src/commands/review.rs @@ -6,10 +6,10 @@ use crate::config::schema::Config; use crate::engine::Severity; use crate::formatters::{OutputFormat, formatter_for}; use crate::git; +use crate::progress::{ProgressReporter, TokenInfo, diff_stats}; /// Exit codes for the review command. pub const EXIT_OK: i32 = 0; -pub const EXIT_ERROR: i32 = 1; pub const EXIT_BLOCKED: i32 = 2; /// Review command options. @@ -55,6 +55,7 @@ pub async fn execute_review( llm_config: &crate::engine::LLMConfig, opts: &ReviewOptions, format: OutputFormat, + progress: &ProgressReporter, ) -> Result { // 1. Get the diff let diff = get_diff(opts, config)?; @@ -73,7 +74,13 @@ pub async fn execute_review( }); } - // 2. Validate size + // 2. Emit parsing_diff event if progress enabled + if progress.is_enabled() { + let (files_changed, lines_changed) = diff_stats(&diff); + progress.parsing_diff(files_changed, lines_changed); + } + + // 3. Validate size let max_size = opts.max_diff_size.unwrap_or(config.hook.max_diff_size); if diff.len() > max_size { anyhow::bail!( @@ -89,17 +96,45 @@ pub async fn execute_review( "running review" ); - // 3. Call the LLM engine - let response = crate::engine::review::review_diff_with_cache( + // 4. Emit calling_llm event + if progress.is_enabled() { + progress.calling_llm(&llm_config.provider, &llm_config.model); + } + + let llm_start = std::time::Instant::now(); + + // 5. Call the LLM engine + let response = match crate::engine::review::review_diff_with_cache( config, llm_config, &diff, opts.stream, !opts.no_cache, + opts.quiet, ) - .await?; + .await + { + Ok(resp) => { + if progress.is_enabled() { + let duration_ms = llm_start.elapsed().as_millis() as u64; + let tokens = resp + .tokens_used + .as_ref() + .map(TokenInfo::from_usage) + .unwrap_or_else(TokenInfo::zero); + progress.llm_response(&tokens, duration_ms); + } + resp + } + Err(e) => { + if progress.is_enabled() { + progress.error(&e.to_string(), "calling_llm"); + } + return Err(e); + } + }; - // 4. Filter by severity if specified + // 6. Filter by severity if specified let min_severity = if let Some(ref sev) = opts.severity { Severity::from_str_lossy(sev) } else { @@ -114,17 +149,31 @@ pub async fn execute_review( .issues .retain(|i| i.severity <= min_severity); - // 5. Format output + // 7. Format output let formatter = formatter_for(format); let output = formatter.format_review(&filtered_response)?; - // 6. Return exit code + // 8. Return exit code let exit_code = if response.should_block && config.hook.mode == "block" { EXIT_BLOCKED } else { EXIT_OK }; + // 9. Emit complete event + if progress.is_enabled() { + let tokens = response + .tokens_used + .as_ref() + .map(TokenInfo::from_usage) + .unwrap_or_else(TokenInfo::zero); + progress.complete( + filtered_response.issues.len(), + exit_code == EXIT_BLOCKED, + &tokens, + ); + } + Ok(ReviewResult { exit_code, output }) } diff --git a/src/config/providers.rs b/src/config/providers.rs index 8cc8e97..b360a3c 100644 --- a/src/config/providers.rs +++ b/src/config/providers.rs @@ -62,8 +62,3 @@ pub fn preset_has_key(preset: &ProviderPreset) -> bool { pub fn detected_presets() -> Vec<&'static ProviderPreset> { PRESETS.iter().filter(|p| preset_has_key(p)).collect() } - -/// Find the first detected provider preset. -pub fn first_detected_preset() -> Option<&'static ProviderPreset> { - PRESETS.iter().find(|p| preset_has_key(p)) -} diff --git a/src/engine/llm.rs b/src/engine/llm.rs index 4e4e1a9..084ae8b 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -10,14 +10,36 @@ use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse, TokenUsage}; /// Shared reqwest::Client with connection pooling. Reused across all LLM requests. /// Created lazily on first use to avoid blocking initialization. /// Per-request timeout is set via .timeout() on the RequestBuilder. +/// +/// Supports `REQUESTS_CA_BUNDLE` env var for custom CA certificates +/// (corporate proxies with self-signed certs). static SHARED_CLIENT: LazyLock = LazyLock::new(|| { - reqwest::Client::builder() - .pool_max_idle_per_host(4) - .build() - .unwrap_or_else(|e| { - tracing::error!("failed to build shared HTTP client: {}", e); - reqwest::Client::new() - }) + let mut builder = reqwest::Client::builder().pool_max_idle_per_host(4); + + // Support custom CA certificates for corporate proxies. + // REQUESTS_CA_BUNDLE is the de-facto standard used by Python requests, + // curl, Node.js, and most HTTP tooling. + if let Ok(ca_path) = std::env::var("REQUESTS_CA_BUNDLE") { + match std::fs::read(&ca_path) { + Ok(ca_data) => match reqwest::Certificate::from_pem(&ca_data) { + Ok(cert) => { + builder = builder.add_root_certificate(cert); + tracing::debug!("loaded custom CA bundle from REQUESTS_CA_BUNDLE"); + } + Err(e) => { + tracing::warn!("failed to parse CA bundle {}: {}", ca_path, e); + } + }, + Err(e) => { + tracing::warn!("failed to read CA bundle {}: {}", ca_path, e); + } + } + } + + builder.build().unwrap_or_else(|e| { + tracing::error!("failed to build shared HTTP client: {}", e); + reqwest::Client::new() + }) }); /// Return the shared reqwest::Client for LLM API requests. @@ -55,6 +77,10 @@ struct ChatChoice { message: ChatMessage, } +/// Usage statistics from the LLM API response. +/// `prompt_tokens` and `completion_tokens` are kept for API completeness +/// (deserialization) even though only `total_tokens` is currently accessed. +#[allow(dead_code)] #[derive(Debug, Clone, Deserialize)] struct Usage { prompt_tokens: u32, @@ -237,8 +263,13 @@ pub async fn review_diff( rules: &[String], response_format: &str, system_prompt_override: Option<&str>, + quiet: bool, ) -> Result { - let spinner = create_spinner("Reviewing diff…"); + let spinner = if quiet { + None + } else { + Some(create_spinner("Reviewing diff…")) + }; let user_prompt = build_review_prompt(diff, focus, rules); @@ -248,7 +279,7 @@ pub async fn review_diff( llm_config, system_prompt, &user_prompt, - Some(&spinner), + spinner.as_ref(), response_format, ) .await?; @@ -256,7 +287,9 @@ pub async fn review_diff( let parse_result = parse_review_response(&raw); match parse_result { Ok(result) => { - spinner.finish_and_clear(); + if let Some(sp) = spinner { + sp.finish_and_clear(); + } Ok(ReviewResponse { issues: result.0, summary: result.1, @@ -267,7 +300,9 @@ pub async fn review_diff( Err(e) => { // LLM produced invalid JSON — retry once with stricter prompt debug!(error = %e, "first parse attempt failed, retrying LLM request"); - spinner.set_message("Retrying (parse error)…"); + if let Some(sp) = &spinner { + sp.set_message("Retrying (parse error)…"); + } let strict_prompt = format!( "{}\n\nIMPORTANT: Your response MUST contain only valid JSON. \ Ensure all strings use proper JSON escape sequences. \ @@ -278,12 +313,14 @@ pub async fn review_diff( llm_config, system_prompt, &strict_prompt, - Some(&spinner), + spinner.as_ref(), response_format, ) .await?; let (issues, summary, tokens_used) = parse_review_response(&retry_raw)?; - spinner.finish_and_clear(); + if let Some(sp) = spinner { + sp.finish_and_clear(); + } Ok(ReviewResponse { issues, summary, diff --git a/src/engine/review.rs b/src/engine/review.rs index 470cf30..8cd18e5 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -3,7 +3,7 @@ use tracing::{debug, instrument}; use crate::config::schema::Config; use crate::engine::llm; -use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse, ScanResponse}; +use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse}; /// Load a custom system prompt from a file path. /// Returns the file content, or None if the file doesn't exist, can't be read, @@ -51,33 +51,6 @@ pub fn resolve_system_prompt(inline: Option<&str>, file_path: Option<&str>) -> O } } -/// Run a code review on the given diff string. -/// -/// Builds the prompt from the diff + config focus/rules, calls the LLM, -/// parses the response, and sets `should_block` based on the config's -/// `hook.min_severity` threshold. -#[instrument(skip_all)] -pub async fn review_diff( - config: &Config, - llm_config: &LLMConfig, - diff: &str, -) -> Result { - review_diff_inner(config, llm_config, diff, false, true).await -} - -/// Run a code review on the given diff string with optional streaming. -/// -/// When `stream` is true, LLM tokens are printed to stdout in real-time. -#[instrument(skip_all)] -pub async fn review_diff_with_stream( - config: &Config, - llm_config: &LLMConfig, - diff: &str, - stream: bool, -) -> Result { - review_diff_inner(config, llm_config, diff, stream, true).await -} - /// Run a code review on the given diff string with optional streaming and cache control. /// /// When `stream` is true, LLM tokens are printed to stdout in real-time. @@ -89,8 +62,9 @@ pub async fn review_diff_with_cache( diff: &str, stream: bool, use_cache: bool, + quiet: bool, ) -> Result { - review_diff_inner(config, llm_config, diff, stream, use_cache).await + review_diff_inner(config, llm_config, diff, stream, use_cache, quiet).await } async fn review_diff_inner( @@ -99,6 +73,7 @@ async fn review_diff_inner( diff: &str, stream: bool, use_cache: bool, + quiet: bool, ) -> Result { debug!( diff_len = diff.len(), @@ -155,6 +130,7 @@ async fn review_diff_inner( &config.rules, &config.response_format, review_prompt.as_deref(), + quiet, ) .await? }; @@ -208,74 +184,6 @@ async fn review_diff_inner( Ok(response) } -/// Scan a full project or set of files. -/// -/// Walks the directory, filters files, batches them, calls the LLM, -/// and aggregates results. -#[instrument(skip_all)] -pub async fn scan_project( - config: &Config, - llm_config: &LLMConfig, - files_content: &str, - files_count: usize, - lines_count: usize, -) -> Result { - debug!( - files = files_count, - lines = lines_count, - "starting project scan" - ); - - if files_content.trim().is_empty() { - return Ok(ScanResponse { - issues: vec![], - files_scanned: 0, - lines_scanned: 0, - summary: "No files to scan.".to_string(), - tokens_used: None, - should_block: false, - }); - } - - // Resolve custom system prompt for scan - let scan_prompt = resolve_system_prompt( - config.scan_system_prompt_override.as_deref(), - config.scan_system_prompt_file.as_deref(), - ); - - let (issues, summary, tokens_used) = llm::scan_files( - llm_config, - files_content, - &config.focus, - &config.rules, - &config.response_format, - scan_prompt.as_deref(), - ) - .await?; - - // Apply ignore rules - let issues = apply_ignore_rules(issues, &config.ignore.rules); - - // Calculate should_block - let min_severity = config.hook.min_severity_level(); - // Ord order: Critical(0) < Major(1) < Minor(2) < Info(3) - let should_block = issues.iter().any(|issue| issue.severity <= min_severity); - - let default_summary = format!( - "Scanned {} files, found {} issues.", - files_count, - issues.len() - ); - Ok(ScanResponse { - issues, - files_scanned: files_count, - lines_scanned: lines_count, - summary: summary.unwrap_or(default_summary), - tokens_used, - should_block, - }) -} - /// Filter out issues whose issue_type matches any ignored rule pattern. fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> Vec { if ignore_rules.is_empty() { diff --git a/src/engine/scanner.rs b/src/engine/scanner.rs index a69f718..ade4da7 100644 --- a/src/engine/scanner.rs +++ b/src/engine/scanner.rs @@ -3,9 +3,9 @@ use std::path::Path; use anyhow::Result; use glob::Pattern; +use ignore::WalkBuilder; use indicatif::{ProgressBar, ProgressStyle}; use tracing::debug; -use walkdir::WalkDir; /// A file to be scanned, with its relative path and content. #[derive(Debug, Clone)] @@ -18,12 +18,6 @@ pub struct FileEntry { pub lines: usize, } -/// Maximum characters per batch to avoid exceeding LLM token limits. -const MAX_BATCH_CHARS: usize = 60_000; - -/// Maximum files per batch. -const MAX_FILES_PER_BATCH: usize = 20; - /// File extensions to include in scans by default (source code). const DEFAULT_EXTENSIONS: &[&str] = &[ "rs", "py", "js", "ts", "tsx", "jsx", "go", "java", "kt", "rb", "c", "cpp", "h", "hpp", "cs", @@ -34,6 +28,8 @@ const DEFAULT_EXTENSIONS: &[&str] = &[ /// Walk a project directory, respecting .gitignore, and collect scannable files. /// /// Uses `include` and `exclude` glob patterns to filter results. +/// The `ignore` crate handles .gitignore (including nested), .git/info/exclude, +/// and hidden-file filtering automatically. pub fn walk_project( root: &Path, include_patterns: &[String], @@ -62,9 +58,6 @@ pub fn walk_project( .filter_map(|p| Pattern::new(p).ok()) .collect(); - // Load .gitignore if present - let gitignore = load_gitignore(root); - let mut entries = Vec::new(); let spinner = ProgressBar::new_spinner(); @@ -76,17 +69,23 @@ pub fn walk_project( ); spinner.set_message("Scanning files…"); - for entry in WalkDir::new(root) - .into_iter() - .filter_entry(|e| { - // Skip hidden files/directories - e.file_name() - .to_str() - .map(|s| !s.starts_with('.')) - .unwrap_or(true) - }) - .filter_map(|e| e.ok()) - { + let walker = WalkBuilder::new(root) + .hidden(true) // respect hidden files (skip dotfiles) + .git_ignore(true) // respect .gitignore at all levels + .git_global(true) // respect core.excludesFile + .git_exclude(true) // respect .git/info/exclude + .require_git(false) // apply gitignore rules even outside git repos + .build(); + + for result in walker { + let entry = match result { + Ok(e) => e, + Err(err) => { + debug!(error = %err, "error during directory walk"); + continue; + } + }; + let path = entry.path(); // Skip directories @@ -94,17 +93,13 @@ pub fn walk_project( continue; } + // Get relative path (entry.depth() ensures it's under root) let relative = path .strip_prefix(root) .unwrap_or(path) .to_string_lossy() .to_string(); - // Check gitignore - if is_ignored_by_gitignore(&relative, &gitignore) { - continue; - } - // Check exclude patterns if exclude_globs.iter().any(|g| g.matches(&relative)) { continue; @@ -200,46 +195,3 @@ pub fn format_batch_for_prompt(files: &[FileEntry]) -> String { output } - -/// Load .gitignore patterns from a project root. -fn load_gitignore(root: &Path) -> Vec { - let gitignore_path = root.join(".gitignore"); - if !gitignore_path.is_file() { - return Vec::new(); - } - - std::fs::read_to_string(&gitignore_path) - .unwrap_or_default() - .lines() - .filter(|line| { - let trimmed = line.trim(); - !trimmed.is_empty() && !trimmed.starts_with('#') - }) - .map(|s| s.to_string()) - .collect() -} - -/// Simple check if a path matches any gitignore pattern. -fn is_ignored_by_gitignore(path: &str, patterns: &[String]) -> bool { - for pattern in patterns { - let p = pattern.trim(); - if p.is_empty() { - continue; - } - // Simple glob matching - if let Ok(glob) = Pattern::new(&format!("**/{p}")) { - if glob.matches(path) { - return true; - } - } - // Direct prefix match for directory patterns - if p.ends_with('/') && path.starts_with(p) { - return true; - } - // Exact file match - if path == p || path.ends_with(&format!("/{p}")) { - return true; - } - } - false -} diff --git a/src/engine/types.rs b/src/engine/types.rs index ca7e7a7..9837705 100644 --- a/src/engine/types.rs +++ b/src/engine/types.rs @@ -54,7 +54,12 @@ impl fmt::Display for Severity { } } -/// Issue type categories +/// Issue type categories. +/// +/// Kept for API completeness — used for deserialization and future typed +/// issue-type matching. Currently the LLM returns issue_type as a string, +/// but this enum provides a structured alternative. +#[allow(dead_code)] #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] pub enum IssueType { @@ -66,6 +71,7 @@ pub enum IssueType { Suggestion, } +#[allow(dead_code)] impl fmt::Display for IssueType { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { @@ -79,6 +85,7 @@ impl fmt::Display for IssueType { } } +#[allow(dead_code)] impl IssueType { /// Parse from string with lenient matching (accepts plural forms, etc.) pub fn from_str_lossy(s: &str) -> Self { @@ -431,17 +438,32 @@ impl Default for LLMConfig { } } -/// CLI exit codes +/// CLI exit codes. +/// +/// Kept for API completeness — consumers and tests may reference these. +#[allow(dead_code)] pub const EXIT_OK: i32 = 0; +#[allow(dead_code)] pub const EXIT_ERROR: i32 = 1; +#[allow(dead_code)] pub const EXIT_BLOCKED: i32 = 2; +#[allow(dead_code)] pub const EXIT_AUTH_ERROR: i32 = 3; -/// Maximum diff size in bytes (50KB by default) +/// Maximum diff size in bytes (50KB by default). +/// +/// Kept for API completeness — future commands may enforce this limit. +#[allow(dead_code)] pub const MAX_DIFF_SIZE: usize = 50 * 1024; -/// Maximum files per scan batch +/// Maximum files per scan batch. +/// +/// Kept for API completeness — scanner uses matching values inline. +#[allow(dead_code)] pub const MAX_SCAN_BATCH_FILES: usize = 20; -/// Maximum characters per scan batch +/// Maximum characters per scan batch. +/// +/// Kept for API completeness — scanner uses matching values inline. +#[allow(dead_code)] pub const MAX_SCAN_BATCH_CHARS: usize = 80_000; diff --git a/src/formatters/compact.rs b/src/formatters/compact.rs index 10aaa2f..c021ea8 100644 --- a/src/formatters/compact.rs +++ b/src/formatters/compact.rs @@ -26,6 +26,12 @@ impl Formatter for CompactFormatter { output.push_str(&format_issue_compact(issue)); } + // Footer watermark (only when issues found) + output.push_str(&format!( + "Reviewed by Cora v{}\n", + env!("CARGO_PKG_VERSION") + )); + Ok(output) } @@ -43,6 +49,14 @@ impl Formatter for CompactFormatter { output.push_str(&format_issue_compact(issue)); } + // Footer watermark (only when issues found) + if !response.issues.is_empty() { + output.push_str(&format!( + "Reviewed by Cora v{}\n", + env!("CARGO_PKG_VERSION") + )); + } + Ok(output) } } @@ -136,6 +150,15 @@ mod tests { let fmt = CompactFormatter; let output = fmt.format_review(&empty_response()).unwrap(); assert!(output.contains("No issues found")); + // No watermark when no issues + assert!(!output.contains("Reviewed by Cora")); + } + + #[test] + fn format_review_with_issues_has_watermark() { + let fmt = CompactFormatter; + let output = fmt.format_review(&sample_response()).unwrap(); + assert!(output.contains("Reviewed by Cora v")); } #[test] diff --git a/src/formatters/json_fmt.rs b/src/formatters/json_fmt.rs index e18cf03..2e68991 100644 --- a/src/formatters/json_fmt.rs +++ b/src/formatters/json_fmt.rs @@ -1,23 +1,48 @@ use anyhow::Result; +use serde_json::{Value, json}; use crate::engine::{ReviewResponse, ScanResponse}; use crate::formatters::Formatter; -/// JSON formatter: outputs raw JSON. +/// JSON formatter: outputs raw JSON with a watermark footer. pub struct JsonFormatter; impl Formatter for JsonFormatter { fn format_review(&self, response: &ReviewResponse) -> Result { - let json = serde_json::to_string_pretty(response)?; + let mut obj = serde_json::to_value(response)?; + add_watermark(&mut obj, &response.issues); + let json = serde_json::to_string_pretty(&obj)?; Ok(json) } fn format_scan(&self, response: &ScanResponse) -> Result { - let json = serde_json::to_string_pretty(response)?; + let mut obj = serde_json::to_value(response)?; + add_watermark_scan(&mut obj, &response.issues); + let json = serde_json::to_string_pretty(&obj)?; Ok(json) } } +/// Add a `reviewed_by` watermark field to a JSON value when issues are present. +fn add_watermark(obj: &mut Value, issues: &[crate::engine::ReviewIssue]) { + if !issues.is_empty() { + if let Some(map) = obj.as_object_mut() { + map.insert( + "reviewed_by".to_string(), + json!({ + "tool": "cora", + "version": env!("CARGO_PKG_VERSION") + }), + ); + } + } +} + +/// Add a `reviewed_by` watermark field for scan responses. +fn add_watermark_scan(obj: &mut Value, issues: &[crate::engine::ReviewIssue]) { + add_watermark(obj, issues); +} + #[cfg(test)] mod tests { use super::*; @@ -92,6 +117,19 @@ mod tests { let output = fmt.format_review(&empty_response()).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&output).unwrap(); assert_eq!(parsed["issues"].as_array().unwrap().len(), 0); + // No watermark when no issues + assert!(parsed.get("reviewed_by").is_none()); + } + + #[test] + fn format_review_with_issues_has_watermark() { + let fmt = JsonFormatter; + let output = fmt.format_review(&sample_response()).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&output).unwrap(); + // Watermark should be present when issues exist + let reviewed_by = &parsed["reviewed_by"]; + assert_eq!(reviewed_by["tool"].as_str().unwrap(), "cora"); + assert!(reviewed_by["version"].as_str().is_some()); } #[test] diff --git a/src/formatters/pretty.rs b/src/formatters/pretty.rs index ae92857..63e04a2 100644 --- a/src/formatters/pretty.rs +++ b/src/formatters/pretty.rs @@ -81,6 +81,13 @@ impl Formatter for PrettyFormatter { )); } + // Footer watermark (only when issues found) + output.push_str(&format!( + "\n{}\n{}", + "─".repeat(60).dimmed(), + format!("Reviewed by Cora v{}", env!("CARGO_PKG_VERSION")).dimmed() + )); + Ok(output) } @@ -158,6 +165,15 @@ impl Formatter for PrettyFormatter { )); } + // Footer watermark (only when issues found) + if !response.issues.is_empty() { + output.push_str(&format!( + "\n{}\n{}", + "─".repeat(60).dimmed(), + format!("Reviewed by Cora v{}", env!("CARGO_PKG_VERSION")).dimmed() + )); + } + Ok(output) } } @@ -294,6 +310,22 @@ mod tests { let fmt = PrettyFormatter; let output = fmt.format_review(&empty_response()).unwrap(); assert!(output.contains("No issues found")); + // No watermark when no issues + assert!(!output.contains("Reviewed by Cora")); + } + + #[test] + fn format_review_with_issues_has_watermark() { + let fmt = PrettyFormatter; + let output = fmt.format_review(&sample_response()).unwrap(); + assert!(output.contains("Reviewed by Cora v")); + } + + #[test] + fn format_scan_with_issues_has_watermark() { + let fmt = PrettyFormatter; + let output = fmt.format_scan(&sample_scan_response()).unwrap(); + assert!(output.contains("Reviewed by Cora v")); } #[test] diff --git a/src/formatters/sarif.rs b/src/formatters/sarif.rs index 5e08451..8122d2a 100644 --- a/src/formatters/sarif.rs +++ b/src/formatters/sarif.rs @@ -105,6 +105,18 @@ fn build_sarif(issues: &[ReviewIssue]) -> Value { }) .collect(); + // Build invocations array with watermark + let invocations = if !issues.is_empty() { + json!([{ + "executionSuccessful": true, + "properties": { + "cora.watermark": format!("Reviewed by Cora v{}", env!("CARGO_PKG_VERSION")) + } + }]) + } else { + json!([]) + }; + json!({ "$schema": "https://raw.githubusercontent.com/oasis-tcs/sarif-spec/master/Schemata/sarif-schema-2.1.0.json", "version": "2.1.0", @@ -117,7 +129,8 @@ fn build_sarif(issues: &[ReviewIssue]) -> Value { "rules": rules } }, - "results": results + "results": results, + "invocations": invocations }] }) } @@ -230,6 +243,22 @@ mod tests { let parsed: serde_json::Value = serde_json::from_str(&output).unwrap(); let results = parsed["runs"][0]["results"].as_array().unwrap(); assert!(results.is_empty()); + // No watermark invocations when no issues + let invocations = parsed["runs"][0]["invocations"].as_array().unwrap(); + assert!(invocations.is_empty()); + } + + #[test] + fn sarif_with_issues_has_watermark_invocation() { + let fmt = SarifFormatter; + let output = fmt.format_review(&sample_response()).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&output).unwrap(); + let invocations = parsed["runs"][0]["invocations"].as_array().unwrap(); + assert_eq!(invocations.len(), 1); + let wm = &invocations[0]["properties"]["cora.watermark"]; + let wm_str = wm.as_str().unwrap(); + assert!(wm_str.contains("Cora")); + assert!(wm_str.contains("v")); } #[test] diff --git a/src/git/diff.rs b/src/git/diff.rs index b0d086b..aa50ac0 100644 --- a/src/git/diff.rs +++ b/src/git/diff.rs @@ -159,6 +159,10 @@ fn parse_remote_url(url: &str) -> (String, String) { } /// Check if a path is inside a git worktree. +/// +/// Kept for API completeness — useful for pre-commit hooks and future commands +/// that need to guard against running outside a repository. +#[allow(dead_code)] pub fn is_inside_git_repo(dir: Option<&Path>) -> bool { let cwd = std::env::current_dir().unwrap_or_else(|_| std::path::PathBuf::from(".")); let search_dir = dir.unwrap_or(&cwd); diff --git a/src/git/files.rs b/src/git/files.rs index 1961547..7d6d933 100644 --- a/src/git/files.rs +++ b/src/git/files.rs @@ -1,192 +1,2 @@ -use std::path::Path; - -use anyhow::{Context, Result}; -use git2::{Repository, Status, StatusOptions, StatusShow, TreeWalkMode, TreeWalkResult}; -use glob::Pattern; - -/// Open the git repository at or above the current working directory. -pub fn open_repo() -> Result { - Repository::discover(std::env::current_dir()?).context("not inside a git repository") -} - -/// List all tracked files in the repository. -pub fn list_tracked_files() -> Result> { - let repo = open_repo()?; - let head = repo.head().context("no HEAD reference")?; - let commit = head.peel_to_commit().context("HEAD is not a commit")?; - let tree = commit.tree().context("failed to read tree")?; - - let mut files = Vec::new(); - tree.walk(TreeWalkMode::PreOrder, |_root, entry| { - if entry.kind() == Some(git2::ObjectType::Blob) { - if let Some(name) = entry.name() { - if !is_ignored_dir(name) { - files.push(name.to_string()); - } - } - } - TreeWalkResult::Ok - }) - .context("failed to walk tree")?; - - Ok(files) -} - -/// Check if a file path falls in a commonly ignored directory. -fn is_ignored_dir(path: &str) -> bool { - let ignored_prefixes = [ - "node_modules/", - ".git/", - "vendor/", - "__pycache__/", - ".venv/", - "venv/", - "target/", - "build/", - "dist/", - ".next/", - "coverage/", - ]; - for prefix in &ignored_prefixes { - if path.starts_with(prefix) { - return true; - } - } - false -} - -/// List files that changed on the current branch compared to `base_branch`. -pub fn list_changed_files(base_branch: &str) -> Result> { - let repo = open_repo()?; - - let base_oid = repo - .revparse_single(base_branch) - .with_context(|| format!("cannot resolve branch '{base_branch}'"))? - .peel_to_commit()? - .id(); - - let _head_oid = repo.head()?.peel_to_commit()?.id(); - - let base_tree = repo - .find_tree(base_oid) - .context("failed to find base tree")?; - let head_commit = repo - .head()? - .peel_to_commit() - .context("HEAD is not a commit")?; - let head_tree = repo - .find_tree(head_commit.tree_id()) - .context("failed to find HEAD tree")?; - - let mut opts = git2::DiffOptions::new(); - let diff = repo - .diff_tree_to_tree(Some(&base_tree), Some(&head_tree), Some(&mut opts)) - .context("failed to diff trees")?; - - let mut files = Vec::new(); - for delta in diff.deltas() { - if let Some(path) = delta.new_file().path() { - if let Some(name) = path.to_str() { - files.push(name.to_string()); - } - } - } - - Ok(files) -} - -/// List files that are currently staged (in the index). -pub fn list_staged_files() -> Result> { - let repo = open_repo()?; - let head = repo.head().ok(); - let head_tree = head - .and_then(|h| h.peel_to_commit().ok()) - .and_then(|c| c.tree().ok()); - - let mut opts = git2::DiffOptions::new(); - let diff = repo - .diff_tree_to_index(head_tree.as_ref(), None, Some(&mut opts)) - .context("failed to diff staged files")?; - - let mut files = Vec::new(); - for delta in diff.deltas() { - if let Some(path) = delta.new_file().path() { - if let Some(name) = path.to_str() { - files.push(name.to_string()); - } - } - } - - Ok(files) -} - -/// List all modified/added/deleted files (staged + unstaged). -pub fn list_all_changed_files() -> Result> { - let repo = open_repo()?; - - let mut opts = StatusOptions::new(); - opts.show(StatusShow::IndexAndWorkdir) - .include_untracked(true) - .recurse_untracked_dirs(true); - - let statuses = repo - .statuses(Some(&mut opts)) - .context("failed to get git status")?; - - let mut files = Vec::new(); - for entry in statuses.iter() { - if entry.status().intersects( - Status::INDEX_NEW - | Status::INDEX_MODIFIED - | Status::INDEX_DELETED - | Status::WT_MODIFIED - | Status::WT_NEW - | Status::WT_DELETED, - ) { - if let Some(path) = entry.path() { - files.push(path.to_string()); - } - } - } - - files.sort(); - files.dedup(); - Ok(files) -} - -/// Filter a list of file paths using glob include/exclude patterns. -pub fn filter_by_globs(files: &[String], include: &[String], exclude: &[String]) -> Vec { - let include_patterns: Vec = include - .iter() - .filter_map(|p| Pattern::new(p).ok()) - .collect(); - - let exclude_patterns: Vec = exclude - .iter() - .filter_map(|p| Pattern::new(p).ok()) - .collect(); - - files - .iter() - .filter(|f| { - let path = Path::new(f); - let included = include_patterns.is_empty() - || include_patterns.iter().any(|p| p.matches_path(path)); - - let excluded = exclude_patterns.iter().any(|p| p.matches_path(path)); - - included && !excluded - }) - .cloned() - .collect() -} - -/// Check if a given path is gitignored. -pub fn is_gitignored(path: &Path) -> Result { - let repo = open_repo()?; - let relative = path - .strip_prefix(repo.workdir().unwrap_or(path)) - .unwrap_or(path); - - Ok(repo.is_path_ignored(relative)?) -} +// Git file utility functions. +// Currently empty — functions migrated to individual command modules as needed. diff --git a/src/hook/install.rs b/src/hook/install.rs index 9d0f4b9..a62e3f9 100644 --- a/src/hook/install.rs +++ b/src/hook/install.rs @@ -102,6 +102,10 @@ fn find_git_hooks_dir() -> Result { } /// Check whether the cora pre-commit hook is installed. +/// +/// Kept for API completeness — useful for future `cora hook status` and +/// guard logic in pre-commit hook template. +#[allow(dead_code)] pub fn is_hook_installed() -> Result { let hooks_dir = find_git_hooks_dir()?; let hook_path = hooks_dir.join("pre-commit"); diff --git a/src/main.rs b/src/main.rs index 09d1f2a..bb2d88d 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,7 +1,3 @@ -// Suppress unused warnings for utility functions not yet called from main -// These will be used as the codebase grows -#![allow(dead_code)] - use anyhow::Result; use clap::{Parser, Subcommand}; use colored::Colorize; @@ -14,6 +10,7 @@ mod engine; mod formatters; mod git; mod hook; +mod progress; use commands::{auth, completion, config_cmd, hook_cmd, init, providers, review, scan, upload}; use config::loader; @@ -103,10 +100,18 @@ enum Command { #[clap(long)] unstaged: bool, + /// Override max diff size in chars (default: 50000 from config) + #[clap(long, name = "CHARS")] + max_diff_size: Option, + /// Stream LLM response tokens in real-time #[clap(long)] stream: bool, + /// Output structured NDJSON progress events to stderr + #[clap(long)] + progress: bool, + /// Suppress all output except the formatted review result #[clap(long, short)] quiet: bool, @@ -287,7 +292,9 @@ async fn main() -> Result<()> { commit, diff_file, unstaged, + max_diff_size, stream, + progress, quiet, severity, no_cache, @@ -305,7 +312,9 @@ async fn main() -> Result<()> { commit, diff_file, unstaged, + max_diff_size, stream, + progress, quiet, severity, no_cache, @@ -409,7 +418,9 @@ struct ReviewOpts { commit: Option, diff_file: Option, unstaged: bool, + max_diff_size: Option, stream: bool, + progress: bool, quiet: bool, severity: Option, upload: bool, @@ -448,6 +459,15 @@ async fn cmd_review(globals: &GlobalOptions, opts: ReviewOpts) -> Result { resolve_format(globals.format.as_deref(), &config)? }; + let progress_reporter = if opts.progress { + progress::ProgressReporter::new() + } else { + progress::ProgressReporter::disabled() + }; + + // Emit started event if progress enabled + progress_reporter.started("review", opts.base.as_deref()); + let review_opts = review::ReviewOptions { staged: opts.staged, unpushed: opts.unpushed, @@ -455,15 +475,15 @@ async fn cmd_review(globals: &GlobalOptions, opts: ReviewOpts) -> Result { commit: opts.commit.clone(), diff_file: opts.diff_file.clone(), unstaged: opts.unstaged, - max_diff_size: None, + max_diff_size: opts.max_diff_size, stream: opts.stream, - quiet: opts.quiet, + quiet: opts.quiet || opts.progress, severity: opts.severity.clone(), no_cache: opts.no_cache, }; - // When streaming and not quiet, show a simpler message - if opts.stream && !opts.quiet { + // When streaming and not quiet/progress, show a simpler message + if opts.stream && !opts.quiet && !opts.progress { eprintln!( "{}", format!( @@ -475,8 +495,14 @@ async fn cmd_review(globals: &GlobalOptions, opts: ReviewOpts) -> Result { } // Execute the review (returns formatted output) - let result = - review::execute_review(&config, &llm_config, &review_opts, effective_format).await?; + let result = review::execute_review( + &config, + &llm_config, + &review_opts, + effective_format, + &progress_reporter, + ) + .await?; // Print the formatted output print!("{}", result.output); diff --git a/src/progress.rs b/src/progress.rs new file mode 100644 index 0000000..77fbb9b --- /dev/null +++ b/src/progress.rs @@ -0,0 +1,399 @@ +//! Structured NDJSON progress reporting for the `cora review` command. +//! +//! When `--progress` is set, progress events are written to stderr as one JSON object +//! per line (NDJSON). When not set, the reporter is a no-op with zero overhead. + +use std::io::Write; + +use chrono::Utc; +use serde::Serialize; + +/// NDJSON progress event emitted to stderr. +#[derive(Debug, Clone, Serialize)] +#[serde(tag = "status", rename_all = "snake_case")] +pub enum ProgressEvent { + /// Review execution started. + Started { + mode: String, + base: Option, + timestamp: String, + }, + /// Diff has been parsed and stats are available. + ParsingDiff { + files_changed: usize, + lines_changed: usize, + timestamp: String, + }, + /// About to call the LLM. + CallingLlm { + provider: String, + model: String, + timestamp: String, + }, + /// LLM responded. + LlmResponse { + tokens: TokenInfo, + duration_ms: u64, + timestamp: String, + }, + /// Review completed successfully. + Complete { + issues: usize, + blocked: bool, + tokens: TokenInfo, + timestamp: String, + }, + /// An error occurred. + Error { + message: String, + phase: String, + timestamp: String, + }, +} + +/// Token usage information for progress events. +#[derive(Debug, Clone, Serialize)] +pub struct TokenInfo { + pub input: u32, + pub output: u32, + #[serde(skip_serializing_if = "Option::is_none")] + pub estimated_cost_usd: Option, +} + +impl TokenInfo { + pub fn from_usage(usage: &crate::engine::types::TokenUsage) -> Self { + TokenInfo { + input: usage.input_tokens, + output: usage.output_tokens, + estimated_cost_usd: if usage.estimated_cost_usd > 0.0 { + Some(usage.estimated_cost_usd) + } else { + None + }, + } + } + + pub fn zero() -> Self { + TokenInfo { + input: 0, + output: 0, + estimated_cost_usd: None, + } + } +} + +/// A progress reporter that emits NDJSON events to stderr. +/// +/// Create with `ProgressReporter::new()` when `--progress` is enabled, +/// or use `ProgressReporter::disabled()` (the default) for a no-op. +#[derive(Debug)] +pub struct ProgressReporter { + enabled: bool, +} + +impl ProgressReporter { + /// Create a no-op progress reporter (when `--progress` is not set). + pub fn disabled() -> Self { + ProgressReporter { enabled: false } + } + + /// Create an active progress reporter (when `--progress` is set). + pub fn new() -> Self { + ProgressReporter { enabled: true } + } + + /// Returns true if this reporter is active. + pub fn is_enabled(&self) -> bool { + self.enabled + } + + fn now_iso() -> String { + Utc::now().to_rfc3339() + } + + fn emit(&self, event: &ProgressEvent) { + if !self.enabled { + return; + } + if let Ok(json) = serde_json::to_string(event) { + let _ = writeln!(std::io::stderr(), "{}", json); + } + } + + /// Emit a "started" event. + pub fn started(&self, mode: &str, base: Option<&str>) { + self.emit(&ProgressEvent::Started { + mode: mode.to_string(), + base: base.map(|s| s.to_string()), + timestamp: Self::now_iso(), + }); + } + + /// Emit a "parsing_diff" event. + pub fn parsing_diff(&self, files_changed: usize, lines_changed: usize) { + self.emit(&ProgressEvent::ParsingDiff { + files_changed, + lines_changed, + timestamp: Self::now_iso(), + }); + } + + /// Emit a "calling_llm" event. + pub fn calling_llm(&self, provider: &str, model: &str) { + self.emit(&ProgressEvent::CallingLlm { + provider: provider.to_string(), + model: model.to_string(), + timestamp: Self::now_iso(), + }); + } + + /// Emit a "llm_response" event. + pub fn llm_response(&self, tokens: &TokenInfo, duration_ms: u64) { + self.emit(&ProgressEvent::LlmResponse { + tokens: tokens.clone(), + duration_ms, + timestamp: Self::now_iso(), + }); + } + + /// Emit a "complete" event. + pub fn complete(&self, issues: usize, blocked: bool, tokens: &TokenInfo) { + self.emit(&ProgressEvent::Complete { + issues, + blocked, + tokens: tokens.clone(), + timestamp: Self::now_iso(), + }); + } + + /// Emit an "error" event. + pub fn error(&self, message: &str, phase: &str) { + self.emit(&ProgressEvent::Error { + message: message.to_string(), + phase: phase.to_string(), + timestamp: Self::now_iso(), + }); + } +} + +/// Extract diff statistics: (files_changed, lines_changed). +pub fn diff_stats(diff: &str) -> (usize, usize) { + let mut files_changed = 0; + let mut lines_changed = 0; + + for line in diff.lines() { + let trimmed = line.trim_start(); + // Count file headers from diff output + if trimmed.starts_with("--- ") || trimmed.starts_with("+++ ") { + // Only count once per file pair (the --- line) + if trimmed.starts_with("--- ") { + files_changed += 1; + } + } + // Count changed lines (lines starting with + or - but not +++ or ---) + if (trimmed.starts_with('+') && !trimmed.starts_with("++")) + || (trimmed.starts_with('-') && !trimmed.starts_with("--")) + { + lines_changed += 1; + } + } + + // files_changed is double-counted from --- lines (each pair has one ---), + // which is correct since we only increment on ---. + (files_changed, lines_changed) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn diff_stats_empty() { + let (files, lines) = diff_stats(""); + assert_eq!(files, 0); + assert_eq!(lines, 0); + } + + #[test] + fn diff_stats_single_file() { + let diff = "\ +--- a/src/main.rs ++++ b/src/main.rs +@@ -1,4 +1,5 @@ + fn main() { +- println!(\"hello\"); ++ println!(\"world\"); ++ let x = 1; + } +"; + let (files, lines) = diff_stats(diff); + assert_eq!(files, 1); + assert_eq!(lines, 3); // -1 +2 +1 + } + + #[test] + fn diff_stats_multiple_files() { + let diff = "\ +--- a/file1.rs ++++ b/file1.rs +@@ -1,3 +1,3 @@ +- old ++ new +--- a/file2.rs ++++ b/file2.rs +@@ -1,3 +1,4 @@ +- foo ++ bar ++ baz +"; + let (files, lines) = diff_stats(diff); + assert_eq!(files, 2); + assert_eq!(lines, 5); // -1 +1 -1 +1 +1 + } + + #[test] + fn disabled_reporter_no_output() { + let reporter = ProgressReporter::disabled(); + assert!(!reporter.is_enabled()); + // These should not panic or write anything + reporter.started("review", None); + reporter.parsing_diff(1, 10); + reporter.calling_llm("openai", "gpt-4o-mini"); + reporter.llm_response(&TokenInfo::zero(), 100); + reporter.complete(0, false, &TokenInfo::zero()); + reporter.error("test error", "test_phase"); + } + + #[test] + fn enabled_reporter_is_enabled() { + let reporter = ProgressReporter::new(); + assert!(reporter.is_enabled()); + } + + #[test] + fn progress_event_started_serializes() { + let event = ProgressEvent::Started { + mode: "review".to_string(), + base: Some("origin/develop".to_string()), + timestamp: "2026-06-02T10:00:00+00:00".to_string(), + }; + let json = serde_json::to_string(&event).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["status"], "started"); + assert_eq!(parsed["mode"], "review"); + assert_eq!(parsed["base"], "origin/develop"); + } + + #[test] + fn progress_event_parsing_diff_serializes() { + let event = ProgressEvent::ParsingDiff { + files_changed: 12, + lines_changed: 340, + timestamp: "2026-06-02T10:00:00+00:00".to_string(), + }; + let json = serde_json::to_string(&event).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["status"], "parsing_diff"); + assert_eq!(parsed["files_changed"], 12); + assert_eq!(parsed["lines_changed"], 340); + } + + #[test] + fn progress_event_calling_llm_serializes() { + let event = ProgressEvent::CallingLlm { + provider: "openai".to_string(), + model: "gpt-4o-mini".to_string(), + timestamp: "2026-06-02T10:00:00+00:00".to_string(), + }; + let json = serde_json::to_string(&event).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["status"], "calling_llm"); + assert_eq!(parsed["provider"], "openai"); + assert_eq!(parsed["model"], "gpt-4o-mini"); + } + + #[test] + fn progress_event_llm_response_serializes() { + let event = ProgressEvent::LlmResponse { + tokens: TokenInfo { + input: 8200, + output: 2400, + estimated_cost_usd: Some(0.003), + }, + duration_ms: 3200, + timestamp: "2026-06-02T10:00:00+00:00".to_string(), + }; + let json = serde_json::to_string(&event).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["status"], "llm_response"); + assert_eq!(parsed["tokens"]["input"], 8200); + assert_eq!(parsed["tokens"]["output"], 2400); + assert_eq!(parsed["duration_ms"], 3200); + } + + #[test] + fn progress_event_complete_serializes() { + let event = ProgressEvent::Complete { + issues: 3, + blocked: false, + tokens: TokenInfo { + input: 8200, + output: 2400, + estimated_cost_usd: Some(0.003), + }, + timestamp: "2026-06-02T10:00:00+00:00".to_string(), + }; + let json = serde_json::to_string(&event).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["status"], "complete"); + assert_eq!(parsed["issues"], 3); + assert_eq!(parsed["blocked"], false); + } + + #[test] + fn progress_event_error_serializes() { + let event = ProgressEvent::Error { + message: "API timeout after 120s".to_string(), + phase: "calling_llm".to_string(), + timestamp: "2026-06-02T10:00:00+00:00".to_string(), + }; + let json = serde_json::to_string(&event).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["status"], "error"); + assert_eq!(parsed["message"], "API timeout after 120s"); + assert_eq!(parsed["phase"], "calling_llm"); + } + + #[test] + fn token_info_zero() { + let tokens = TokenInfo::zero(); + assert_eq!(tokens.input, 0); + assert_eq!(tokens.output, 0); + assert!(tokens.estimated_cost_usd.is_none()); + } + + #[test] + fn token_info_serializes_without_cost_when_zero() { + let tokens = TokenInfo::zero(); + let json = serde_json::to_string(&tokens).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert!( + !parsed + .as_object() + .unwrap() + .contains_key("estimated_cost_usd") + ); + } + + #[test] + fn token_info_serializes_with_cost_when_nonzero() { + let tokens = TokenInfo { + input: 100, + output: 50, + estimated_cost_usd: Some(0.001), + }; + let json = serde_json::to_string(&tokens).unwrap(); + let parsed: serde_json::Value = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed["estimated_cost_usd"], 0.001); + } +}