diff --git a/README.md b/README.md index eb499b5..874aa8a 100644 --- a/README.md +++ b/README.md @@ -84,10 +84,19 @@ commitbot --diff my-changes.diff # With a custom branch name for context commitbot --diff my-changes.diff --branch feature/ISSUE-123-auth -# From stdin (pipe a diff) +# From stdin (pipe a diff)task : git diff HEAD~3 | commitbot --diff - + +# From a commit hash (looked up in git history automatically) +commitbot --diff aabcf3b6ce03e4f1503c7a0fdda7c120bf73c8bc ``` +When `--diff` looks like a commit hash, Commitbot first checks whether it +exists in the current repo's history and, if so, summarizes the diff +introduced by that commit. If it doesn't match a commit, Commitbot falls +back to treating the value as a file path; if neither a matching commit nor +a file is found, it reports an error instead of guessing. + --- ### Pull Request Summaries diff --git a/commitbot.toml b/commitbot.toml index a2e3aaf..e817015 100644 --- a/commitbot.toml +++ b/commitbot.toml @@ -25,7 +25,8 @@ max_concurrent_requests = 8 ["MikeGarde/commitbot"] provider = "lmstudio" model = "google/gemma-4-12b-qat" -url = "http://GPU.localdomain:1234/v1" +url = "http://gpu.garde.one:1234/v1" +# Fallback / direct IP: url = "http://192.168.1.16:1234/v1" max_concurrent_requests = 2 diff --git a/src/config.rs b/src/config.rs index 3440d8d..3fdb6dd 100644 --- a/src/config.rs +++ b/src/config.rs @@ -24,6 +24,10 @@ pub struct Config { pub stream: bool, /// HTTP request timeout in seconds for LLM calls pub request_timeout_secs: u64, + /// Maximum size (in bytes) of a single file's diff sent to the LLM. + /// Larger diffs (e.g. notebooks with embedded base64 output, minified + /// bundles) are truncated to avoid exceeding the model's context window. + pub max_diff_bytes: usize, } impl Config { @@ -56,6 +60,7 @@ impl Config { let max_concurrent_requests = r.get_usize("max_concurrent_requests", 4); let stream = r.get_bool("stream", true); let request_timeout_secs = r.get_u64("request_timeout_secs", 300); + let max_diff_bytes = r.get_usize("max_diff_bytes", 20_000); // Cleanup: trim stray quotes if any upstream included them let provider = provider.trim_matches('"').to_string(); @@ -66,7 +71,12 @@ impl Config { .map(|s| s.trim_matches('"').trim().to_string()) .filter(|s| !s.is_empty()); - if provider == "openai" && openai_api_key.is_none() { + let is_custom_endpoint = base_url + .as_deref() + .map(|u| !u.starts_with("https://api.openai.com")) + .unwrap_or(false); + + if provider == "openai" && openai_api_key.is_none() && !is_custom_endpoint { return Err(anyhow!( "OPENAI_API_KEY must be set via CLI, env var, or config file for provider=openai" )); @@ -80,6 +90,7 @@ impl Config { max_concurrent_requests, stream, request_timeout_secs, + max_diff_bytes, }) } } @@ -94,6 +105,7 @@ struct FileConfig { pub max_concurrent_requests: Option, pub stream: Option, pub request_timeout_secs: Option, + pub max_diff_bytes: Option, } /// Root of the TOML file: @@ -178,6 +190,7 @@ impl<'a> ConfigResolver<'a> { "max_concurrent_requests" => Some("COMMITBOT_MAX_CONCURRENT_REQUESTS"), "stream" => Some("COMMITBOT_STREAM"), "request_timeout_secs" => Some("COMMITBOT_REQUEST_TIMEOUT_SECS"), + "max_diff_bytes" => Some("COMMITBOT_MAX_DIFF_BYTES"), _ => None, } } @@ -207,6 +220,7 @@ impl<'a> ConfigResolver<'a> { }; match key { "max_concurrent_requests" => cfg.max_concurrent_requests, + "max_diff_bytes" => cfg.max_diff_bytes, _ => None, } } diff --git a/src/git.rs b/src/git.rs index 5a3dbc2..079d86e 100644 --- a/src/git.rs +++ b/src/git.rs @@ -83,7 +83,25 @@ impl RemoteRepo { } } -/// Run a git command and capture stdout as String. +pub fn format_git_error(args: &[&str], code: Option, stderr: &str) -> anyhow::Error { + let stderr = stderr.trim(); + + if stderr.contains("xcodebuild -license") || stderr.contains("Xcode license") { + return anyhow!( + "Xcode license agreement required.\n\ + Apple's git cannot run until the Xcode / Command Line Tools license is accepted.\n\n\ + Please run:\n sudo xcodebuild -license\n\n\ + Once accepted, run commitbot again." + ); + } + + if stderr.is_empty() { + anyhow!("git {:?} exited with status {:?}", args, code) + } else { + anyhow!("git {:?} exited with status {:?}: {}", args, code, stderr) + } +} + pub fn git_output(args: &[&str]) -> Result { let output = GitCommand::new("git") .args(args) @@ -92,20 +110,7 @@ pub fn git_output(args: &[&str]) -> Result { 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 {:?}: {}", - args, - output.status.code(), - stderr - )); + return Err(format_git_error(args, output.status.code(), &stderr)); } Ok(String::from_utf8_lossy(&output.stdout).to_string()) @@ -127,7 +132,6 @@ fn remote_origin_url() -> Option { .filter(|url| !url.is_empty()) } -/// Get the current branch name. pub fn current_branch() -> Result { let name = git_output(&["rev-parse", "--abbrev-ref", "HEAD"])? .trim() @@ -135,7 +139,25 @@ pub fn current_branch() -> Result { Ok(name) } -/// Get a list of staged files. +pub fn looks_like_commit_hash(s: &str) -> bool { + let len = s.len(); + (4..=40).contains(&len) && s.chars().all(|c| c.is_ascii_hexdigit()) +} + +pub fn resolve_commit_diff(hash: &str) -> Result> { + let verified = GitCommand::new("git") + .args(["rev-parse", "--verify", "--quiet", &format!("{hash}^{{commit}}")]) + .output() + .with_context(|| format!("failed to run git rev-parse for '{hash}'"))?; + + if !verified.status.success() { + return Ok(None); + } + + let diff = git_output(&["show", "--format=", hash])?; + Ok(Some(diff)) +} + pub fn staged_files() -> Result> { let output = git_output(&["diff", "--cached", "--name-only"])?; let files = output diff --git a/src/lib.rs b/src/lib.rs index a525a10..e701e1d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -73,6 +73,35 @@ impl FileCategory { } } +/// Truncates an oversized diff before it's ever sent to an LLM. +/// +/// Some diffs (Jupyter notebooks with embedded base64 image outputs, minified +/// bundles, generated files) can run into the megabytes, which blows past any +/// local model's context window and can wedge the upstream server for every +/// other in-flight request. Cutting on a line boundary keeps the remaining +/// diff readable instead of ending mid-line. +pub fn truncate_diff(diff: &str, max_bytes: usize) -> String { + if diff.len() <= max_bytes { + return diff.to_string(); + } + + let mut cut = max_bytes; + while cut > 0 && !diff.is_char_boundary(cut) { + cut -= 1; + } + + let head = match diff[..cut].rfind('\n') { + Some(idx) => &diff[..idx], + None => &diff[..cut], + }; + + let omitted = diff.len() - head.len(); + format!( + "{head}\n... [diff truncated, {omitted} bytes omitted — file is unusually large, \ + likely generated or contains embedded binary/base64 content]" + ) +} + /// Represents a single staged file's change and metadata. #[derive(Debug, Clone)] pub struct FileChange { @@ -86,3 +115,24 @@ pub struct FileChange { /// [`FileCategory::Lock`] and [`FileCategory::Ignored`] files. pub summary: Option, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn truncate_diff_leaves_small_diffs_untouched() { + let diff = "diff --git a/f b/f\n+hello\n"; + assert_eq!(truncate_diff(diff, 1_000), diff); + } + + #[test] + fn truncate_diff_cuts_oversized_diffs_on_a_line_boundary() { + let diff = "line one\nline two\nline three\n"; + let result = truncate_diff(diff, 15); + + assert!(result.starts_with("line one\n")); + assert!(!result.contains("line three")); + assert!(result.contains("truncated")); + } +} diff --git a/src/llm/openai.rs b/src/llm/openai.rs index f4bc392..6007a19 100644 --- a/src/llm/openai.rs +++ b/src/llm/openai.rs @@ -27,6 +27,7 @@ struct ChatMessage { #[derive(Deserialize)] struct ChatResponse { + #[serde(default)] choices: Vec, usage: Option, } @@ -38,18 +39,25 @@ struct ChatChoice { #[derive(Deserialize)] struct ChatMessageResponse { - content: String, + #[serde(default)] + content: Option, + #[serde(default)] + reasoning_content: Option, } -#[derive(Deserialize)] +#[derive(Deserialize, Default)] struct ChatUsage { + #[serde(default)] prompt_tokens: u32, + #[serde(default)] completion_tokens: u32, + #[serde(default)] total_tokens: u32, } #[derive(Deserialize)] struct ModelListResponse { + #[serde(default)] data: Vec, } @@ -60,6 +68,7 @@ struct ModelListEntry { #[derive(Deserialize)] struct StreamResponse { + #[serde(default)] choices: Vec, } @@ -70,7 +79,11 @@ struct StreamChoice { #[derive(Deserialize)] struct StreamDelta { + #[serde(default)] content: Option, + #[serde(default)] + #[allow(dead_code)] + reasoning_content: Option, } /// How to confirm the configured model exists on the upstream server. @@ -138,7 +151,7 @@ impl OpenAiClient { client, api_key, model, - api_base_url: api_base_url.trim_end_matches('/').to_string(), + api_base_url: normalize_base_url(&api_base_url), stream, provider_label: provider_label.into(), model_validation, @@ -155,13 +168,9 @@ impl OpenAiClient { } } - /// Join `path` under `/v1`, tolerating a base URL that already ends in `/v1`. + /// Join `path` under `/v1`, with base URL guaranteed normalized. fn v1_url(&self, path: &str) -> String { - if self.api_base_url.ends_with("/v1") { - format!("{}/{}", self.api_base_url, path) - } else { - format!("{}/v1/{}", self.api_base_url, path) - } + format!("{}/v1/{}", self.api_base_url, path.trim_start_matches('/')) } fn chat_url(&self) -> String { @@ -210,8 +219,19 @@ impl OpenAiClient { let content = chat_resp .choices .first() - .map(|c| c.message.content.clone()) - .ok_or_else(|| anyhow!("no choices returned from {}", self.provider_label))?; + .and_then(|c| { + c.message + .content + .clone() + .filter(|s| !s.trim().is_empty()) + .or_else(|| { + c.message + .reasoning_content + .clone() + .filter(|s| !s.trim().is_empty()) + }) + }) + .ok_or_else(|| anyhow!("no content returned from {}", self.provider_label))?; if let Some(usage) = &chat_resp.usage { // Recover from a poisoned mutex instead of panicking so the CLI @@ -318,11 +338,30 @@ impl OpenAiClient { )); } - let listed: ModelListResponse = resp - .json() - .with_context(|| format!("failed to parse model list from {url}"))?; + let listed: ModelListResponse = match resp.json() { + Ok(l) => l, + Err(e) => { + log::warn!( + "{} model listing at {} could not be parsed: {}. Proceeding anyway...", + self.provider_label, + url, + e + ); + return Ok(()); + } + }; - if listed.data.iter().any(|m| m.id == self.model) { + if listed.data.is_empty() { + log::warn!( + "{} at {} returned an empty model list; proceeding with {:?}", + self.provider_label, + url, + self.model + ); + return Ok(()); + } + + if listed.data.iter().any(|m| matches_model_id(&self.model, &m.id)) { return Ok(()); } @@ -347,6 +386,60 @@ impl OpenAiClient { } } +/// Normalizes any variation of base URL (e.g. `http://host:1234`, `http://host:1234/v1`, +/// `http://host:1234/V1/`, `http://host:1234/api/v1`, `http://host:1234/v1/chat/completions`) +/// down to the base origin so that `v1_url` can deterministically append `/v1/{path}`. +pub fn normalize_base_url(raw: &str) -> String { + let mut url = raw.trim().trim_end_matches('/').to_string(); + + if url.to_ascii_lowercase().ends_with("/chat/completions") { + url = url[..url.len() - "/chat/completions".len()] + .trim_end_matches('/') + .to_string(); + } else if url.to_ascii_lowercase().ends_with("/models") { + url = url[..url.len() - "/models".len()] + .trim_end_matches('/') + .to_string(); + } + + if url.to_ascii_lowercase().ends_with("/api/v1") { + url = url[..url.len() - "/api/v1".len()] + .trim_end_matches('/') + .to_string(); + } else if url.to_ascii_lowercase().ends_with("/v1") { + url = url[..url.len() - "/v1".len()] + .trim_end_matches('/') + .to_string(); + } + + url +} + +fn clean_model_name(s: &str) -> &str { + let without_gguf = s.strip_suffix(".gguf").unwrap_or(s); + without_gguf.split(':').next().unwrap_or(without_gguf) +} + +/// Check if a model candidate string from the server matches the configured model name. +/// Handles exact match, case-insensitivity, repo prefixes (e.g. "google/gemma-4-12b-qat" vs "gemma-4-12b-qat"), +/// and stripping file extensions like ".gguf" or tags like ":latest". +pub fn matches_model_id(configured: &str, candidate: &str) -> bool { + if configured == candidate { + return true; + } + if configured.eq_ignore_ascii_case(candidate) { + return true; + } + let conf_base = configured.rsplit('/').next().unwrap_or(configured); + let cand_base = candidate.rsplit('/').next().unwrap_or(candidate); + if conf_base.eq_ignore_ascii_case(cand_base) { + return true; + } + let conf_clean = clean_model_name(conf_base); + let cand_clean = clean_model_name(cand_base); + conf_clean.eq_ignore_ascii_case(cand_clean) +} + fn parse_stream_line(line: &str) -> Result> { let line = line.trim_start(); if !line.starts_with("data:") { @@ -368,8 +461,40 @@ fn parse_stream_line(line: &str) -> Result> { impl LlmClient for OpenAiClient { fn validate_model(&self) -> Result<()> { match self.model_validation { - ModelValidation::Retrieve => self.validate_model_by_retrieve(), - ModelValidation::List => self.validate_model_by_list(), + ModelValidation::Retrieve => match self.validate_model_by_retrieve() { + Ok(()) => Ok(()), + Err(e) => { + log::debug!( + "Retrieve model validation failed: {e}. Falling back to list validation." + ); + if let Ok(()) = self.validate_model_by_list() { + return Ok(()); + } + if self.provider_label != "OpenAI" { + log::warn!( + "Could not validate model {:?} on {}: {}. Proceeding anyway...", + self.model, + self.provider_label, + e + ); + Ok(()) + } else { + Err(e) + } + } + }, + ModelValidation::List => match self.validate_model_by_list() { + Ok(()) => Ok(()), + Err(e) => { + log::debug!( + "List model validation failed: {e}. Falling back to retrieve validation." + ); + if let Ok(()) = self.validate_model_by_retrieve() { + return Ok(()); + } + Err(e) + } + }, } } @@ -620,6 +745,120 @@ mod tests { ); } + #[test] + fn builds_lm_studio_urls_from_uppercase_v1_base() { + let client = lm_studio("http://gpu.garde.one:1234/V1/"); + + assert_eq!(client.models_url(), "http://gpu.garde.one:1234/v1/models"); + assert_eq!( + client.chat_url(), + "http://gpu.garde.one:1234/v1/chat/completions" + ); + } + + #[test] + fn builds_lm_studio_urls_from_api_v1_base() { + let client = lm_studio("http://localhost:1234/api/v1"); + + assert_eq!(client.models_url(), "http://localhost:1234/v1/models"); + assert_eq!( + client.chat_url(), + "http://localhost:1234/v1/chat/completions" + ); + } + + #[test] + fn builds_lm_studio_urls_from_full_endpoint() { + let client = lm_studio("http://localhost:1234/v1/chat/completions"); + + assert_eq!(client.models_url(), "http://localhost:1234/v1/models"); + assert_eq!( + client.chat_url(), + "http://localhost:1234/v1/chat/completions" + ); + } + + #[test] + fn matches_model_id_variations() { + assert!(matches_model_id( + "google/gemma-4-12b-qat", + "google/gemma-4-12b-qat" + )); + assert!(matches_model_id( + "google/gemma-4-12b-qat", + "GOOGLE/GEMMA-4-12B-QAT" + )); + assert!(matches_model_id( + "gemma-4-12b-qat", + "google/gemma-4-12b-qat" + )); + assert!(matches_model_id( + "google/gemma-4-12b-qat", + "gemma-4-12b-qat" + )); + assert!(matches_model_id( + "gemma-4-12b-qat", + "google/gemma-4-12b-qat.gguf" + )); + assert!(matches_model_id( + "gemma-4-12b-qat", + "google/gemma-4-12b-qat:latest" + )); + assert!(!matches_model_id( + "gemma-4-12b-qat", + "ibm/granite-4-h-tiny" + )); + } + + #[test] + fn decodes_chat_response_with_null_content_and_reasoning() { + let body = r#"{ + "id": "chatcmpl-test", + "object": "chat.completion", + "created": 123456, + "model": "google/gemma-4-12b-qat", + "choices": [ + { + "index": 0, + "message": { + "role": "assistant", + "content": null, + "reasoning_content": "Just thinking..." + }, + "finish_reason": "stop" + } + ], + "usage": { + "prompt_tokens": 10, + "completion_tokens": 5, + "total_tokens": 15 + } + }"#; + + let parsed: ChatResponse = serde_json::from_str(body).expect("valid chat response"); + let choice = parsed.choices.first().expect("choice present"); + assert_eq!(choice.message.content, None); + assert_eq!( + choice.message.reasoning_content.as_deref(), + Some("Just thinking...") + ); + } + + #[test] + fn decodes_streaming_chunk_with_reasoning_and_no_content() { + let line = r#"data: {"id":"c","choices":[{"index":0,"delta":{"reasoning_content":"thought"}}]}"#; + let res = parse_stream_line(line).expect("valid stream line"); + assert_eq!(res, None); + + let line_content = r#"data: {"id":"c","choices":[{"index":0,"delta":{"content":"word"}}]}"#; + let res_content = parse_stream_line(line_content).expect("valid stream line"); + assert_eq!(res_content, Some("word".to_string())); + + let line_done = "data: [DONE]"; + let res_done = parse_stream_line(line_done).expect("valid stream line"); + assert_eq!(res_done, None); + } + #[test] fn decodes_model_list_payload() { let body = r#"{"object":"list","data":[ diff --git a/src/main.rs b/src/main.rs index 754c00f..4dffc2b 100644 --- a/src/main.rs +++ b/src/main.rs @@ -2,8 +2,8 @@ use anyhow::{anyhow, Result}; use clap::Parser; use commitbot::config::Config; use commitbot::git::{ - collect_pr_items, current_branch, format_pr_commit_appendix, split_diff_by_file, - staged_diff_for_file, staged_files, PrSummaryMode, + collect_pr_items, current_branch, format_pr_commit_appendix, looks_like_commit_hash, + resolve_commit_diff, split_diff_by_file, staged_diff_for_file, staged_files, PrSummaryMode, }; use commitbot::llm::LlmClient; use commitbot::{Cli, Command, FileCategory, FileChange}; @@ -256,29 +256,64 @@ fn summarize_files_concurrently( Ok(()) } +/// Read the content behind `--diff `. +/// +/// `-` reads stdin. Otherwise, if the argument looks like a commit hash, we +/// first try to resolve it against git history; if that doesn't find a +/// commit, or the argument didn't look like a hash to begin with, we fall +/// back to treating it as a file path. +fn read_diff_arg(diff_arg: &str) -> Result { + if diff_arg == "-" { + let mut buf = String::new(); + io::stdin().read_to_string(&mut buf)?; + return Ok(buf); + } + + if looks_like_commit_hash(diff_arg) && let Some(diff) = resolve_commit_diff(diff_arg)? { + return Ok(diff); + } + + let path = std::path::Path::new(diff_arg); + if path.is_file() { + std::fs::read_to_string(path) + .map_err(|e| anyhow!("Failed to read diff file '{}': {}", diff_arg, e)) + } else { + Err(anyhow!( + "'{}' is not a known commit hash or an existing file.", + diff_arg + )) + } +} + +/// Branch name paired with a list of (file path, diff) pairs. +type BranchDiffs = (String, Vec<(String, String)>); + +/// Load per-file diffs from `--diff `, splitting a combined diff +/// into (path, diff) pairs. Returns `None` when the resolved content is +/// empty (already reported to the user). +fn load_external_diff(cli: &Cli, diff_arg: &str) -> Result> { + let combined = read_diff_arg(diff_arg)?; + if combined.trim().is_empty() { + println!("No diff content found."); + return Ok(None); + } + let mut per_file = split_diff_by_file(&combined); + if per_file.is_empty() { + per_file = vec![("(diff)".to_string(), combined)]; + } + let branch = cli + .branch + .clone() + .unwrap_or_else(|| current_branch().unwrap_or_else(|_| "unknown-branch".to_string())); + Ok(Some((branch, per_file))) +} + fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { let (branch, file_pairs) = if let Some(ref diff_arg) = cli.diff { - let combined = if diff_arg == "-" { - let mut buf = String::new(); - io::stdin().read_to_string(&mut buf)?; - buf - } else { - std::fs::read_to_string(diff_arg) - .map_err(|e| anyhow!("Failed to read diff file '{}': {}", diff_arg, e))? - }; - if combined.trim().is_empty() { - println!("No diff content found."); - return Ok(()); - } - let mut per_file = split_diff_by_file(&combined); - if per_file.is_empty() { - per_file = vec![("(diff)".to_string(), combined)]; + match load_external_diff(cli, diff_arg)? { + Some(pair) => pair, + None => return Ok(()), } - let branch = cli - .branch - .clone() - .unwrap_or_else(|| current_branch().unwrap_or_else(|_| "unknown-branch".to_string())); - (branch, per_file) } else { let branch = current_branch()?; let files = staged_files()?; @@ -314,7 +349,7 @@ fn run_interactive(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { file_changes.push(FileChange { path, category, - diff, + diff: commitbot::truncate_diff(&diff, cfg.max_diff_bytes), summary: None, }); } @@ -424,28 +459,10 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { let using_external_diff = cli.diff.is_some(); let (branch, file_pairs): (String, Vec<(String, String)>) = if let Some(ref diff_arg) = cli.diff { - let combined = if diff_arg == "-" { - let mut buf = String::new(); - io::stdin().read_to_string(&mut buf)?; - buf - } else { - std::fs::read_to_string(diff_arg) - .map_err(|e| anyhow!("Failed to read diff file '{}': {}", diff_arg, e))? - }; - - if combined.trim().is_empty() { - println!("No diff content found."); - return Ok(()); - } - - let mut per_file = split_diff_by_file(&combined); - if per_file.is_empty() { - per_file = vec![("(diff)".to_string(), combined)]; + match load_external_diff(cli, diff_arg)? { + Some(pair) => pair, + None => return Ok(()), } - let branch = cli.branch.clone().unwrap_or_else(|| { - current_branch().unwrap_or_else(|_| "unknown-branch".to_string()) - }); - (branch, per_file) } else { let branch = current_branch()?; let files = staged_files()?; @@ -474,7 +491,7 @@ fn run_auto(cli: &Cli, cfg: &Config, llm: &dyn LlmClient) -> Result<()> { FileChange { path, category, - diff, + diff: commitbot::truncate_diff(&diff, cfg.max_diff_bytes), summary: None, } }) diff --git a/src/setup.rs b/src/setup.rs index b80965b..85990ab 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -8,15 +8,18 @@ use crate::llm::openai::{ModelValidation, OpenAiClient}; pub fn build_llm_client(cfg: &Config) -> Result> { match cfg.provider.as_str() { "openai" => { - let key = cfg - .openai_api_key - .clone() - .ok_or_else(|| anyhow!("OPENAI_API_KEY must be set for provider=openai"))?; let base_url = cfg .base_url .clone() .unwrap_or_else(|| "https://api.openai.com".to_string()); + let is_custom_endpoint = !base_url.starts_with("https://api.openai.com"); + let validation = if is_custom_endpoint { + ModelValidation::List + } else { + ModelValidation::Retrieve + }; + log::debug!( "Using OpenAiClient with model: {} (stream={}, timeout={}s)", cfg.model, @@ -24,12 +27,14 @@ pub fn build_llm_client(cfg: &Config) -> Result> { cfg.request_timeout_secs ); - Ok(Box::new(OpenAiClient::new( - key, + Ok(Box::new(OpenAiClient::openai_compatible( + cfg.openai_api_key.clone(), cfg.model.clone(), base_url, cfg.stream, cfg.request_timeout_secs, + "OpenAI", + validation, ))) } "ollama" => { diff --git a/tests/config.rs b/tests/config.rs index f072860..b6c8de5 100644 --- a/tests/config.rs +++ b/tests/config.rs @@ -190,3 +190,29 @@ model = "gpt-5-nano" fs::remove_file(config_path).ok(); } + +#[test] +fn custom_openai_endpoint_needs_no_api_key() { + let config_path = write_temp_config( + "custom_openai", + r#" +[default] +provider = "openai" +model = "local-model" +url = "http://192.168.1.16:1234/v1" +"#, + ); + + let cli = Cli::parse_from([ + "commitbot", + "--config", + config_path.to_str().expect("utf-8 path"), + ]); + + let cfg = Config::from_sources(&cli).expect("custom openai endpoint should allow no api key"); + assert_eq!(cfg.provider, "openai"); + assert_eq!(cfg.openai_api_key, None); + assert_eq!(cfg.base_url.as_deref(), Some("http://192.168.1.16:1234/v1")); + + fs::remove_file(config_path).ok(); +} diff --git a/tests/git.rs b/tests/git.rs index 9f41680..4b2ef63 100644 --- a/tests/git.rs +++ b/tests/git.rs @@ -1,9 +1,14 @@ use commitbot::git::{ - find_first_pr_number, format_pr_commit_appendix_with_remote, parse_remote_repo, - short_commit_hash, split_diff_by_file, staged_diff_for_file, staged_files, PrItem, - PrSummaryMode, + find_first_pr_number, format_git_error, format_pr_commit_appendix_with_remote, + looks_like_commit_hash, parse_remote_repo, resolve_commit_diff, short_commit_hash, + split_diff_by_file, staged_diff_for_file, staged_files, PrItem, PrSummaryMode, }; use std::process::Command; +use std::sync::Mutex; + +/// `std::env::set_current_dir` changes process-wide state, so tests that use +/// it must not run concurrently with each other. +static CWD_LOCK: Mutex<()> = Mutex::new(()); /// 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. @@ -12,12 +17,16 @@ fn repo_with_staged_nested_file() -> (tempfile::TempDir, std::path::PathBuf) { let root = dir.path(); let run = |args: &[&str]| { - let status = Command::new("git") + let output = Command::new("git") .args(args) .current_dir(root) - .status() + .output() .expect("run git"); - assert!(status.success(), "git {:?} failed", args); + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + let err = format_git_error(args, output.status.code(), &stderr); + panic!("{err}"); + } }; run(&["init", "-q"]); @@ -172,6 +181,7 @@ fn short_commit_hash_short_input() { #[test] fn staged_diff_for_file_works_from_subdirectory() { + let _guard = CWD_LOCK.lock().expect("cwd lock"); let (_dir, nested_dir) = repo_with_staged_nested_file(); let original_cwd = std::env::current_dir().expect("current dir"); @@ -198,3 +208,89 @@ fn pr_summary_mode_as_str() { assert_eq!(PrSummaryMode::ByCommits.as_str(), "commits"); assert_eq!(PrSummaryMode::ByPrs.as_str(), "prs"); } + +#[test] +fn format_git_error_detects_xcode_license_error() { + let stderr = "You have not agreed to the Xcode license agreements. Please run 'sudo xcodebuild -license' from within a Terminal window to review and agree to the Xcode and Apple SDKs license."; + let err = format_git_error(&["rev-parse", "--abbrev-ref", "HEAD"], Some(69), stderr); + let msg = err.to_string(); + assert!(msg.contains("Xcode license agreement required")); + assert!(msg.contains("sudo xcodebuild -license")); +} + +#[test] +fn format_git_error_legacy_xcode_license_error() { + let stderr = "Agreeing to the Xcode/iOS license requires admin privileges, please run 'sudo xcodebuild -license' and then retry this command."; + let err = format_git_error(&["diff", "--cached"], Some(69), stderr); + let msg = err.to_string(); + assert!(msg.contains("Xcode license agreement required")); + assert!(msg.contains("sudo xcodebuild -license")); +} + +#[test] +fn format_git_error_standard_git_error() { + let stderr = "fatal: not a git repository (or any of the parent directories): .git"; + let err = format_git_error(&["status"], Some(128), stderr); + let msg = err.to_string(); + assert!(msg.contains("git [\"status\"] exited with status Some(128)")); + assert!(msg.contains("fatal: not a git repository")); +} + +#[test] +fn looks_like_commit_hash_accepts_hex_strings_in_range() { + assert!(looks_like_commit_hash("a5484b6")); + assert!(looks_like_commit_hash("a5484b6ce03e4f1503c7a0fdda7c120bf73c8bc")); + assert!(looks_like_commit_hash("dead")); +} + +#[test] +fn looks_like_commit_hash_rejects_non_hex_or_bad_length() { + assert!(!looks_like_commit_hash("abc")); // too short + assert!(!looks_like_commit_hash("my-changes.diff")); // not hex + assert!(!looks_like_commit_hash("-")); // stdin marker + assert!(!looks_like_commit_hash( + "a5484b6ce03e4f1503c7a0fdda7c120bf73c8bcaa" // too long + )); +} + +#[test] +fn resolve_commit_diff_finds_existing_commit() { + let _guard = CWD_LOCK.lock().expect("cwd lock"); + 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 hash = commitbot::git::git_output(&["rev-parse", "HEAD"])? + .trim() + .to_string(); + let diff = resolve_commit_diff(&hash)?.expect("commit should resolve"); + assert!(diff.contains("OrderItem.php")); + anyhow::Ok(()) + })(); + + std::env::set_current_dir(original_cwd).expect("restore cwd"); + result.expect("resolve_commit_diff for existing commit"); +} + +#[test] +fn resolve_commit_diff_returns_none_for_unknown_hash() { + let _guard = CWD_LOCK.lock().expect("cwd lock"); + 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 = resolve_commit_diff("deadbeef"); + + std::env::set_current_dir(original_cwd).expect("restore cwd"); + assert_eq!(result.expect("should not error"), None); +} + +#[test] +fn format_git_error_empty_stderr() { + let err = format_git_error(&["status"], Some(1), ""); + let msg = err.to_string(); + assert_eq!(msg, "git [\"status\"] exited with status Some(1)"); +}