From 82facd4ebf1c796b696c19750ead398819f764a0 Mon Sep 17 00:00:00 2001 From: Lifei Zhou Date: Tue, 3 Mar 2026 02:36:52 +1100 Subject: [PATCH] chore: openai reasoning model cleanup (#7529) --- crates/goose/src/context_mgmt/mod.rs | 1 + crates/goose/src/model.rs | 118 ++++++++++++++++-- crates/goose/src/providers/bedrock.rs | 1 + crates/goose/src/providers/databricks.rs | 2 - .../goose/src/providers/formats/databricks.rs | 27 ++-- crates/goose/src/providers/formats/openai.rs | 33 ++--- .../src/providers/formats/openai_responses.rs | 1 + ui/desktop/openapi.json | 4 + ui/desktop/src/api/types.gen.ts | 1 + .../models/subcomponents/SwitchModelModal.tsx | 6 +- 10 files changed, 142 insertions(+), 52 deletions(-) diff --git a/crates/goose/src/context_mgmt/mod.rs b/crates/goose/src/context_mgmt/mod.rs index 0f278baca6..85d9097ff4 100644 --- a/crates/goose/src/context_mgmt/mod.rs +++ b/crates/goose/src/context_mgmt/mod.rs @@ -564,6 +564,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }, max_tool_responses: None, } diff --git a/crates/goose/src/model.rs b/crates/goose/src/model.rs index 9c679f2342..ca9c03ef02 100644 --- a/crates/goose/src/model.rs +++ b/crates/goose/src/model.rs @@ -57,6 +57,8 @@ pub struct ModelConfig { /// Provider-specific request parameters (e.g., anthropic_beta headers) #[serde(default, skip_serializing_if = "Option::is_none")] pub request_params: Option>, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reasoning: Option, } impl ModelConfig { @@ -104,21 +106,22 @@ impl ModelConfig { toolshim_model, fast_model_config: None, request_params, + reasoning: None, }) } pub fn with_canonical_limits(mut self, provider_name: &str) -> Self { - if self.context_limit.is_none() || self.max_tokens.is_none() { - if let Some(canonical) = crate::providers::canonical::maybe_get_canonical_model( - provider_name, - &self.model_name, - ) { - if self.context_limit.is_none() { - self.context_limit = Some(canonical.limit.context); - } - if self.max_tokens.is_none() { - self.max_tokens = canonical.limit.output.map(|o| o as i32); - } + if let Some(canonical) = + crate::providers::canonical::maybe_get_canonical_model(provider_name, &self.model_name) + { + if self.context_limit.is_none() { + self.context_limit = Some(canonical.limit.context); + } + if self.max_tokens.is_none() { + self.max_tokens = canonical.limit.output.map(|o| o as i32); + } + if self.reasoning.is_none() { + self.reasoning = canonical.reasoning; } } @@ -275,6 +278,18 @@ impl ModelConfig { self.context_limit.unwrap_or(DEFAULT_CONTEXT_LIMIT) } + pub fn is_openai_reasoning_model(&self) -> bool { + const DATABRICKS_MODEL_NAME_PREFIXES: &[&str] = &["goose-", "databricks-"]; + const REASONING_PREFIXES: &[&str] = &["o1", "o3", "o4", "gpt-5"]; + + let base = DATABRICKS_MODEL_NAME_PREFIXES + .iter() + .find_map(|p| self.model_name.strip_prefix(p)) + .unwrap_or(&self.model_name); + + REASONING_PREFIXES.iter().any(|p| base.starts_with(p)) + } + pub fn max_output_tokens(&self) -> i32 { if let Some(tokens) = self.max_tokens { return tokens; @@ -408,4 +423,85 @@ mod tests { None ); } + + mod with_canonical_limits { + use super::*; + + #[test] + fn sets_limits_from_canonical_model() { + let config = ModelConfig::new_or_fail("gpt-4o").with_canonical_limits("openai"); + + assert_eq!(config.context_limit, Some(128_000)); + assert_eq!(config.max_tokens, Some(16_384)); + assert_eq!(config.reasoning, Some(false)); + } + + #[test] + fn does_not_override_existing_context_limit() { + let mut config = ModelConfig::new_or_fail("gpt-4o"); + config.context_limit = Some(64_000); + let config = config.with_canonical_limits("openai"); + + assert_eq!(config.context_limit, Some(64_000)); + } + + #[test] + fn does_not_override_existing_max_tokens() { + let mut config = ModelConfig::new_or_fail("gpt-4o"); + config.max_tokens = Some(1_000); + let config = config.with_canonical_limits("openai"); + + assert_eq!(config.max_tokens, Some(1_000)); + } + + #[test] + fn unknown_model_leaves_fields_none() { + let config = + ModelConfig::new_or_fail("totally-unknown-model").with_canonical_limits("openai"); + + assert_eq!(config.context_limit, None); + assert_eq!(config.max_tokens, None); + assert_eq!(config.reasoning, None); + } + } + + mod is_openai_reasoning_model { + use super::*; + + #[test] + fn bare_reasoning_models() { + assert!(ModelConfig::new_or_fail("o1").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("o1-preview").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("o3").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("o3-mini").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("o4-mini").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("gpt-5").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("gpt-5-3-codex").is_openai_reasoning_model()); + } + + #[test] + fn goose_prefixed_reasoning_models() { + assert!(ModelConfig::new_or_fail("goose-o3-mini").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("goose-o4-mini").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("goose-gpt-5").is_openai_reasoning_model()); + } + + #[test] + fn databricks_prefixed_reasoning_models() { + assert!(ModelConfig::new_or_fail("databricks-o3-mini").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("databricks-o4-mini").is_openai_reasoning_model()); + assert!(ModelConfig::new_or_fail("databricks-gpt-5").is_openai_reasoning_model()); + } + + #[test] + fn non_reasoning_models() { + assert!(!ModelConfig::new_or_fail("claude-sonnet-4").is_openai_reasoning_model()); + assert!(!ModelConfig::new_or_fail("gpt-4o").is_openai_reasoning_model()); + assert!( + !ModelConfig::new_or_fail("databricks-claude-sonnet-4").is_openai_reasoning_model() + ); + assert!(!ModelConfig::new_or_fail("goose-claude-sonnet-4").is_openai_reasoning_model()); + assert!(!ModelConfig::new_or_fail("llama-3-70b").is_openai_reasoning_model()); + } + } } diff --git a/crates/goose/src/providers/bedrock.rs b/crates/goose/src/providers/bedrock.rs index 845ca22b39..66d13d4454 100644 --- a/crates/goose/src/providers/bedrock.rs +++ b/crates/goose/src/providers/bedrock.rs @@ -429,6 +429,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }, retry_config: RetryConfig::default(), name: "aws_bedrock".to_string(), diff --git a/crates/goose/src/providers/databricks.rs b/crates/goose/src/providers/databricks.rs index 728d539a80..f5c764940a 100644 --- a/crates/goose/src/providers/databricks.rs +++ b/crates/goose/src/providers/databricks.rs @@ -46,10 +46,8 @@ pub const DATABRICKS_DEFAULT_MODEL: &str = "databricks-claude-sonnet-4"; const DATABRICKS_DEFAULT_FAST_MODEL: &str = "databricks-claude-haiku-4-5"; pub const DATABRICKS_KNOWN_MODELS: &[&str] = &[ "databricks-claude-sonnet-4-5", - "databricks-claude-3-7-sonnet", "databricks-meta-llama-3-3-70b-instruct", "databricks-meta-llama-3-1-405b-instruct", - "databricks-dbrx-instruct", ]; pub const DATABRICKS_DOC_URL: &str = diff --git a/crates/goose/src/providers/formats/databricks.rs b/crates/goose/src/providers/formats/databricks.rs index a7598033e4..3989358599 100644 --- a/crates/goose/src/providers/formats/databricks.rs +++ b/crates/goose/src/providers/formats/databricks.rs @@ -545,14 +545,10 @@ pub fn create_request( } let model_name = model_config.model_name.to_string(); - let is_o1 = model_name.starts_with("o1") || model_name.starts_with("goose-o1"); - let is_o3 = model_name.starts_with("o3") || model_name.starts_with("goose-o3"); - let is_gpt_5 = model_name.starts_with("gpt-5") || model_name.starts_with("goose-gpt-5"); - let is_openai_reasoning_model = is_o1 || is_o3 || is_gpt_5; + let is_openai_reasoning_model = model_config.is_openai_reasoning_model(); let is_claude_sonnet = model_name.contains("claude-3-7-sonnet") || model_name.contains("claude-4-sonnet"); // can be goose- or databricks- - // Only extract reasoning effort for O1/O3 models let (model_name, reasoning_effort) = if is_openai_reasoning_model { let parts: Vec<&str> = model_config.model_name.split('-').collect(); let last_part = parts.last().unwrap(); @@ -568,7 +564,6 @@ pub fn create_request( ), } } else { - // For non-O family models, use the model name as is and no reasoning effort (model_config.model_name.to_string(), None) }; @@ -650,16 +645,10 @@ pub fn create_request( } } - // OpenAI reasoning models use max_completion_tokens instead of max_tokens - let key = if is_openai_reasoning_model { - "max_completion_tokens" - } else { - "max_tokens" - }; - payload - .as_object_mut() - .unwrap() - .insert(key.to_string(), json!(model_config.max_output_tokens())); + payload.as_object_mut().unwrap().insert( + "max_completion_tokens".to_string(), + json!(model_config.max_output_tokens()), + ); } // Apply cache control for Claude models to enable prompt caching @@ -1056,6 +1045,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }; let request = create_request(&model_config, "system", &[], &[], &ImageFormat::OpenAi)?; let obj = request.as_object().unwrap(); @@ -1067,7 +1057,7 @@ mod tests { "content": "system" } ], - "max_tokens": 1024 + "max_completion_tokens": 1024 }); for (key, value) in expected.as_object().unwrap() { @@ -1088,6 +1078,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }; let request = create_request(&model_config, "system", &[], &[], &ImageFormat::OpenAi)?; assert_eq!(request["reasoning_effort"], "high"); @@ -1440,6 +1431,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }; let messages = vec![ @@ -1492,6 +1484,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }; let messages = vec![Message::user().with_text("Hello")]; diff --git a/crates/goose/src/providers/formats/openai.rs b/crates/goose/src/providers/formats/openai.rs index 095a391863..c6147e853c 100644 --- a/crates/goose/src/providers/formats/openai.rs +++ b/crates/goose/src/providers/formats/openai.rs @@ -769,14 +769,9 @@ pub fn create_request( )); } - let is_ox_model = model_config.model_name.starts_with("o1") - || model_config.model_name.starts_with("o2") - || model_config.model_name.starts_with("o3") - || model_config.model_name.starts_with("o4") - || model_config.model_name.starts_with("gpt-5"); + let is_reasoning_model = model_config.is_openai_reasoning_model(); - // Only extract reasoning effort for O-series models - let (model_name, reasoning_effort) = if is_ox_model { + let (model_name, reasoning_effort) = if is_reasoning_model { let parts: Vec<&str> = model_config.model_name.split('-').collect(); let last_part = parts.last().unwrap(); @@ -791,12 +786,11 @@ pub fn create_request( ), } } else { - // For non-O family models, use the model name as is and no reasoning effort (model_config.model_name.to_string(), None) }; let system_message = json!({ - "role": if is_ox_model { "developer" } else { "system" }, + "role": if is_reasoning_model { "developer" } else { "system" }, "content": system }); @@ -822,22 +816,16 @@ pub fn create_request( } // o1, o3 models currently don't support temperature - if !is_ox_model { + if !is_reasoning_model { if let Some(temp) = model_config.temperature { payload["temperature"] = json!(temp); } } - // o1/o3 models use max_completion_tokens instead of max_tokens - let key = if is_ox_model { - "max_completion_tokens" - } else { - "max_tokens" - }; - payload - .as_object_mut() - .unwrap() - .insert(key.to_string(), json!(model_config.max_output_tokens())); + payload.as_object_mut().unwrap().insert( + "max_completion_tokens".to_string(), + json!(model_config.max_output_tokens()), + ); if for_streaming { payload["stream"] = json!(true); @@ -1500,6 +1488,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }; let request = create_request( &model_config, @@ -1518,7 +1507,7 @@ mod tests { "content": "system" } ], - "max_tokens": 1024 + "max_completion_tokens": 1024 }); for (key, value) in expected.as_object().unwrap() { @@ -1540,6 +1529,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }; let request = create_request( &model_config, @@ -1581,6 +1571,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }; let request = create_request( &model_config, diff --git a/crates/goose/src/providers/formats/openai_responses.rs b/crates/goose/src/providers/formats/openai_responses.rs index c86916b09b..56f5eeacaf 100644 --- a/crates/goose/src/providers/formats/openai_responses.rs +++ b/crates/goose/src/providers/formats/openai_responses.rs @@ -827,6 +827,7 @@ mod tests { toolshim_model: None, fast_model_config: None, request_params: None, + reasoning: None, }; let messages = vec![ diff --git a/ui/desktop/openapi.json b/ui/desktop/openapi.json index a7954b6ff8..9b6483be0c 100644 --- a/ui/desktop/openapi.json +++ b/ui/desktop/openapi.json @@ -5826,6 +5826,10 @@ "model_name": { "type": "string" }, + "reasoning": { + "type": "boolean", + "nullable": true + }, "request_params": { "type": "object", "description": "Provider-specific request parameters (e.g., anthropic_beta headers)", diff --git a/ui/desktop/src/api/types.gen.ts b/ui/desktop/src/api/types.gen.ts index a61b656916..a906f2caf6 100644 --- a/ui/desktop/src/api/types.gen.ts +++ b/ui/desktop/src/api/types.gen.ts @@ -689,6 +689,7 @@ export type ModelConfig = { context_limit?: number | null; max_tokens?: number | null; model_name: string; + reasoning?: boolean | null; /** * Provider-specific request parameters (e.g., anthropic_beta headers) */ diff --git a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx index adda5ea534..dda23fc6eb 100644 --- a/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx +++ b/ui/desktop/src/components/settings/models/subcomponents/SwitchModelModal.tsx @@ -231,7 +231,11 @@ export const SwitchModelModal = ({ if (claudeThinkingType === 'adaptive') { upsert('CLAUDE_THINKING_EFFORT', claudeThinkingEffort, false).catch(console.warn); } else if (claudeThinkingType === 'enabled') { - upsert('CLAUDE_THINKING_BUDGET', parseInt(claudeThinkingBudget, 10) || 16000, false).catch(console.warn); + upsert( + 'CLAUDE_THINKING_BUDGET', + parseInt(claudeThinkingBudget, 10) || 16000, + false + ).catch(console.warn); } }