From f6401edc8555fbbf6c29f7f0a3164327f941e707 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Wed, 27 May 2026 19:45:03 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20Ollama=20API=20compatibility=20=E2=80=94?= =?UTF-8?q?=20allow=20bare=20model=20names=20and=20smart=20openai/=20prefi?= =?UTF-8?q?x=20handling=20on=20wire?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fix: Ollama API compatibility — allow bare model names and smart openai/ prefix handling on wire --- .../crates/api/src/providers/openai_compat.rs | 184 ++++++++++++++++-- .../api/tests/openai_compat_integration.rs | 43 +++- rust/crates/rusty-claude-cli/src/main.rs | 138 ++++++++++++- 3 files changed, 341 insertions(+), 24 deletions(-) diff --git a/rust/crates/api/src/providers/openai_compat.rs b/rust/crates/api/src/providers/openai_compat.rs index d5291b8eae..39fcec233d 100644 --- a/rust/crates/api/src/providers/openai_compat.rs +++ b/rust/crates/api/src/providers/openai_compat.rs @@ -932,6 +932,48 @@ fn strip_routing_prefix(model: &str) -> &str { } } +/// Normalize a base URL for comparison purposes. +/// +/// Strips any trailing slashes and a trailing `/chat/completions` path +/// component so that the following variants are all treated as equivalent: +/// - `https://api.openai.com/v1` +/// - `https://api.openai.com/v1/` +/// - `https://api.openai.com/v1/chat/completions` +fn normalize_base_url(url: &str) -> &str { + let url = url.trim_end_matches('/'); + url.strip_suffix("/chat/completions") + .map(|s| s.trim_end_matches('/')) + .unwrap_or(url) +} + +/// Extract the host (without port) from a URL string. +/// Returns an empty string if the URL cannot be parsed. +fn url_host(url: &str) -> &str { + // Strip scheme ("https://", "http://", etc.) + let rest = match url.split_once("://") { + Some((_, r)) => r, + None => return "", + }; + // Isolate the authority (before the first '/', '?', or '#') + let authority = rest.split(['/', '?', '#']).next().unwrap_or(""); + // Strip optional userinfo (e.g. "user:pass@" in "user:pass@localhost:11434") + let authority = match authority.rsplit_once('@') { + Some((_, host_port)) => host_port, + None => authority, + }; + if authority.starts_with('[') { + // IPv6 literal: host is between '[' and ']' + authority + .split(']') + .next() + .unwrap_or("") + .trim_start_matches('[') + } else { + // IPv4 or hostname: strip optional port + authority.split(':').next().unwrap_or("") + } +} + fn wire_model_for_base_url<'a>( model: &'a str, config: OpenAiCompatConfig, @@ -944,21 +986,24 @@ fn wire_model_for_base_url<'a>( let lowered_prefix = prefix.to_ascii_lowercase(); if lowered_prefix == "openai" { - let trimmed_base_url = base_url.trim_end_matches('/'); - let default_openai = DEFAULT_OPENAI_BASE_URL.trim_end_matches('/'); - if matches!( - lowered_prefix.as_str(), - "xai" | "grok" | "kimi" | "gemini" | "gemma" - ) { + // The `openai/` prefix is a claw-code routing hint. Whether to strip it + // depends on the target endpoint: + // + // - Default OpenAI endpoint: strip (it is only a routing prefix here). + // - Known-local endpoints (localhost / 127.0.0.1 / ::1, e.g. Ollama, + // LM Studio): strip because local servers use bare model names. + // - Custom non-local endpoints (OpenRouter, other gateways): preserve + // the full slug so the gateway receives the model ID it expects + // (e.g. `openai/gpt-4.1-mini` for OpenRouter). + let is_default_url = normalize_base_url(base_url) + .eq_ignore_ascii_case(normalize_base_url(config.default_base_url)); + let host = url_host(base_url); + let is_local_url = + host.eq_ignore_ascii_case("localhost") || matches!(host, "127.0.0.1" | "::1"); + if is_default_url || is_local_url { return Cow::Borrowed(&model[pos + 1..]); } - if config.provider_name == "OpenAI" && trimmed_base_url != default_openai { - // Only preserve the full slug if it's NOT a model we want to strip - if !model.contains("gemini") && !model.contains("gemma") { - return Cow::Borrowed(model); - } - } - return Cow::Borrowed(&model[pos + 1..]); + return Cow::Borrowed(model); } if matches!(lowered_prefix.as_str(), "xai" | "grok" | "qwen" | "kimi") { @@ -2730,4 +2775,117 @@ mod tests { assert_eq!(super::strip_routing_prefix("kimi-k2.5"), "kimi-k2.5"); // no prefix, unchanged assert_eq!(super::strip_routing_prefix("kimi/kimi-k1.5"), "kimi-k1.5"); } + + #[test] + fn wire_model_strips_openai_prefix_for_custom_base_url() { + // Issue #3123: Ollama models with openai/ prefix should have prefix + // stripped for the default OpenAI endpoint and for known-local endpoints, + // but preserved for custom non-local gateways (e.g. OpenRouter). + use std::borrow::Cow; + let ollama_url = "http://localhost:11434/v1"; + let openrouter_url = "https://openrouter.ai/api/v1"; + let config = super::OpenAiCompatConfig::openai(); + + // openai/ prefix stripped for known-local URL (Ollama) + assert_eq!( + super::wire_model_for_base_url("openai/qwen2.5-coder:7b", config, ollama_url), + Cow::Borrowed("qwen2.5-coder:7b") + ); + + // openai/ prefix stripped for 127.0.0.1 + assert_eq!( + super::wire_model_for_base_url("openai/llama3.2", config, "http://127.0.0.1:11434/v1"), + Cow::Borrowed("llama3.2") + ); + + // openai/ prefix stripped for IPv6 loopback + assert_eq!( + super::wire_model_for_base_url("openai/llama3.2", config, "http://[::1]:11434/v1"), + Cow::Borrowed("llama3.2") + ); + + // openai/ prefix stripped for default OpenAI URL + assert_eq!( + super::wire_model_for_base_url("openai/gpt-4o", config, super::DEFAULT_OPENAI_BASE_URL), + Cow::Borrowed("gpt-4o") + ); + + // openai/ prefix preserved for custom non-local gateway (OpenRouter) + assert_eq!( + super::wire_model_for_base_url("openai/gpt-4.1-mini", config, openrouter_url), + Cow::Borrowed("openai/gpt-4.1-mini") + ); + + // openai/ prefix preserved for a domain that contains "localhost" as a substring + // (false-positive guard: must match the host exactly, not via substring) + assert_eq!( + super::wire_model_for_base_url( + "openai/gpt-4.1-mini", + config, + "https://not-localhost.example.com/v1" + ), + Cow::Borrowed("openai/gpt-4.1-mini") + ); + + // Bare model names (no slash) pass through unchanged + assert_eq!( + super::wire_model_for_base_url("qwen2.5-coder:7b", config, ollama_url), + Cow::Borrowed("qwen2.5-coder:7b") + ); + + // xai/ prefix stripped + let xai_config = super::OpenAiCompatConfig::xai(); + assert_eq!( + super::wire_model_for_base_url("xai/grok-3", xai_config, super::DEFAULT_XAI_BASE_URL), + Cow::Borrowed("grok-3") + ); + + // Regression: trailing slash on the default OpenAI URL must still strip openai/ + assert_eq!( + super::wire_model_for_base_url("openai/gpt-4o", config, "https://api.openai.com/v1/"), + Cow::Borrowed("gpt-4o") + ); + + // Regression: full chat/completions path as base URL must still strip openai/ + assert_eq!( + super::wire_model_for_base_url( + "openai/gpt-4o", + config, + "https://api.openai.com/v1/chat/completions" + ), + Cow::Borrowed("gpt-4o") + ); + + // Regression: host matching is case-insensitive for default OpenAI URL + assert_eq!( + super::wire_model_for_base_url("openai/gpt-4o", config, "https://API.OPENAI.COM/v1"), + Cow::Borrowed("gpt-4o") + ); + + // Regression: host matching is case-insensitive for known-local URLs + assert_eq!( + super::wire_model_for_base_url("openai/llama3.2", config, "http://LOCALHOST:11434/v1"), + Cow::Borrowed("llama3.2") + ); + + // Regression: URLs with userinfo should still be recognized as local + assert_eq!( + super::wire_model_for_base_url( + "openai/llama3.2", + config, + "http://user:pass@localhost:11434/v1" + ), + Cow::Borrowed("llama3.2") + ); + + // Regression: URLs with userinfo for non-local gateways should preserve prefix + assert_eq!( + super::wire_model_for_base_url( + "openai/gpt-4.1-mini", + config, + "https://user:pass@openrouter.ai/api/v1" + ), + Cow::Borrowed("openai/gpt-4.1-mini") + ); + } } diff --git a/rust/crates/api/tests/openai_compat_integration.rs b/rust/crates/api/tests/openai_compat_integration.rs index d87446756e..e503391ad5 100644 --- a/rust/crates/api/tests/openai_compat_integration.rs +++ b/rust/crates/api/tests/openai_compat_integration.rs @@ -162,7 +162,7 @@ async fn send_message_preserves_deepseek_reasoning_content_before_text() { } #[tokio::test] -async fn custom_openai_gateway_preserves_slash_model_ids_and_extra_body_params() { +async fn custom_openai_gateway_strips_openai_prefix_and_preserves_extra_body_params() { let state = Arc::new(Mutex::new(Vec::::new())); let body = concat!( "{", @@ -206,7 +206,7 @@ async fn custom_openai_gateway_preserves_slash_model_ids_and_extra_body_params() let captured = state.lock().await; let request = captured.first().expect("captured request"); let body: serde_json::Value = serde_json::from_str(&request.body).expect("json body"); - assert_eq!(body["model"], json!("openai/gpt-4.1-mini")); + assert_eq!(body["model"], json!("gpt-4.1-mini")); assert_eq!( body["web_search_options"], json!({"search_context_size": "low"}) @@ -214,6 +214,45 @@ async fn custom_openai_gateway_preserves_slash_model_ids_and_extra_body_params() assert_eq!(body["parallel_tool_calls"], json!(false)); } +#[tokio::test] +async fn custom_openai_gateway_preserves_non_routing_slash_model_ids() { + let state = Arc::new(Mutex::new(Vec::::new())); + let body = concat!( + "{", + "\"id\":\"chatcmpl_non_routing_slash\",", + "\"model\":\"my-org/my-fine-tuned-model\",", + "\"choices\":[{", + "\"message\":{\"role\":\"assistant\",\"content\":\"Custom model reply\",\"tool_calls\":[]},", + "\"finish_reason\":\"stop\"", + "}],", + "\"usage\":{\"prompt_tokens\":4,\"completion_tokens\":3}", + "}" + ); + let server = spawn_server( + state.clone(), + vec![http_response("200 OK", "application/json", body)], + ) + .await; + + let client = OpenAiCompatClient::new("openai-test-key", OpenAiCompatConfig::openai()) + .with_base_url(server.base_url()); + let response = client + .send_message(&MessageRequest { + model: "my-org/my-fine-tuned-model".to_string(), + ..sample_request(false) + }) + .await + .expect("gateway request should succeed"); + + assert_eq!(response.model, "my-org/my-fine-tuned-model"); + assert_eq!(response.total_tokens(), 7); + + let captured = state.lock().await; + let request = captured.first().expect("captured request"); + let body: serde_json::Value = serde_json::from_str(&request.body).expect("json body"); + assert_eq!(body["model"], json!("my-org/my-fine-tuned-model")); +} + #[tokio::test] async fn send_message_blocks_oversized_xai_requests_before_the_http_call() { let state = Arc::new(Mutex::new(Vec::::new())); diff --git a/rust/crates/rusty-claude-cli/src/main.rs b/rust/crates/rusty-claude-cli/src/main.rs index 14cfbba968..4872472a9d 100644 --- a/rust/crates/rusty-claude-cli/src/main.rs +++ b/rust/crates/rusty-claude-cli/src/main.rs @@ -1926,9 +1926,41 @@ fn resolve_model_alias_with_config(model: &str) -> String { resolve_model_alias(trimmed).to_string() } -/// Validate model syntax at parse time. -/// Accepts: known aliases (opus, sonnet, haiku) or provider/model pattern. -/// Rejects: empty, whitespace-only, strings with spaces, or invalid chars. +/// Returns true if the given base URL points to a loopback/local host +/// (localhost, 127.0.0.1, or ::1). +fn is_local_base_url(url: &str) -> bool { + let rest = match url.split_once("://") { + Some((_, r)) => r, + None => return false, + }; + let authority = rest.split(['/', '?', '#']).next().unwrap_or(""); + // Strip optional userinfo (e.g. "user:pass@") + let authority = match authority.rsplit_once('@') { + Some((_, host_port)) => host_port, + None => authority, + }; + let host = if authority.starts_with('[') { + // IPv6 literal + authority + .split(']') + .next() + .unwrap_or("") + .trim_start_matches('[') + } else { + authority.split(':').next().unwrap_or("") + }; + host.eq_ignore_ascii_case("localhost") || matches!(host, "127.0.0.1" | "::1") +} + +/// Validate model syntax at parse time. Callers must resolve model aliases +/// (e.g. "opus" → "anthropic/claude-opus-4-6") before calling this function; +/// raw aliases are rejected. +/// +/// Accepts: `provider/model` format (e.g. "anthropic/claude-opus-4-6"), +/// or bare model names when `OPENAI_BASE_URL` points to a loopback host +/// (for local providers like Ollama, LM Studio, vLLM — e.g. "qwen2.5-coder:7b"). +/// Rejects: empty or whitespace-only strings, strings containing spaces, +/// and bare model names when no loopback `OPENAI_BASE_URL` is configured. fn validate_model_syntax(model: &str) -> Result<(), String> { let trimmed = model.trim(); if trimmed.is_empty() { @@ -1944,6 +1976,19 @@ fn validate_model_syntax(model: &str) -> Result<(), String> { // Check provider/model format: provider_id/model_id let parts: Vec<&str> = trimmed.split('/').collect(); if parts.len() != 2 || parts[0].is_empty() || parts[1].is_empty() { + // When OPENAI_BASE_URL points to a loopback/local endpoint + // (Ollama, LM Studio, vLLM, etc.), allow bare model names + // (e.g. "qwen2.5-coder:7b", "llama3:8b") that don't follow the + // provider/model convention. Only bypass for local hosts to avoid + // masking errors for non-local gateways (e.g. OpenRouter) where + // a slash-containing slug is typically required. + if parts.len() == 1 { + if let Ok(base_url) = std::env::var("OPENAI_BASE_URL") { + if is_local_base_url(&base_url) { + return Ok(()); + } + } + } // #154: hint if the model looks like it belongs to a different provider let mut err_msg = format!( "invalid model syntax: '{}'.\nExpected provider/model (e.g., anthropic/claude-opus-4-7)", @@ -11334,6 +11379,15 @@ fn print_help(output_format: CliOutputFormat) -> Result<(), Box std::sync::MutexGuard<'static, ()> { + use std::sync::{Mutex, OnceLock}; + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) +} + #[cfg(test)] mod tests { use super::{ @@ -11620,11 +11674,8 @@ mod tests { ); } - fn env_lock() -> MutexGuard<'static, ()> { - static LOCK: OnceLock> = OnceLock::new(); - LOCK.get_or_init(|| Mutex::new(())) - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) + fn env_lock() -> std::sync::MutexGuard<'static, ()> { + super::env_lock() } fn with_current_dir(cwd: &Path, f: impl FnOnce() -> T) -> T { @@ -16447,7 +16498,37 @@ mod dump_manifests_tests { #[cfg(test)] mod alias_resolution_tests { - use super::{resolve_model_alias_with_config, validate_model_syntax}; + use std::ffi::OsString; + + use super::{env_lock, resolve_model_alias_with_config, validate_model_syntax}; + + struct ScopedEnvVar { + key: &'static str, + previous: Option, + } + + impl ScopedEnvVar { + fn set(key: &'static str, value: &str) -> Self { + let previous = std::env::var_os(key); + std::env::set_var(key, value); + Self { key, previous } + } + + fn remove(key: &'static str) -> Self { + let previous = std::env::var_os(key); + std::env::remove_var(key); + Self { key, previous } + } + } + + impl Drop for ScopedEnvVar { + fn drop(&mut self) { + match &self.previous { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } #[test] fn test_alias_resolution_builtin() { @@ -16468,6 +16549,9 @@ mod alias_resolution_tests { #[test] fn test_alias_resolution_syntax_validation() { + let _guard = env_lock(); + let _url = ScopedEnvVar::remove("OPENAI_BASE_URL"); + // Resolved aliases should pass syntax validation let resolved = resolve_model_alias_with_config("opus"); assert!(validate_model_syntax(&resolved).is_ok()); @@ -16478,6 +16562,9 @@ mod alias_resolution_tests { #[test] fn test_unknown_alias_fails_validation() { + let _guard = env_lock(); + let _url = ScopedEnvVar::remove("OPENAI_BASE_URL"); + // Unknown aliases resolve to themselves let resolved = resolve_model_alias_with_config("unknown-alias"); assert_eq!(resolved, "unknown-alias"); @@ -16490,9 +16577,42 @@ mod alias_resolution_tests { #[test] fn test_direct_provider_model_passes() { + let _guard = env_lock(); + let _url = ScopedEnvVar::remove("OPENAI_BASE_URL"); + // Direct provider/model strings should remain unchanged and pass let model = "openai/gpt-4o"; assert_eq!(resolve_model_alias_with_config(model), model); assert!(validate_model_syntax(model).is_ok()); } + + #[test] + fn test_bare_model_name_passes_when_openai_base_url_set() { + // Issue #3123: Ollama-style bare model names (no provider/ prefix) + // should pass validation when OPENAI_BASE_URL is set, indicating + // a local OpenAI-compatible endpoint is configured. + let _guard = env_lock(); + let _url = ScopedEnvVar::set("OPENAI_BASE_URL", "http://localhost:11434/v1"); + assert!(validate_model_syntax("qwen2.5-coder:7b").is_ok()); + assert!(validate_model_syntax("llama3:8b").is_ok()); + assert!(validate_model_syntax("mistral").is_ok()); + } + + #[test] + fn test_bare_model_name_rejected_for_non_local_openai_base_url() { + // Bare model names should be rejected when OPENAI_BASE_URL points to + // a non-local gateway (e.g. OpenRouter), since those typically require + // a slash-containing slug like `openai/gpt-4.1-mini`. + let _guard = env_lock(); + let _url = ScopedEnvVar::set("OPENAI_BASE_URL", "https://openrouter.ai/api/v1"); + assert!(validate_model_syntax("mistral").is_err()); + assert!(validate_model_syntax("qwen2.5-coder:7b").is_err()); + } + + #[test] + fn test_bare_model_name_passes_for_127_0_0_1() { + let _guard = env_lock(); + let _url = ScopedEnvVar::set("OPENAI_BASE_URL", "http://127.0.0.1:11434/v1"); + assert!(validate_model_syntax("llama3:8b").is_ok()); + } }