diff --git a/Cargo.lock b/Cargo.lock index 369687a..d55320d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -273,6 +273,7 @@ dependencies = [ "reqwest", "serde", "serde_json", + "tempfile", "toml", ] @@ -511,6 +512,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "fastrand" +version = "2.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9f1f227452a390804cdb637b74a86990f2a7d7ba4b7d5693aac9b4dd6defd8d6" + [[package]] name = "find-msvc-tools" version = "0.1.9" @@ -1910,6 +1917,19 @@ dependencies = [ "libc", ] +[[package]] +name = "tempfile" +version = "3.27.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd" +dependencies = [ + "fastrand", + "getrandom 0.3.4", + "once_cell", + "rustix", + "windows-sys 0.61.2", +] + [[package]] name = "termtree" version = "0.5.1" diff --git a/Cargo.toml b/Cargo.toml index 2052364..fe2867b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,7 @@ toml = "^1.0" [dev-dependencies] assert_cmd = "^2.1.1" predicates = "^3.1.3" +tempfile = "^3.10" [profile.release] opt-level = 3 diff --git a/devops/render-release-notes.sh b/devops/render-release-notes.sh index d2b0e8a..5aa0411 100644 --- a/devops/render-release-notes.sh +++ b/devops/render-release-notes.sh @@ -25,6 +25,12 @@ cat <, pub max_concurrent_requests: Option, pub stream: Option, + pub request_timeout_secs: Option, } /// Root of the TOML file: @@ -169,6 +174,7 @@ impl<'a> ConfigResolver<'a> { "base_url" => Some("COMMITBOT_BASE_URL"), "max_concurrent_requests" => Some("COMMITBOT_MAX_CONCURRENT_REQUESTS"), "stream" => Some("COMMITBOT_STREAM"), + "request_timeout_secs" => Some("COMMITBOT_REQUEST_TIMEOUT_SECS"), _ => None, } } @@ -202,6 +208,18 @@ impl<'a> ConfigResolver<'a> { } } + fn file_u64(&self, key: &str, repo: bool) -> Option { + let cfg = if repo { + &self.file_repo + } else { + &self.file_default + }; + match key { + "request_timeout_secs" => cfg.request_timeout_secs, + _ => None, + } + } + fn file_bool(&self, key: &str, repo: bool) -> Option { let cfg = if repo { &self.file_repo @@ -226,6 +244,11 @@ impl<'a> ConfigResolver<'a> { env::var(env_key).ok().and_then(|s| s.parse::().ok()) } + fn env_u64(&self, key: &str) -> Option { + let env_key = self.env_key_for(key)?; + env::var(env_key).ok().and_then(|s| s.parse::().ok()) + } + fn env_bool(&self, key: &str) -> Option { let env_key = self.env_key_for(key)?; env::var(env_key).ok().and_then(|s| s.parse::().ok()) @@ -429,6 +452,28 @@ impl<'a> ConfigResolver<'a> { value } + /// Resolve a u64. + pub fn get_u64(&self, key: &str, default: u64) -> u64 { + let mut value = default; + let mut src = ValueSource::Hardcoded; + + if let Some(v) = self.file_u64(key, false) { + value = v; + src = ValueSource::FileDefault; + } + if let Some(v) = self.file_u64(key, true) { + value = v; + src = ValueSource::FileRepo; + } + if let Some(v) = self.env_u64(key) { + value = v; + src = ValueSource::Env; + } + + self.log_decision(key, &value, src); + value + } + /// Resolve a bool. pub fn get_bool(&self, key: &str, default: bool) -> bool { let mut value = default; diff --git a/src/git.rs b/src/git.rs index 20f3879..5a3dbc2 100644 --- a/src/git.rs +++ b/src/git.rs @@ -91,10 +91,20 @@ pub fn git_output(args: &[&str]) -> Result { .with_context(|| format!("failed to run git {:?}", args))?; if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + let stderr = stderr.trim(); + if stderr.is_empty() { + return Err(anyhow!( + "git {:?} exited with status {:?}", + args, + output.status.code() + )); + } return Err(anyhow!( - "git {:?} exited with status {:?}", + "git {:?} exited with status {:?}: {}", args, - output.status.code() + output.status.code(), + stderr )); } @@ -137,8 +147,14 @@ pub fn staged_files() -> Result> { } /// Get per-file staged diff. +/// +/// `staged_files` returns paths relative to the repo root, but a bare pathspec +/// is resolved by git relative to the current working directory. Anchor it +/// with the `:/` top-level magic pathspec so this still works when commitbot +/// is invoked from a subdirectory of the repo. pub fn staged_diff_for_file(path: &str) -> Result { - let diff = git_output(&["diff", "--cached", "--", path])?; + let pathspec = format!(":/{path}"); + let diff = git_output(&["diff", "--cached", "--", &pathspec])?; Ok(diff) } diff --git a/src/llm/ollama.rs b/src/llm/ollama.rs index f0b71ba..5c9778e 100644 --- a/src/llm/ollama.rs +++ b/src/llm/ollama.rs @@ -2,10 +2,10 @@ use anyhow::{Result, anyhow}; use log; use musli::json; use musli::{Decode, Encode}; -use reqwest::StatusCode; use reqwest::blocking::Client; use std::io::BufReader; use std::sync::Mutex; +use std::time::Instant; use crate::FileChange; use crate::git::{PrItem, PrSummaryMode}; @@ -64,8 +64,9 @@ struct TokenUsage { } impl OllamaClient { - pub fn new(base_url: impl Into, model: impl Into, stream: bool) -> Self { + pub fn new(base_url: impl Into, model: impl Into, stream: bool, timeout_secs: u64) -> Self { let http = Client::builder() + .timeout(std::time::Duration::from_secs(timeout_secs)) .build() .expect("failed to build HTTP client"); Self { @@ -115,6 +116,7 @@ impl OllamaClient { let url = format!("{}/api/chat", self.base_url); + let t0 = Instant::now(); let resp = self .http .post(&url) @@ -127,13 +129,16 @@ impl OllamaClient { if stream { let reader = BufReader::new(resp); - return read_stream_to_string(reader, parse_stream_line); + let result = read_stream_to_string(reader, parse_stream_line); + log::debug!("Ollama response time: {:.2?}", t0.elapsed()); + return result; } let resp_text = resp .text() .map_err(|e| anyhow!("Failed to read Ollama response body: {e}"))?; + log::debug!("Ollama response time: {:.2?}", t0.elapsed()); log::trace!("Ollama raw JSON response: {resp_text}"); #[derive(Debug, Decode)] @@ -194,50 +199,9 @@ fn parse_stream_line(line: &str) -> Result> { impl LlmClient for OllamaClient { fn validate_model(&self) -> Result<()> { - let url = self.tags_url(); - let resp = self - .http - .get(&url) - .send() - .map_err(|e| anyhow!("Error calling Ollama at {url}: {e}"))?; - - if resp.status() != StatusCode::OK { - let status = resp.status(); - let body = resp.text().unwrap_or_default(); - return Err(anyhow!( - "Ollama model validation failed at {url}: HTTP {} - {}", - status.as_u16(), - body - )); - } - - let body = resp - .text() - .map_err(|e| anyhow!("Failed to read Ollama tags response from {url}: {e}"))?; - let parsed: OllamaTagsResponse = json::from_str(&body) - .map_err(|e| anyhow!("Failed to decode Ollama tags response from {url}: {e}"))?; - - if parsed.models.iter().any(|model| model.name == self.model) { - return Ok(()); - } - - let available = parsed - .models - .iter() - .map(|model| model.name.as_str()) - .collect::>() - .join(", "); - - Err(anyhow!( - "Model {:?} was not found at {}. Available models: {}", - self.model, - url, - if available.is_empty() { - "" - } else { - &available - } - )) + // Skipping model validation for Ollama; use a future "list models" command + // to query /api/tags if the user requests it. + Ok(()) } fn summarize_file( @@ -309,19 +273,19 @@ mod tests { #[test] fn trims_trailing_slash_in_tags_url() { - let client = OllamaClient::new("http://localhost:11434/", "qwen3-coder:30b", false); + let client = OllamaClient::new("http://localhost:11434/", "gemma4:31b", false, 90); assert_eq!(client.tags_url(), "http://localhost:11434/api/tags"); } #[test] fn decodes_ollama_tags_payload() { - let body = r#"{"models":[{"name":"qwen3-coder:30b"},{"name":"gpt-oss:20b"}]}"#; + let body = r#"{"models":[{"name":"gemma4:31b"},{"name":"qwen3-coder:30b"},{"name":"gpt-oss:20b"}]}"#; let parsed: OllamaTagsResponse = json::from_str(body).expect("valid tags payload"); assert!(parsed .models .iter() - .any(|model| model.name == "qwen3-coder:30b")); + .any(|model| model.name == "gemma4:31b")); assert!(!parsed .models .iter() diff --git a/src/llm/openai.rs b/src/llm/openai.rs index 91c383f..72d14d1 100644 --- a/src/llm/openai.rs +++ b/src/llm/openai.rs @@ -81,9 +81,9 @@ struct TokenUsage { } impl OpenAiClient { - pub fn new(api_key: String, model: String, api_base_url: String, stream: bool) -> Self { + pub fn new(api_key: String, model: String, api_base_url: String, stream: bool, timeout_secs: u64) -> Self { let client = Client::builder() - .timeout(Duration::from_secs(90)) + .timeout(Duration::from_secs(timeout_secs)) .build() .expect("failed to build HTTP client"); @@ -122,6 +122,7 @@ impl OpenAiClient { log::info!("Calling OpenAI model {:?}", &req.model); + let t0 = std::time::Instant::now(); let resp = self .client .post(url) @@ -141,6 +142,7 @@ impl OpenAiClient { } let chat_resp: ChatResponse = resp.json().context("failed to parse OpenAI response")?; + log::debug!("OpenAI response time: {:.2?}", t0.elapsed()); let content = chat_resp .choices .first() @@ -164,6 +166,7 @@ impl OpenAiClient { log::info!("Streaming OpenAI model {:?}", &req.model); + let t0 = std::time::Instant::now(); let resp = self .client .post(url) @@ -183,7 +186,9 @@ impl OpenAiClient { } let reader = BufReader::new(resp); - read_stream_to_string(reader, parse_stream_line) + let result = read_stream_to_string(reader, parse_stream_line); + log::debug!("OpenAI streaming response time: {:.2?}", t0.elapsed()); + result } } @@ -401,6 +406,7 @@ mod tests { "gpt-5-nano".into(), "https://api.openai.com".into(), false, + 90, ); assert_eq!( @@ -416,6 +422,7 @@ mod tests { "gpt-5-nano".into(), "https://api.openai.com/v1".into(), false, + 90, ); assert_eq!( diff --git a/src/main.rs b/src/main.rs index 32142bd..714085e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -188,6 +188,7 @@ fn summarize_files_concurrently( scope.spawn(move || { log::debug!("Summarizing file: {}", path); + let t0 = std::time::Instant::now(); let res = (|| -> Result { let fc = FileChange { path, @@ -205,6 +206,7 @@ fn summarize_files_concurrently( )?; Ok(summary) })(); + let elapsed = t0.elapsed(); pb.inc(1); @@ -212,10 +214,10 @@ fn summarize_files_concurrently( match &res { Ok(summary) => { let snippet = preview_snippet(summary); - line.finish_with_message(dimmed(&snippet)); + line.finish_with_message(dimmed(&format!("{snippet} ({elapsed:.2?})"))); } Err(err) => { - line.finish_with_message(dimmed(&format!("error: {err}"))); + line.finish_with_message(dimmed(&format!("error: {err} ({elapsed:.2?})"))); } } } @@ -384,12 +386,15 @@ fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { if cfg.stream { let _msg = llm.generate_commit_message(&branch, &file_changes, ticket_summary.as_deref())?; } else { + let t0 = std::time::Instant::now(); let msg = llm.generate_commit_message(&branch, &file_changes, ticket_summary.as_deref())?; + let elapsed = t0.elapsed(); if msg.ends_with('\n') { print!("{msg}"); } else { println!("{msg}"); } + println!("Commit message generated in {elapsed:.2?}"); } println!(); @@ -518,12 +523,15 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { if cfg.stream { let _msg = llm.generate_commit_message(&branch, &file_changes, ticket_summary.as_deref())?; } else { + let t0 = std::time::Instant::now(); let msg = llm.generate_commit_message(&branch, &file_changes, ticket_summary.as_deref())?; + let elapsed = t0.elapsed(); if msg.ends_with('\n') { print!("{msg}"); } else { println!("{msg}"); } + println!("Commit message generated in {elapsed:.2?}"); } println!(); @@ -532,8 +540,8 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { } Ok(()) -} +} fn run_pr( cli: &Cli, cfg: &Config, @@ -583,20 +591,24 @@ fn run_pr( let ticket_summary = resolved_ticket_summary(cli); let _pr_message = if cfg.stream { println!(); + let t0 = std::time::Instant::now(); let msg = llm.generate_pr_message(base, &from_branch, mode, &items, ticket_summary.as_deref())?; - println!(); + println!("\nPR message generated in {:.2?}", t0.elapsed()); msg } else { println!(); + let t0 = std::time::Instant::now(); let msg = llm.generate_pr_message(base, &from_branch, mode, &items, ticket_summary.as_deref())?; + let elapsed = t0.elapsed(); if msg.ends_with('\n') { print!("{msg}"); } else { println!("{msg}"); } + println!("PR message generated in {elapsed:.2?}"); msg }; diff --git a/src/setup.rs b/src/setup.rs index 3e2bd3b..62444e7 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -18,9 +18,10 @@ pub fn build_llm_client(cfg: &Config) -> Result> { .unwrap_or_else(|| "https://api.openai.com".to_string()); log::debug!( - "Using OpenAiClient with model: {} (stream={})", + "Using OpenAiClient with model: {} (stream={}, timeout={}s)", cfg.model, - cfg.stream + cfg.stream, + cfg.request_timeout_secs ); Ok(Box::new(OpenAiClient::new( @@ -28,6 +29,7 @@ pub fn build_llm_client(cfg: &Config) -> Result> { cfg.model.clone(), base_url, cfg.stream, + cfg.request_timeout_secs, ))) } "ollama" => { @@ -37,15 +39,17 @@ pub fn build_llm_client(cfg: &Config) -> Result> { .unwrap_or_else(|| "http://localhost:11434".to_string()); log::debug!( - "Using OllamaClient with model: {} (stream={})", + "Using OllamaClient with model: {} (stream={}, timeout={}s)", cfg.model, - cfg.stream + cfg.stream, + cfg.request_timeout_secs ); Ok(Box::new(OllamaClient::new( base_url, cfg.model.clone(), cfg.stream, + cfg.request_timeout_secs, ))) } other => Err(anyhow!("Unknown provider: {}", other)), diff --git a/tests/git.rs b/tests/git.rs index 2589c68..9f41680 100644 --- a/tests/git.rs +++ b/tests/git.rs @@ -1,7 +1,41 @@ use commitbot::git::{ find_first_pr_number, format_pr_commit_appendix_with_remote, parse_remote_repo, - short_commit_hash, split_diff_by_file, PrItem, PrSummaryMode, + short_commit_hash, split_diff_by_file, staged_diff_for_file, staged_files, PrItem, + PrSummaryMode, }; +use std::process::Command; + +/// Set up a throwaway git repo with a staged change in a nested file, and +/// return its tempdir handle plus the nested directory's path. +fn repo_with_staged_nested_file() -> (tempfile::TempDir, std::path::PathBuf) { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path(); + + let run = |args: &[&str]| { + let status = Command::new("git") + .args(args) + .current_dir(root) + .status() + .expect("run git"); + assert!(status.success(), "git {:?} failed", args); + }; + + run(&["init", "-q"]); + run(&["config", "user.email", "test@example.com"]); + run(&["config", "user.name", "Test"]); + + let nested_dir = root.join("app").join("Models"); + std::fs::create_dir_all(&nested_dir).expect("mkdir"); + let file = nested_dir.join("OrderItem.php"); + std::fs::write(&file, "original\n").expect("write"); + run(&["add", "."]); + run(&["commit", "-q", "-m", "initial"]); + + std::fs::write(&file, "original\nchanged\n").expect("rewrite"); + run(&["add", "."]); + + (dir, nested_dir) +} #[test] fn parses_github_ssh_remote() { @@ -136,6 +170,29 @@ fn short_commit_hash_short_input() { assert_eq!(result, "abc"); } +#[test] +fn staged_diff_for_file_works_from_subdirectory() { + let (_dir, nested_dir) = repo_with_staged_nested_file(); + + let original_cwd = std::env::current_dir().expect("current dir"); + std::env::set_current_dir(&nested_dir).expect("chdir into nested dir"); + + let result = (|| { + let files = staged_files()?; + assert_eq!(files, vec!["app/Models/OrderItem.php".to_string()]); + + let diff = staged_diff_for_file(&files[0])?; + assert!( + diff.contains("+changed"), + "expected diff to contain the staged change, got: {diff:?}" + ); + anyhow::Ok(()) + })(); + + std::env::set_current_dir(original_cwd).expect("restore cwd"); + result.expect("staged diff lookup from subdirectory"); +} + #[test] fn pr_summary_mode_as_str() { assert_eq!(PrSummaryMode::ByCommits.as_str(), "commits");