From 5d9ecec82cff8fedcd785af8480bc71b2e335666 Mon Sep 17 00:00:00 2001 From: vikingowl Date: Sun, 26 Oct 2025 01:21:17 +0200 Subject: [PATCH] feat(ollama): align provider defaults with codex semantics --- agents.md | 6 + crates/owlen-core/src/providers/ollama.rs | 24 +- crates/owlen-core/tests/ollama_wiremock.rs | 307 +++++++++++---------- 3 files changed, 187 insertions(+), 150 deletions(-) create mode 100644 agents.md diff --git a/agents.md b/agents.md new file mode 100644 index 0000000..3c06574 --- /dev/null +++ b/agents.md @@ -0,0 +1,6 @@ +# Agents Upgrade Plan + +- test: rebuild ollama integration suite to exercise local, cloud, search, and quota error flows +- refactor: simplify provider/session coupling so compute_web_search_settings shares normalization utilities +- docs: clarify cloud vs local env configuration (api key precedence, insecure toggle) in configuration guide +- ci: add targeted regression job for ollama providers covering chat, streaming, and tool execution paths diff --git a/crates/owlen-core/src/providers/ollama.rs b/crates/owlen-core/src/providers/ollama.rs index 49b7ef9..71acde9 100644 --- a/crates/owlen-core/src/providers/ollama.rs +++ b/crates/owlen-core/src/providers/ollama.rs @@ -2053,10 +2053,14 @@ fn normalize_base_url( } let path = url.path().trim_end_matches('/'); - if path == "/api" { - url.set_path("/"); - } else if !path.is_empty() && path != "/" { - return Err("Ollama base URLs must not include additional path segments".to_string()); + match path { + "" | "/" => {} + "/api" | "/v1" => { + url.set_path("/"); + } + _ => { + return Err("Ollama base URLs must not include additional path segments".to_string()); + } } if mode_hint == OllamaMode::Cloud { @@ -2163,6 +2167,18 @@ mod tests { assert_eq!(url, "https://ollama.com"); } + #[test] + fn normalize_base_url_accepts_v1_path_for_local() { + let url = normalize_base_url(Some("http://localhost:11434/v1"), OllamaMode::Local).unwrap(); + assert_eq!(url, "http://localhost:11434"); + } + + #[test] + fn normalize_base_url_accepts_v1_path_for_cloud() { + let url = normalize_base_url(Some("https://api.ollama.com/v1"), OllamaMode::Cloud).unwrap(); + assert_eq!(url, "https://ollama.com"); + } + #[test] fn normalize_base_url_canonicalises_api_hostname() { let url = normalize_base_url(Some("https://api.ollama.com"), OllamaMode::Cloud).unwrap(); diff --git a/crates/owlen-core/tests/ollama_wiremock.rs b/crates/owlen-core/tests/ollama_wiremock.rs index 6e4cb08..a0e2d28 100644 --- a/crates/owlen-core/tests/ollama_wiremock.rs +++ b/crates/owlen-core/tests/ollama_wiremock.rs @@ -1,7 +1,7 @@ -use std::sync::Arc; +use std::{sync::Arc, time::Duration}; use owlen_core::tools::WEB_SEARCH_TOOL_NAME; -use owlen_core::types::{ChatParameters, Role}; +use owlen_core::types::{ChatParameters, ChatResponse, Role}; use owlen_core::{ Config, Provider, providers::OllamaProvider, @@ -84,6 +84,19 @@ async fn create_session( (session, temp_dir) } +async fn send_prompt(session: &mut SessionController, message: &str) -> ChatResponse { + match session + .send_message(message.to_string(), ChatParameters::default()) + .await + { + Ok(SessionOutcome::Complete(response)) => response, + Ok(SessionOutcome::Streaming { .. }) => { + panic!("expected complete outcome, got streaming response") + } + Err(err) => panic!("send_message failed: {err:?}"), + } +} + fn configure_local(base_url: &str) -> Config { let mut config = Config::default(); config.general.default_provider = "ollama_local".into(); @@ -100,7 +113,7 @@ fn configure_local(base_url: &str) -> Config { config } -fn configure_cloud(base_url: &str) -> Config { +fn configure_cloud(base_url: &str, search_endpoint: &str) -> Config { let mut config = Config::default(); config.general.default_provider = "ollama_cloud".into(); config.general.default_model = Some("llama3:8b-cloud".into()); @@ -119,7 +132,7 @@ fn configure_cloud(base_url: &str) -> Config { cloud.api_key = Some("test-key".into()); cloud.extra.insert( "web_search_endpoint".into(), - Value::String("/v1/web/search".into()), + Value::String(search_endpoint.into()), ); cloud.extra.insert( owlen_core::config::OLLAMA_CLOUD_ENDPOINT_KEY.into(), @@ -130,47 +143,32 @@ fn configure_cloud(base_url: &str) -> Config { config } -#[tokio::test(flavor = "multi_thread")] -async fn local_provider_happy_path_records_usage() { +async fn run_local_completion(base_suffix: &str) { let server = MockServer::start().await; + let raw_base = format!("{}{}", server.uri(), base_suffix); let tags = load_fixture("ollama_tags"); let completion = load_fixture("ollama_local_completion"); - let base_url = server.uri(); - - let tags_template = ResponseTemplate::new(200).set_body_json(tags); Mock::given(method("GET")) .and(path("/api/tags")) - .respond_with(tags_template.clone()) + .respond_with(ResponseTemplate::new(200).set_body_json(tags)) .mount(&server) .await; Mock::given(method("POST")) .and(path("/api/chat")) .respond_with(ResponseTemplate::new(200).set_body_json(completion)) + .expect(1) .mount(&server) .await; - let config = configure_local(&base_url); + let config = configure_local(&raw_base); let provider: Arc = - Arc::new(OllamaProvider::new(base_url.clone()).expect("local provider")); + Arc::new(OllamaProvider::new(raw_base.clone()).expect("local provider")); let (mut session, _tmp) = create_session(provider, config).await; - - let outcome = session - .send_message( - "Summarise the local status.".to_string(), - ChatParameters::default(), - ) - .await - .expect("local completion"); - - let response = match outcome { - SessionOutcome::Complete(response) => response, - _ => panic!("expected complete outcome"), - }; - + let response = send_prompt(&mut session, "Summarise the local status.").await; assert_eq!(response.message.content, "Local response complete."); let snapshot = session @@ -182,34 +180,41 @@ async fn local_provider_happy_path_records_usage() { assert_eq!(snapshot.weekly.total_tokens, 36); } -#[tokio::test(flavor = "multi_thread")] -async fn cloud_tool_call_flows_through_web_search() { +async fn run_cloud_tool_flow(base_suffix: &str) { let server = MockServer::start().await; + let raw_base = format!("{}{}", server.uri(), base_suffix); + let search_endpoint = if base_suffix.is_empty() { + "/v1/web/search" + } else { + "/web/search" + }; let tags = load_fixture("ollama_tags"); let tool_call = load_fixture("ollama_cloud_tool_call"); let final_chunk = load_fixture("ollama_cloud_final"); - let base_url = server.uri(); - - let tags_template = ResponseTemplate::new(200).set_body_json(tags); Mock::given(method("GET")) .and(path("/api/tags")) - .respond_with(tags_template.clone()) + .and(header("authorization", "Bearer test-key")) + .respond_with(ResponseTemplate::new(200).set_body_json(tags)) .mount(&server) .await; Mock::given(method("POST")) .and(path("/api/chat")) + .and(header("authorization", "Bearer test-key")) .and(BodySubstringMatcher::not_contains("\"role\":\"tool\"")) .respond_with(ResponseTemplate::new(200).set_body_json(tool_call)) + .expect(1) .mount(&server) .await; Mock::given(method("POST")) .and(path("/api/chat")) + .and(header("authorization", "Bearer test-key")) .and(BodySubstringMatcher::contains("\"role\":\"tool\"")) .respond_with(ResponseTemplate::new(200).set_body_json(final_chunk)) + .expect(1) .mount(&server) .await; @@ -229,38 +234,36 @@ async fn cloud_tool_call_flows_through_web_search() { .mount(&server) .await; - let config = configure_cloud(&base_url); + let config = configure_cloud(&raw_base, search_endpoint); let cloud_cfg = config .providers .get("ollama_cloud") .expect("cloud provider config") .clone(); assert_eq!(cloud_cfg.api_key.as_deref(), Some("test-key")); - assert_eq!(cloud_cfg.base_url.as_deref(), Some(base_url.as_str())); + assert_eq!(cloud_cfg.base_url.as_deref(), Some(raw_base.as_str())); + assert_eq!(cloud_cfg.api_key.as_deref(), Some("test-key")); + assert_eq!(cloud_cfg.base_url.as_deref(), Some(raw_base.as_str())); + let provider: Arc = Arc::new( OllamaProvider::from_config("ollama_cloud", &cloud_cfg, Some(&config.general)) .expect("cloud provider"), ); let (mut session, _tmp) = create_session(provider, config).await; + let search_url = format!( + "{}/{}", + raw_base.trim_end_matches('/'), + search_endpoint.trim_start_matches('/') + ); + session.grant_consent( WEB_SEARCH_TOOL_NAME, vec!["network".into()], - vec![format!("{}/v1/web/search", base_url)], + vec![search_url], ); - let outcome = session - .send_message( - "What is new in Rust today?".to_string(), - ChatParameters::default(), - ) - .await; - - let response = match outcome { - Ok(SessionOutcome::Complete(response)) => response, - Ok(_) => panic!("expected complete outcome"), - Err(err) => panic!("cloud completion: {err:?}"), - }; + let response = send_prompt(&mut session, "What is new in Rust today?").await; assert_eq!( response.message.content, "Rust 1.85 shipped today. Summarising the highlights now." @@ -283,114 +286,126 @@ async fn cloud_tool_call_flows_through_web_search() { .await .expect("usage snapshot"); assert_eq!(snapshot.provider, "ollama_cloud"); - assert_eq!(snapshot.hourly.total_tokens, 112); // 64 prompt + 48 completion + assert_eq!(snapshot.hourly.total_tokens, 112); assert_eq!(snapshot.weekly.total_tokens, 112); } +async fn run_cloud_error( + base_suffix: &str, + status: u16, + error_body: Value, + prompt: &str, +) -> String { + let server = MockServer::start().await; + let raw_base = format!("{}{}", server.uri(), base_suffix); + let search_endpoint = if base_suffix.is_empty() { + "/v1/web/search" + } else { + "/web/search" + }; + + let tags = load_fixture("ollama_tags"); + + Mock::given(method("GET")) + .and(path("/api/tags")) + .and(header("authorization", "Bearer test-key")) + .respond_with(ResponseTemplate::new(200).set_body_json(tags)) + .mount(&server) + .await; + + Mock::given(method("POST")) + .and(path("/api/chat")) + .and(header("authorization", "Bearer test-key")) + .respond_with( + ResponseTemplate::new(status) + .set_body_json(error_body) + .set_delay(Duration::from_millis(5)), + ) + .expect(1) + .mount(&server) + .await; + + let config = configure_cloud(&raw_base, search_endpoint); + let cloud_cfg = config + .providers + .get("ollama_cloud") + .expect("cloud provider config") + .clone(); + let provider: Arc = Arc::new( + OllamaProvider::from_config("ollama_cloud", &cloud_cfg, Some(&config.general)) + .expect("cloud provider"), + ); + + let (mut session, _tmp) = create_session(provider, config).await; + + let err_text = match session + .send_message(prompt.to_string(), ChatParameters::default()) + .await + { + Ok(_) => panic!("expected error status {status} but request succeeded"), + Err(err) => err.to_string(), + }; + + let snapshot = session + .current_usage_snapshot() + .await + .expect("usage snapshot"); + assert_eq!(snapshot.hourly.total_tokens, 0); + assert_eq!(snapshot.weekly.total_tokens, 0); + + err_text +} + +#[tokio::test(flavor = "multi_thread")] +async fn local_provider_happy_path_records_usage() { + run_local_completion("").await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn local_provider_accepts_v1_base_url() { + run_local_completion("/v1").await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn cloud_tool_call_flows_through_web_search() { + run_cloud_tool_flow("").await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn cloud_tool_call_accepts_v1_base_url() { + run_cloud_tool_flow("/v1").await; +} + #[tokio::test(flavor = "multi_thread")] async fn cloud_unauthorized_degrades_without_usage() { - let server = MockServer::start().await; - - let tags = load_fixture("ollama_tags"); - let tags_template = ResponseTemplate::new(200).set_body_json(tags); - let base_url = server.uri(); - Mock::given(method("GET")) - .and(path("/api/tags")) - .respond_with(tags_template.clone()) - .mount(&server) + for suffix in ["", "/v1"] { + let err_text = run_cloud_error( + suffix, + 401, + json!({ "error": "unauthorized" }), + "Switch to cloud", + ) .await; - - Mock::given(method("POST")) - .and(path("/api/chat")) - .respond_with(ResponseTemplate::new(401).set_body_json(json!({ - "error": "unauthorized" - }))) - .mount(&server) - .await; - - let config = configure_cloud(&base_url); - let cloud_cfg = config - .providers - .get("ollama_cloud") - .expect("cloud provider config") - .clone(); - let provider: Arc = Arc::new( - OllamaProvider::from_config("ollama_cloud", &cloud_cfg, Some(&config.general)) - .expect("cloud provider"), - ); - - let (mut session, _tmp) = create_session(provider, config).await; - - let err_text = match session - .send_message("Switch to cloud".to_string(), ChatParameters::default()) - .await - { - Ok(_) => panic!("expected unauthorized error, but request succeeded"), - Err(err) => err.to_string(), - }; - assert!( - err_text.contains("unauthorized") || err_text.contains("API key"), - "error should surface unauthorized detail, got: {err_text}" - ); - - let snapshot = session - .current_usage_snapshot() - .await - .expect("usage snapshot"); - assert_eq!(snapshot.hourly.total_tokens, 0); - assert_eq!(snapshot.weekly.total_tokens, 0); + assert!( + err_text.contains("unauthorized") || err_text.contains("API key"), + "error should surface unauthorized detail for base '{suffix}', got: {err_text}" + ); + } } #[tokio::test(flavor = "multi_thread")] async fn cloud_rate_limit_returns_error_without_usage() { - let server = MockServer::start().await; - - let tags = load_fixture("ollama_tags"); - let tags_template = ResponseTemplate::new(200).set_body_json(tags); - let base_url = server.uri(); - Mock::given(method("GET")) - .and(path("/api/tags")) - .respond_with(tags_template.clone()) - .mount(&server) + for suffix in ["", "/v1"] { + let err_text = run_cloud_error( + suffix, + 429, + json!({ "error": "too many requests" }), + "Hit rate limit", + ) .await; - - Mock::given(method("POST")) - .and(path("/api/chat")) - .respond_with(ResponseTemplate::new(429).set_body_json(json!({ - "error": "too many requests" - }))) - .mount(&server) - .await; - - let config = configure_cloud(&base_url); - let cloud_cfg = config - .providers - .get("ollama_cloud") - .expect("cloud provider config") - .clone(); - let provider: Arc = Arc::new( - OllamaProvider::from_config("ollama_cloud", &cloud_cfg, Some(&config.general)) - .expect("cloud provider"), - ); - - let (mut session, _tmp) = create_session(provider, config).await; - - let err_text = match session - .send_message("Hit rate limit".to_string(), ChatParameters::default()) - .await - { - Ok(_) => panic!("expected rate-limit error, but request succeeded"), - Err(err) => err.to_string(), - }; - assert!( - err_text.contains("rate limited") || err_text.contains("429"), - "error should mention rate limiting, got: {err_text}" - ); - - let snapshot = session - .current_usage_snapshot() - .await - .expect("usage snapshot"); - assert_eq!(snapshot.hourly.total_tokens, 0); - assert_eq!(snapshot.weekly.total_tokens, 0); + assert!( + err_text.contains("rate limited") || err_text.contains("429"), + "error should mention rate limiting for base '{suffix}', got: {err_text}" + ); + } }