diff --git a/crates/goose-provider-types/src/base.rs b/crates/goose-provider-types/src/base.rs index 9924796af7..4a6309b282 100644 --- a/crates/goose-provider-types/src/base.rs +++ b/crates/goose-provider-types/src/base.rs @@ -266,6 +266,9 @@ pub struct ModelInfo { /// Whether this model supports reasoning/thinking controls #[serde(default)] pub reasoning: bool, + /// Whether this model accepts image input; when set, overrides canonical detection + #[serde(default, skip_serializing_if = "Option::is_none")] + pub supports_vision: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub thinking_preservation_format: Option, /// Static params merged into the request body for this model. @@ -284,6 +287,7 @@ impl ModelInfo { currency: None, supports_cache_control: None, reasoning: false, + supports_vision: None, thinking_preservation_format: None, request_params: None, } @@ -299,6 +303,11 @@ impl ModelInfo { self } + pub fn with_vision_support(mut self, supports_vision: bool) -> Self { + self.supports_vision = Some(supports_vision); + self + } + /// Create a new ModelInfo with cost information (per token) pub fn with_cost( name: impl Into, @@ -315,6 +324,7 @@ impl ModelInfo { currency: Some("$".to_string()), supports_cache_control: None, reasoning: false, + supports_vision: None, thinking_preservation_format: None, request_params: None, } @@ -365,11 +375,17 @@ pub fn model_info_for_provider_model(provider_name: &str, model_name: &str) -> M currency: None, supports_cache_control: None, reasoning, + supports_vision: None, thinking_preservation_format: None, request_params: None, } } +/// Model IDs are case-sensitive; declarations apply only to the requested ID. +pub fn find_declared_model<'a>(models: &'a [ModelInfo], model_name: &str) -> Option<&'a ModelInfo> { + models.iter().find(|model| model.name == model_name) +} + /// Build `ModelInfo` for discovered model names, preferring metadata declared in /// provider configuration over the canonical registry. /// @@ -1102,6 +1118,7 @@ mod tests { currency: None, supports_cache_control: None, reasoning: false, + supports_vision: None, thinking_preservation_format: None, request_params: None, }; @@ -1117,6 +1134,7 @@ mod tests { currency: None, supports_cache_control: None, reasoning: false, + supports_vision: None, thinking_preservation_format: None, request_params: None, }; @@ -1132,6 +1150,7 @@ mod tests { currency: None, supports_cache_control: None, reasoning: false, + supports_vision: None, thinking_preservation_format: None, request_params: None, }; diff --git a/crates/goose-provider-types/tests/declared_model.rs b/crates/goose-provider-types/tests/declared_model.rs new file mode 100644 index 0000000000..ed26ed695f --- /dev/null +++ b/crates/goose-provider-types/tests/declared_model.rs @@ -0,0 +1,30 @@ +use goose_provider_types::base::{find_declared_model, ModelInfo}; + +#[test] +fn exact_model_ids_win_over_case_insensitive_collisions() { + let models = [ + ModelInfo::new("Foo").with_vision_support(false), + ModelInfo::new("foo").with_vision_support(true), + ]; + + assert_eq!( + find_declared_model(&models, "foo").unwrap().supports_vision, + Some(true) + ); + assert_eq!( + find_declared_model(&models, "Foo").unwrap().supports_vision, + Some(false) + ); + assert!(find_declared_model(&models, "FOO").is_none()); +} + +#[test] +fn undeclared_case_variants_do_not_inherit_model_metadata() { + let models = [ModelInfo::new("Foo").with_vision_support(true)]; + + assert_eq!(find_declared_model(&models, "Foo").unwrap().name, "Foo"); + assert!(find_declared_model(&models, "foo").is_none()); + assert!(find_declared_model(&models, "FOO").is_none()); + assert!(find_declared_model(&models, "another-model").is_none()); + assert!(find_declared_model(&[], "Foo").is_none()); +} diff --git a/crates/goose-providers/src/azure_foundry.rs b/crates/goose-providers/src/azure_foundry.rs index 077b19a625..9762189ff6 100644 --- a/crates/goose-providers/src/azure_foundry.rs +++ b/crates/goose-providers/src/azure_foundry.rs @@ -417,6 +417,7 @@ fn model_info_for_deployment(deployment_name: &str, model_name: &str) -> ModelIn reasoning: canonical .and_then(|model| model.reasoning) .unwrap_or_else(|| ModelConfig::new(model_name).is_reasoning_model()), + supports_vision: None, thinking_preservation_format: None, request_params: None, } diff --git a/crates/goose-providers/src/databricks.rs b/crates/goose-providers/src/databricks.rs index 80b9638578..9d9c9fdeb5 100644 --- a/crates/goose-providers/src/databricks.rs +++ b/crates/goose-providers/src/databricks.rs @@ -520,6 +520,7 @@ impl DatabricksProvider { currency: None, supports_cache_control: None, reasoning, + supports_vision: None, thinking_preservation_format: None, request_params: None, } diff --git a/crates/goose-providers/src/openai.rs b/crates/goose-providers/src/openai.rs index 3141e70c3a..ec551ed0b9 100644 --- a/crates/goose-providers/src/openai.rs +++ b/crates/goose-providers/src/openai.rs @@ -1,5 +1,8 @@ use super::api_client::ApiClient; -use super::base::{known_models_from_registry, ConfigKey, ModelInfo, Provider, ProviderMetadata}; +use super::base::{ + find_declared_model, known_models_from_registry, ConfigKey, ModelInfo, Provider, + ProviderMetadata, +}; use super::retry::ProviderRetry; use crate::api_client::{AuthMethod, TlsConfig}; use crate::conversation::message::Message; @@ -279,6 +282,7 @@ impl OpenAiProvider { tools: &[Tool], ) -> Result { let mut request_config = model_config.clone(); + request_config.supports_vision = self.vision_support(model_config); if self.native_openai && request_config.reasoning.is_none() { let canonical = goose_provider_types::canonical::maybe_get_canonical_model( "openai", @@ -463,10 +467,13 @@ impl OpenAiProvider { } fn declared_model(&self, model_name: &str) -> Option<&ModelInfo> { - self.custom_models - .as_ref()? - .iter() - .find(|m| m.name == model_name) + find_declared_model(self.custom_models.as_deref()?, model_name) + } + + fn vision_support(&self, model_config: &ModelConfig) -> Option { + self.declared_model(&model_config.model_name) + .and_then(|model| model.supports_vision) + .or(model_config.supports_vision) } fn sanitize_request_for_compat( @@ -806,6 +813,7 @@ impl Provider for OpenAiProvider { let declared_model = self.declared_model(&model_config.model_name); let thinking_preservation_format = declared_model.and_then(|m| m.thinking_preservation_format); + let supports_vision = self.vision_support(model_config).unwrap_or_default(); let mut payload = create_request_with_options( model_config, @@ -817,7 +825,7 @@ impl Provider for OpenAiProvider { OpenAiFormatOptions { preserve_thinking_context: self.preserve_thinking_context || thinking_preservation_format.is_some(), - supports_vision: model_config.supports_vision.unwrap_or_default(), + supports_vision, thinking_preservation_format, }, )?; diff --git a/crates/goose-providers/tests/declared_vision.rs b/crates/goose-providers/tests/declared_vision.rs new file mode 100644 index 0000000000..d06c72533e --- /dev/null +++ b/crates/goose-providers/tests/declared_vision.rs @@ -0,0 +1,127 @@ +use goose_providers::{ + api_client::{ApiClient, AuthMethod}, + base::{ModelInfo, Provider}, + conversation::message::Message, + model::ModelConfig, + openai::OpenAiProviderBuilder, +}; +use rmcp::model::{CallToolRequestParams, CallToolResult, ContentBlock}; +use serde_json::{json, Value}; +use wiremock::{ + matchers::{method, path}, + Mock, MockServer, ResponseTemplate, +}; + +async fn image_request(models: Vec, model: ModelConfig, endpoint: &str) -> Value { + let server = MockServer::start().await; + let response = if endpoint == "responses" { + json!({ + "id": "response-1", "object": "response", "created_at": 0, + "status": "completed", "model": model.model_name, + "output": [{"type": "message", "role": "assistant", "content": [ + {"type": "output_text", "text": "Seen"} + ]}] + }) + } else { + json!({ + "id": "response-1", "model": model.model_name, + "choices": [{"message": {"role": "assistant", "content": "Seen"}, + "finish_reason": "stop"}] + }) + }; + Mock::given(method("POST")) + .and(path(format!("/v1/{endpoint}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(response)) + .expect(1) + .mount(&server) + .await; + + let client = ApiClient::new_with_tls(server.uri(), AuthMethod::NoAuth, None) + .unwrap() + .with_loopback_http_only() + .unwrap(); + let provider = OpenAiProviderBuilder::new(client) + .name("custom_vision") + .base_path(format!("v1/{endpoint}")) + .custom_models(Some(models)) + .supports_streaming(false) + .build(); + let messages = [ + Message::user() + .with_text("Describe these images") + .with_image("dXNlcg==", "image/png"), + Message::assistant() + .with_tool_request("tool-1", Ok(CallToolRequestParams::new("screenshot"))), + Message::user().with_tool_response( + "tool-1", + Ok(CallToolResult::success(vec![ContentBlock::image( + "dG9vbA==", + "image/png", + )])), + ), + ]; + provider + .complete(&model, "Describe images", &messages, &[]) + .await + .unwrap(); + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 1); + let body: Value = requests[0].body_json().unwrap(); + assert_eq!(body["model"], model.model_name); + body +} + +fn assert_images(body: &Value, expected: bool) { + let request = body.to_string(); + if expected { + assert!(request.contains("data:image/png;base64,dXNlcg=="), "{body}"); + assert!(request.contains("data:image/png;base64,dG9vbA=="), "{body}"); + } else { + assert!(!request.contains("data:image"), "{body}"); + assert!(request.contains("model does not support vision"), "{body}"); + } +} + +#[tokio::test] +async fn declared_vision_controls_user_and_tool_images_on_both_endpoints() { + for endpoint in ["chat/completions", "responses"] { + for (declared, configured, expected) in [ + (Some(true), Some(false), true), + (Some(false), Some(true), false), + (Some(true), None, true), + (Some(false), None, false), + (None, Some(true), true), + (None, Some(false), false), + (None, None, false), + ] { + let mut info = ModelInfo::new("local-vlm"); + info.supports_vision = declared; + let mut model = ModelConfig::new("local-vlm"); + model.supports_vision = configured; + let body = image_request(vec![info], model, endpoint).await; + assert_images(&body, expected); + } + } +} + +#[tokio::test] +async fn model_matching_never_borrows_vision_from_a_case_collision() { + for endpoint in ["chat/completions", "responses"] { + let models = vec![ + ModelInfo::new("Foo").with_vision_support(false), + ModelInfo::new("foo").with_vision_support(true), + ]; + for (name, expected) in [("Foo", false), ("foo", true), ("FOO", false)] { + let body = image_request(models.clone(), ModelConfig::new(name), endpoint).await; + assert_images(&body, expected); + } + + let body = image_request( + vec![ModelInfo::new("Foo").with_vision_support(true)], + ModelConfig::new("foo").with_vision_support(false), + endpoint, + ) + .await; + assert_images(&body, false); + } +} diff --git a/crates/goose/src/config/declarative_providers.rs b/crates/goose/src/config/declarative_providers.rs index 10fe48e6c4..76c5209402 100644 --- a/crates/goose/src/config/declarative_providers.rs +++ b/crates/goose/src/config/declarative_providers.rs @@ -307,6 +307,7 @@ pub fn update_custom_provider(params: UpdateCustomProviderParams) -> Result<()> model.supports_cache_control = model .supports_cache_control .or(existing.supports_cache_control); + model.supports_vision = model.supports_vision.or(existing.supports_vision); model.reasoning |= existing.reasoning; model.thinking_preservation_format = model .thinking_preservation_format @@ -611,6 +612,7 @@ mod tests { currency: None, supports_cache_control: None, reasoning: false, + supports_vision: None, thinking_preservation_format: None, request_params: None, }], @@ -799,7 +801,8 @@ mod tests { let temp_root = temp_dir.path().display().to_string(); let _guard = env_lock::lock_env([("GOOSE_PATH_ROOT", Some(temp_root.as_str()))]); - let mut model = ModelInfo::with_cost("large-model", 1_048_576, 0.000002, 0.000006); + let mut model = ModelInfo::with_cost("large-model", 1_048_576, 0.000002, 0.000006) + .with_vision_support(true); model.request_params = Some(HashMap::from([( "temperature".to_string(), serde_json::json!(0.25), @@ -821,23 +824,35 @@ mod tests { }) .unwrap(); - update_custom_provider(UpdateCustomProviderParams { - id: created.name.clone(), - engine: "openai".to_string(), - display_name: created.display_name.clone(), - api_url: created.base_url.clone(), - api_key: None, - models: vec![ModelInfo::new("large-model").with_context_limit(2_097_152)], - supports_streaming: Some(true), - headers: None, - requires_auth: false, - catalog_provider_id: None, - base_path: None, - toolshim: false, - preserves_thinking: None, - auth: None, - }) - .unwrap(); + for (declared, expected) in [ + (None, true), + (Some(false), false), + (None, false), + (Some(true), true), + ] { + let mut replacement = ModelInfo::new("large-model").with_context_limit(2_097_152); + replacement.supports_vision = declared; + update_custom_provider(UpdateCustomProviderParams { + id: created.name.clone(), + engine: "openai".to_string(), + display_name: created.display_name.clone(), + api_url: created.base_url.clone(), + api_key: None, + models: vec![replacement], + supports_streaming: Some(true), + headers: None, + requires_auth: false, + catalog_provider_id: None, + base_path: None, + toolshim: false, + preserves_thinking: None, + auth: None, + }) + .unwrap(); + + let loaded = load_provider(&created.name).unwrap(); + assert_eq!(loaded.config.models[0].supports_vision, Some(expected)); + } let loaded = load_provider(&created.name).unwrap(); let model = &loaded.config.models[0]; diff --git a/crates/goose/src/providers/provider_registry.rs b/crates/goose/src/providers/provider_registry.rs index d297c0eea5..eed95333a7 100644 --- a/crates/goose/src/providers/provider_registry.rs +++ b/crates/goose/src/providers/provider_registry.rs @@ -1,5 +1,8 @@ use super::api_client::TlsConfig; -use super::base::{ConfigKey, ModelInfo, Provider, ProviderDef, ProviderMetadata, ProviderType}; +use super::base::{ + find_declared_model, ConfigKey, ModelInfo, Provider, ProviderDef, ProviderMetadata, + ProviderType, +}; use super::inventory::{InventoryIdentityInput, InventoryRegistration, InventoryResolvers}; use crate::config::{DeclarativeProviderConfig, ExtensionConfig}; use anyhow::Result; @@ -69,6 +72,14 @@ impl ProviderEntry { if self.toolshim_enabled(model.toolshim) { model = model.with_toolshim(true); } + // Declared model metadata is authoritative over canonical detection, which + // may already have populated supports_vision earlier in materialization. + if let Some(supports_vision) = + find_declared_model(&self.metadata.known_models, &model.model_name) + .and_then(|m| m.supports_vision) + { + model.supports_vision = Some(supports_vision); + } crate::model_config::materialize_model_config(&self.metadata.name, model) } @@ -456,4 +467,92 @@ mod tests { assert!(registry.entries["custom_default"].toolshim_enabled(true)); assert!(!registry.entries["custom_default"].toolshim_enabled(false)); } + + #[test] + fn canonical_metadata_preserves_explicit_vision_overrides() { + let mut registry = ProviderRegistry::new(None); + registry.register::(false); + let entry = ®istry.entries["openai"]; + + for (name, canonical_vision) in [("gpt-4o", true), ("gpt-4", false)] { + let detected = entry + .normalize_model_config(ModelConfig::new(name)) + .unwrap(); + assert_eq!(detected.supports_vision, Some(canonical_vision)); + + let overridden = entry + .normalize_model_config( + ModelConfig::new(name).with_vision_support(!canonical_vision), + ) + .unwrap(); + assert_eq!(overridden.supports_vision, Some(!canonical_vision)); + } + } + + #[test] + fn case_collisions_do_not_change_the_selected_models_vision() { + let mut registry = ProviderRegistry::new(None); + let mut config = test_config(); + config.models = vec![ + ModelInfo::new("Foo").with_vision_support(false), + ModelInfo::new("foo").with_vision_support(true), + ModelInfo::new("Bar").with_vision_support(true), + ]; + registry.register_with_name::( + &config, + ProviderType::Custom, + false, + |_| unreachable!("constructor is not used by this test"), + || Ok(InventoryIdentityInput::new("custom_hf", "custom_hf")), + ); + let entry = ®istry.entries["custom_hf"]; + + for (name, expected) in [ + ("Foo", Some(false)), + ("foo", Some(true)), + ("FOO", None), + ("bar", None), + ] { + let model = entry + .normalize_model_config(ModelConfig::new(name)) + .unwrap(); + assert_eq!(model.supports_vision, expected); + } + } + + #[test] + fn declared_supports_vision_overrides_detected_value() { + let mut registry = ProviderRegistry::new(None); + let mut config = test_config(); + config.models = vec![ + ModelInfo::new("vision-model").with_vision_support(true), + ModelInfo::new("text-model").with_vision_support(false), + ModelInfo::new("undeclared-model"), + ]; + registry.register_with_name::( + &config, + ProviderType::Custom, + false, + |_| unreachable!("constructor is not used by this test"), + || Ok(InventoryIdentityInput::new("custom_hf", "custom_hf")), + ); + let entry = ®istry.entries["custom_hf"]; + + let mut detected_without_vision = ModelConfig::new("vision-model"); + detected_without_vision.supports_vision = Some(false); + let mut detected_with_vision = ModelConfig::new("text-model"); + detected_with_vision.supports_vision = Some(true); + + let vision = entry + .normalize_model_config(detected_without_vision) + .unwrap(); + let text = entry.normalize_model_config(detected_with_vision).unwrap(); + let undeclared = entry + .normalize_model_config(ModelConfig::new("undeclared-model")) + .unwrap(); + + assert_eq!(vision.supports_vision, Some(true)); + assert_eq!(text.supports_vision, Some(false)); + assert_eq!(undeclared.supports_vision, None); + } } diff --git a/crates/goose/tests/declared_vision_registry.rs b/crates/goose/tests/declared_vision_registry.rs new file mode 100644 index 0000000000..883a10a67c --- /dev/null +++ b/crates/goose/tests/declared_vision_registry.rs @@ -0,0 +1,95 @@ +use goose::config::declarative_providers::{create_custom_provider, CreateCustomProviderParams}; +use goose::conversation::message::Message; +use goose::providers::{get_from_registry, refresh_custom_providers}; +use goose_providers::{base::ModelInfo, model::ModelConfig}; +use serde_json::json; +use wiremock::{ + matchers::{method, path}, + Mock, MockServer, ResponseTemplate, +}; + +#[tokio::test] +async fn persisted_vision_declarations_control_registry_requests() { + let root = tempfile::tempdir().unwrap(); + let root_path = root.path().to_str().unwrap(); + let _guard = env_lock::lock_env([("GOOSE_PATH_ROOT", Some(root_path))]); + + for endpoint in ["chat/completions", "responses"] { + for (declared, configured, expected) in [ + (Some(true), false, true), + (Some(false), true, false), + (None, true, true), + (None, false, false), + ] { + let server = MockServer::start().await; + let response = if endpoint == "responses" { + json!({ + "id": "response-1", "object": "response", "created_at": 0, + "status": "completed", "model": "local-vlm", + "output": [{"type": "message", "role": "assistant", "content": [ + {"type": "output_text", "text": "Seen"} + ]}] + }) + } else { + json!({ + "id": "response-1", "model": "local-vlm", + "choices": [{"message": {"role": "assistant", "content": "Seen"}, + "finish_reason": "stop"}] + }) + }; + Mock::given(method("POST")) + .and(path(format!("/v1/{endpoint}"))) + .respond_with(ResponseTemplate::new(200).set_body_json(response)) + .expect(1) + .mount(&server) + .await; + + let mut info = ModelInfo::new("local-vlm"); + info.supports_vision = declared; + let saved = create_custom_provider(CreateCustomProviderParams { + engine: "openai".to_string(), + display_name: "Declared Vision Self Test".to_string(), + api_url: server.uri(), + api_key: None, + models: vec![info], + supports_streaming: Some(false), + headers: None, + requires_auth: false, + catalog_provider_id: None, + base_path: Some(format!("v1/{endpoint}")), + toolshim: false, + preserves_thinking: None, + auth: None, + }) + .unwrap(); + refresh_custom_providers().await.unwrap(); + let entry = get_from_registry(&saved.name).await.unwrap(); + let model = entry + .normalize_model_config( + ModelConfig::new("local-vlm").with_vision_support(configured), + ) + .unwrap(); + let provider = entry.create(vec![]).await.unwrap(); + provider + .complete( + &model, + "Describe images", + &[Message::user() + .with_text("Describe this image") + .with_image("dXNlcg==", "image/png")], + &[], + ) + .await + .unwrap(); + let requests = server.received_requests().await.unwrap(); + assert_eq!(requests.len(), 1); + let body: serde_json::Value = requests[0].body_json().unwrap(); + assert_eq!(body["model"], "local-vlm"); + assert_eq!( + body.to_string().contains("data:image/png;base64,dXNlcg=="), + expected, + "{body}" + ); + } + } +} diff --git a/goose-self-test.yaml b/goose-self-test.yaml index 3b06efbce9..121de50d9e 100644 --- a/goose-self-test.yaml +++ b/goose-self-test.yaml @@ -10,6 +10,7 @@ activities: - Validate shell command execution and error handling - Validate Azure AI Foundry provider routing and deployment aliases - Validate the configurable Databricks AI Gateway path across all inference routes + - Validate declared vision through persisted custom-provider configuration and outbound requests - Analyze code structure and parsing capabilities - Test extension discovery and management - Test load tool for knowledge injection and discovery @@ -27,7 +28,7 @@ parameters: input_type: string requirement: optional default: "all" - description: "Which test phases to run: all, basic, extensions, delegation, reasoning, zai-coding-plan, acp-effort, compaction, advanced" + description: "Which test phases to run: all, basic, extensions, delegation, declared-vision, reasoning, zai-coding-plan, acp-effort, compaction, advanced" - key: test_depth input_type: string @@ -344,6 +345,24 @@ prompt: | Log results to: {{ workspace_dir }}/phase3_delegation.md {% endif %} + {% if test_phases == "all" or "declared-vision" in test_phases %} + ## Declared Custom-Provider Vision + + When running from a goose source checkout with a Rust toolchain: + 1. Run `cargo test -p goose --features rustls-tls --test declared_vision_registry`. + 2. Verify saved custom-provider declarations survive registry reload and control actual + image forwarding on both chat completions and Responses. Declared true and false + override opposing model settings; an absent declaration preserves those settings. + 3. Run `cargo test -p goose-providers --features rustls-tls --test declared_vision`. + 4. Verify actual user and tool-result image requests follow the same precedence and + undeclared case variants never inherit another model's declaration. + These tests use a local HTTP mock and isolated configuration, requiring no credentials + or downloaded vision model. If the source checkout or toolchain is unavailable, mark + this phase as SKIPPED. + + Log results to: {{ workspace_dir }}/declared_vision.md + {% endif %} + {% if test_phases == "all" or "vision" in test_phases %} ## 📷 PHASE 3B: Local Inference Vision Testing