diff --git a/crates/goose/src/dictation/providers.rs b/crates/goose/src/dictation/providers.rs index a23d971f94..2d2378970b 100644 --- a/crates/goose/src/dictation/providers.rs +++ b/crates/goose/src/dictation/providers.rs @@ -2,6 +2,7 @@ use crate::config::Config; #[cfg(feature = "local-inference")] use crate::dictation::whisper::LOCAL_WHISPER_MODEL_CONFIG_KEY; use crate::providers::api_client::{ApiClient, AuthMethod}; +use crate::providers::openai::parse_openai_base_url; use anyhow::Result; use serde::{Deserialize, Serialize}; #[cfg(feature = "local-inference")] @@ -10,6 +11,8 @@ use std::time::Duration; use utoipa::ToSchema; const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); +const OPENAI_VERSIONLESS_TRANSCRIPTIONS_PATH: &str = "audio/transcriptions"; +type OpenAiDictationTarget = (String, Vec<(String, String)>, String); #[cfg(feature = "local-inference")] static LOCAL_TRANSCRIBER: once_cell::sync::Lazy< @@ -179,7 +182,25 @@ pub async fn transcribe_local(audio_bytes: Vec) -> Result { })? } -fn build_api_client(provider: DictationProvider) -> Result { +fn openai_dictation_target(raw_url: &str) -> Result { + let (host, query_params, has_v1) = parse_openai_base_url(raw_url)?; + let endpoint_path = if has_v1 { + "v1/audio/transcriptions".to_string() + } else { + OPENAI_VERSIONLESS_TRANSCRIPTIONS_PATH.to_string() + }; + Ok((host, query_params, endpoint_path)) +} + +fn resolve_openai_base_url_target(raw_url: Option<&str>) -> Result> { + raw_url + .map(str::trim) + .filter(|raw_url| !raw_url.is_empty()) + .map(openai_dictation_target) + .transpose() +} + +fn build_api_client(provider: DictationProvider) -> Result<(ApiClient, String)> { let config = Config::global(); let def = get_provider_def(provider); @@ -188,14 +209,35 @@ fn build_api_client(provider: DictationProvider) -> Result { anyhow::anyhow!("{} not configured", def.config_key) })?; - let base_url = if let Some(host_key) = def.host_key { - config + let (base_url, query_params, endpoint_path) = if provider == DictationProvider::OpenAI { + let openai_base_url = config.get_param::("OPENAI_BASE_URL").ok(); + + if let Ok(host) = std::env::var("OPENAI_HOST") { + (host, vec![], def.endpoint_path.to_string()) + } else if let Some(target) = resolve_openai_base_url_target(openai_base_url.as_deref())? { + target + } else if let Ok(host) = config.get_param::("OPENAI_HOST") { + (host, vec![], def.endpoint_path.to_string()) + } else { + ( + def.default_base_url.to_string(), + vec![], + def.endpoint_path.to_string(), + ) + } + } else if let Some(host_key) = def.host_key { + let base_url = config .get(host_key, false) .ok() .and_then(|v| v.as_str().map(|s| s.to_string())) - .unwrap_or_else(|| def.default_base_url.to_string()) + .unwrap_or_else(|| def.default_base_url.to_string()); + (base_url, vec![], def.endpoint_path.to_string()) } else { - def.default_base_url.to_string() + ( + def.default_base_url.to_string(), + vec![], + def.endpoint_path.to_string(), + ) }; let auth = match provider { @@ -209,10 +251,14 @@ fn build_api_client(provider: DictationProvider) -> Result { DictationProvider::Local => anyhow::bail!("Local provider should not use API client"), }; - ApiClient::with_timeout(base_url, auth, REQUEST_TIMEOUT).map_err(|e| { + let mut client = ApiClient::with_timeout(base_url, auth, REQUEST_TIMEOUT).map_err(|e| { tracing::error!("Failed to create API client: {}", e); e - }) + })?; + if !query_params.is_empty() { + client = client.with_query(query_params); + } + Ok((client, endpoint_path)) } pub async fn transcribe_with_provider( @@ -223,8 +269,7 @@ pub async fn transcribe_with_provider( extension: &str, mime_type: &str, ) -> Result { - let client = build_api_client(provider)?; - let def = get_provider_def(provider); + let (client, endpoint_path) = build_api_client(provider)?; let part = reqwest::multipart::Part::bytes(audio_bytes) .file_name(format!("audio.{}", extension)) @@ -239,7 +284,7 @@ pub async fn transcribe_with_provider( .text(model_param, model_value); let response = client - .request(None, def.endpoint_path) + .request(None, &endpoint_path) .multipart_post(form) .await .map_err(|e| { @@ -274,3 +319,50 @@ pub async fn transcribe_with_provider( Ok(text) } + +#[cfg(test)] +mod tests { + use super::{ + openai_dictation_target, resolve_openai_base_url_target, + OPENAI_VERSIONLESS_TRANSCRIPTIONS_PATH, + }; + + #[test] + fn openai_dictation_target_preserves_prefix_and_query_params() { + let (host, query_params, endpoint_path) = openai_dictation_target( + "https://user:pass@gateway.example.com/openai/v1?api-version=2024-02-01", + ) + .unwrap(); + assert_eq!(host, "https://user:pass@gateway.example.com/openai"); + assert_eq!( + query_params, + vec![("api-version".to_string(), "2024-02-01".to_string())] + ); + assert_eq!(endpoint_path, "v1/audio/transcriptions"); + } + + #[test] + fn openai_dictation_target_uses_versionless_endpoint_without_v1() { + let (host, query_params, endpoint_path) = + openai_dictation_target("https://gateway.example.com/custom/api").unwrap(); + assert_eq!(host, "https://gateway.example.com/custom/api"); + assert!(query_params.is_empty()); + assert_eq!(endpoint_path, OPENAI_VERSIONLESS_TRANSCRIPTIONS_PATH); + } + + #[test] + fn openai_dictation_target_keeps_v1_endpoint_for_bare_host() { + let (host, query_params, endpoint_path) = + openai_dictation_target("https://api.openai.com").unwrap(); + assert_eq!(host, "https://api.openai.com"); + assert!(query_params.is_empty()); + assert_eq!(endpoint_path, "v1/audio/transcriptions"); + } + + #[test] + fn resolve_openai_base_url_target_ignores_blank_values() { + assert!(resolve_openai_base_url_target(Some(" ")) + .unwrap() + .is_none()); + } +} diff --git a/crates/goose/src/providers/openai.rs b/crates/goose/src/providers/openai.rs index 5eef3baded..c8c531aa7e 100644 --- a/crates/goose/src/providers/openai.rs +++ b/crates/goose/src/providers/openai.rs @@ -34,8 +34,10 @@ use rmcp::model::Tool; const OPEN_AI_PROVIDER_NAME: &str = "openai"; const OPEN_AI_DEFAULT_BASE_PATH: &str = "v1/chat/completions"; +const OPEN_AI_VERSIONLESS_BASE_PATH: &str = "chat/completions"; const OPEN_AI_DEFAULT_RESPONSES_PATH: &str = "v1/responses"; const OPEN_AI_DEFAULT_MODELS_PATH: &str = "v1/models"; +const OPEN_AI_DEFAULT_EMBEDDINGS_PATH: &str = "v1/embeddings"; pub const OPEN_AI_DEFAULT_MODEL: &str = "gpt-4o"; pub const OPEN_AI_DEFAULT_FAST_MODEL: &str = "gpt-4o-mini"; pub const OPEN_AI_KNOWN_MODELS: &[(&str, usize)] = &[ @@ -67,6 +69,48 @@ pub const OPEN_AI_KNOWN_MODELS: &[(&str, usize)] = &[ pub const OPEN_AI_DOC_URL: &str = "https://platform.openai.com/docs/models"; +type OpenAiBaseUrlParts = (String, Vec<(String, String)>, bool); + +/// Components extracted from an `OPENAI_BASE_URL` value. +struct ParsedBaseUrl { + /// The host (scheme + authority + any path prefix before `/v1`). + host: String, + /// Query parameters to forward on every request. + query_params: Vec<(String, String)>, + /// Whether the URL path ended with `/v1`. + has_v1: bool, + /// `true` when the host was derived from `OPENAI_BASE_URL`. + /// Controls whether `OPENAI_BASE_PATH` is read from env only + /// (to avoid persisted desktop defaults shadowing URL-derived paths) + /// or from config too (to honour Docker Model Runner setups). + from_base_url: bool, +} + +pub(crate) fn parse_openai_base_url(raw_url: &str) -> Result { + let parsed = url::Url::parse(raw_url) + .map_err(|e| anyhow::anyhow!("Invalid OPENAI_BASE_URL '{}': {}", raw_url, e))?; + + let authority = parsed[..url::Position::BeforePath].to_string(); + let query_params: Vec<(String, String)> = parsed + .query_pairs() + .map(|(k, v)| (k.into_owned(), v.into_owned())) + .collect(); + + let path = parsed.path().trim_end_matches('/'); + if path.is_empty() || path == "/" { + return Ok((authority, query_params, true)); + } + + if path == "/v1" { + return Ok((authority, query_params, true)); + } + if let Some(prefix) = path.strip_suffix("/v1") { + return Ok((format!("{}{}", authority, prefix), query_params, true)); + } + + Ok((format!("{}{}", authority, path), query_params, false)) +} + #[derive(Debug, serde::Serialize)] pub struct OpenAiProvider { #[serde(skip)] @@ -85,9 +129,78 @@ pub struct OpenAiProvider { impl OpenAiProvider { pub async fn from_env(model: ModelConfig) -> Result { let config = crate::config::Config::global(); - let host: String = config - .get_param("OPENAI_HOST") - .unwrap_or_else(|_| "https://api.openai.com".to_string()); + + // Resolve host and base_path. + // + // Priority (highest first): + // 1. OPENAI_HOST env var — session override (deprecated but still + // honoured so that `OPENAI_HOST=… goose` keeps working) + // 2. OPENAI_BASE_URL (env or config) — ecosystem-standard + // 3. OPENAI_HOST from config file — persisted by `goose configure` + // 4. Default "https://api.openai.com" + // + // OPENAI_BASE_URL is parsed into host + query params + a flag + // indicating whether the URL included a /v1 path segment. When /v1 + // is present the default base_path is "v1/chat/completions"; + // otherwise "chat/completions" to match the OpenAI SDK convention. + // + // OPENAI_BASE_PATH always wins when set explicitly. + let parsed = if let Ok(h) = std::env::var("OPENAI_HOST") { + // OPENAI_HOST env var takes priority as a session override so + // that existing scripts like `OPENAI_HOST=… goose` still work + // even after OPENAI_BASE_URL is persisted in config. + ParsedBaseUrl { + host: h, + query_params: vec![], + has_v1: true, + from_base_url: false, + } + } else if let Some(raw_url) = config + .get_param::("OPENAI_BASE_URL") + .ok() + .map(|s| s.trim().to_string()) + .filter(|s| !s.is_empty()) + { + Self::parse_base_url(&raw_url)? + } else { + let h: String = config + .get_param("OPENAI_HOST") + .unwrap_or_else(|_| "https://api.openai.com".to_string()); + ParsedBaseUrl { + host: h, + query_params: vec![], + has_v1: true, + from_base_url: false, + } + }; + + // When the host was derived from OPENAI_BASE_URL, read + // OPENAI_BASE_PATH from env only so that the desktop UI's persisted + // default ("v1/chat/completions") doesn't shadow the versionless + // path. When the host came from OPENAI_HOST (env or config), read + // from config too — Docker Model Runner and similar setups persist a + // custom base_path that must be honoured. + let default_bp = || { + if parsed.has_v1 { + OPEN_AI_DEFAULT_BASE_PATH.to_string() + } else { + OPEN_AI_VERSIONLESS_BASE_PATH.to_string() + } + }; + let base_path: String = if parsed.from_base_url { + std::env::var("OPENAI_BASE_PATH").unwrap_or_else(|_| default_bp()) + } else { + config + .get_param("OPENAI_BASE_PATH") + .unwrap_or_else(|_| default_bp()) + }; + + // Only apply the default fast model when talking to OpenAI directly. + // Custom/compatible endpoints likely don't serve gpt-4o-mini, so + // leave fast_model unset (complete_fast will fall back to the main model). + // Parse the URL and compare the hostname exactly to avoid false positives + // (e.g. https://api.openai.com.local:8000 or proxy paths containing api.openai.com). + let host = parsed.host.clone(); // Only apply the default fast model when talking to OpenAI directly. // Custom/compatible endpoints likely don't serve gpt-4o-mini, so @@ -114,9 +227,6 @@ impl OpenAiProvider { .cloned() .map(parse_custom_headers); - let base_path: String = config - .get_param("OPENAI_BASE_PATH") - .unwrap_or_else(|_| OPEN_AI_DEFAULT_BASE_PATH.to_string()); let organization: Option = config.get_param("OPENAI_ORGANIZATION").ok(); let project: Option = config.get_param("OPENAI_PROJECT").ok(); let timeout_secs: u64 = config.get_param("OPENAI_TIMEOUT").unwrap_or(600); @@ -125,8 +235,15 @@ impl OpenAiProvider { Some(key) if !key.is_empty() => AuthMethod::BearerToken(key), _ => AuthMethod::NoAuth, }; - let mut api_client = - ApiClient::with_timeout(host, auth, std::time::Duration::from_secs(timeout_secs))?; + let mut api_client = ApiClient::with_timeout( + parsed.host, + auth, + std::time::Duration::from_secs(timeout_secs), + )?; + + if !parsed.query_params.is_empty() { + api_client = api_client.with_query(parsed.query_params); + } if let Some(org) = &organization { api_client = api_client.with_header("OpenAI-Organization", org)?; @@ -263,7 +380,16 @@ impl OpenAiProvider { }) } - // Derive a base path from the raw URL path + fn parse_base_url(raw_url: &str) -> Result { + let (host, query_params, has_v1) = parse_openai_base_url(raw_url)?; + Ok(ParsedBaseUrl { + host, + query_params, + has_v1, + from_base_url: true, + }) + } + fn derive_base_path(url_path: &str) -> String { let stripped = url_path.trim_start_matches('/'); let normalized = stripped.trim_end_matches('/'); @@ -300,6 +426,11 @@ impl OpenAiProvider { fn should_use_responses_api(model_name: &str, base_path: &str) -> bool { let normalized_base_path = Self::normalize_base_path(base_path); + // Only the standard "v1/chat/completions" is treated as a default + // path that defers to model-based routing. The versionless + // "chat/completions" (derived from an OPENAI_BASE_URL without /v1) + // is treated as custom because versionless gateways typically do not + // support the Responses API. let has_custom_base_path = normalized_base_path != OPEN_AI_DEFAULT_BASE_PATH; if has_custom_base_path { @@ -415,6 +546,7 @@ impl ProviderDef for OpenAiProvider { OPEN_AI_DOC_URL, vec![ ConfigKey::new("OPENAI_API_KEY", false, true, None, true), + ConfigKey::new("OPENAI_BASE_URL", false, false, None, false), ConfigKey::new( "OPENAI_HOST", true, @@ -715,8 +847,13 @@ impl EmbeddingCapable for OpenAiProvider { }; let request_value = serde_json::to_value(request_clone) .map_err(|e| ProviderError::ExecutionError(e.to_string()))?; + let embeddings_path = Self::map_base_path( + &self.base_path, + "embeddings", + OPEN_AI_DEFAULT_EMBEDDINGS_PATH, + ); self.api_client - .api_post(Some(session_id), "v1/embeddings", &request_value) + .api_post(Some(session_id), &embeddings_path, &request_value) .await .map_err(|e| ProviderError::ExecutionError(e.to_string())) }) @@ -894,54 +1031,91 @@ mod tests { let models_path = OpenAiProvider::map_base_path("/custom/path", "models", "v1/models"); assert_eq!(models_path, "/v1/models"); } - #[test] - fn derive_base_path_empty_path_gives_default_endpoint() { - assert_eq!(OpenAiProvider::derive_base_path("/"), "v1/chat/completions"); + fn parse_base_url_strips_v1_from_standard_openai_url() { + let r = OpenAiProvider::parse_base_url("https://api.openai.com/v1").unwrap(); + assert_eq!(r.host, "https://api.openai.com"); + assert!(r.query_params.is_empty()); + assert!(r.has_v1); } #[test] - fn derive_base_path_bare_v1_gives_chat_completions() { + fn parse_base_url_preserves_prefix_before_v1() { + let r = OpenAiProvider::parse_base_url("https://gateway.example.com/openai/v1").unwrap(); + assert_eq!(r.host, "https://gateway.example.com/openai"); + assert!(r.has_v1); + } + + #[test] + fn parse_base_url_handles_no_path() { + let r = OpenAiProvider::parse_base_url("https://api.openai.com").unwrap(); + assert_eq!(r.host, "https://api.openai.com"); + assert!(r.has_v1); + } + + #[test] + fn parse_base_url_handles_trailing_slash() { + let r = OpenAiProvider::parse_base_url("https://api.openai.com/v1/").unwrap(); + assert_eq!(r.host, "https://api.openai.com"); + assert!(r.has_v1); + } + + #[test] + fn parse_base_url_preserves_port() { + let r = OpenAiProvider::parse_base_url("https://localhost:8080/v1").unwrap(); + assert_eq!(r.host, "https://localhost:8080"); + assert!(r.has_v1); + } + + #[test] + fn parse_base_url_preserves_non_v1_path() { + let r = OpenAiProvider::parse_base_url("https://example.com/custom/api").unwrap(); + assert_eq!(r.host, "https://example.com/custom/api"); + assert!(!r.has_v1); + } + + #[test] + fn parse_base_url_preserves_query_params() { + let r = OpenAiProvider::parse_base_url("https://gw.example.com/v1?api-version=2024-02-01") + .unwrap(); + assert_eq!(r.host, "https://gw.example.com"); assert_eq!( - OpenAiProvider::derive_base_path("/v1"), - "v1/chat/completions" + r.query_params, + vec![("api-version".to_string(), "2024-02-01".to_string())] ); + assert!(r.has_v1); } #[test] - fn derive_base_path_v1_with_trailing_slash() { - assert_eq!( - OpenAiProvider::derive_base_path("/v1/"), - "v1/chat/completions" - ); + fn parse_base_url_preserves_multiple_query_params() { + let r = OpenAiProvider::parse_base_url("https://example.com/v1?key=val&foo=bar").unwrap(); + assert_eq!(r.query_params.len(), 2); + assert_eq!(r.query_params[0], ("key".to_string(), "val".to_string())); + assert_eq!(r.query_params[1], ("foo".to_string(), "bar".to_string())); } #[test] - fn derive_base_path_prefixed_v1_appends_chat_completions() { - assert_eq!( - OpenAiProvider::derive_base_path("/zen/go/v1"), - "zen/go/v1/chat/completions" - ); + fn parse_base_url_preserves_credentials() { + let r = OpenAiProvider::parse_base_url("https://user:pass@gateway.example.com/v1").unwrap(); + assert_eq!(r.host, "https://user:pass@gateway.example.com"); + assert!(r.has_v1); } #[test] - fn derive_base_path_prefixed_v1_with_trailing_slash() { - assert_eq!( - OpenAiProvider::derive_base_path("/zen/go/v1/"), - "zen/go/v1/chat/completions" - ); + fn parse_base_url_rejects_empty_string() { + assert!(OpenAiProvider::parse_base_url("").is_err()); } #[test] - fn derive_base_path_full_chat_completions_url_unchanged() { - assert_eq!( - OpenAiProvider::derive_base_path("/openai/v1/chat/completions"), - "openai/v1/chat/completions" - ); + fn parse_base_url_rejects_whitespace_only() { + assert!(OpenAiProvider::parse_base_url(" ").is_err()); } #[test] - fn derive_base_path_non_v1_prefix_unchanged() { - assert_eq!(OpenAiProvider::derive_base_path("/anthropic"), "anthropic"); + fn versionless_base_path_opts_out_of_responses_for_codex_models() { + assert!(!OpenAiProvider::should_use_responses_api( + "gpt-5-codex", + "chat/completions" + )); } }