From d873c91234ad824ab2f5cff9256488d1c80fa580 Mon Sep 17 00:00:00 2001 From: Kyle E DeFreitas Date: Fri, 3 Jul 2026 12:23:26 -0400 Subject: [PATCH] fix: don't bleed provider-specific request_params into delegate() sessions (#9906) Co-authored-by: Douwe M Osinga --- crates/goose-provider-types/src/model.rs | 55 ++++++++---- .../src/agents/platform_extensions/summon.rs | 88 ++++++++++++++++--- 2 files changed, 110 insertions(+), 33 deletions(-) diff --git a/crates/goose-provider-types/src/model.rs b/crates/goose-provider-types/src/model.rs index 6aa9be4859..341beaea99 100644 --- a/crates/goose-provider-types/src/model.rs +++ b/crates/goose-provider-types/src/model.rs @@ -8,6 +8,19 @@ use utoipa::ToSchema; pub const DEFAULT_CONTEXT_LIMIT: usize = 128_000; +/// Request param keys that describe model-family-agnostic reasoning behavior and +/// are therefore safe to carry across a model switch or subagent delegation. +/// Provider-specific keys (e.g. `anthropic_beta`) are deliberately excluded so +/// they can't bleed into a request targeting a different model family. +const INHERITED_SESSION_PARAM_KEYS: &[&str] = &[ + "thinking_effort", + "thinking_budget", + "budget_tokens", + "enable_thinking", + "preserve_thinking_context", + "preserve_unsigned_thinking", +]; + #[derive(Debug, Clone, Serialize, ToSchema)] pub struct ModelConfig { pub model_name: String, @@ -187,22 +200,13 @@ impl ModelConfig { previous: Option<&ModelConfig>, request_params: Option>, ) -> Self { - if let Some(previous) = previous { - let has_thinking_effort = self - .request_params - .as_ref() - .and_then(|params| params.get("thinking_effort")) - .is_some(); - - if !has_thinking_effort { - if let Some(thinking_effort) = previous - .request_params - .as_ref() - .and_then(|params| params.get("thinking_effort")) - .cloned() - { - let params = self.request_params.get_or_insert_with(HashMap::new); - params.insert("thinking_effort".to_string(), thinking_effort); + if let Some(previous_params) = previous.and_then(|p| p.request_params.as_ref()) { + for key in INHERITED_SESSION_PARAM_KEYS { + if let Some(value) = previous_params.get(*key) { + self.request_params + .get_or_insert_with(HashMap::new) + .entry(key.to_string()) + .or_insert_with(|| value.clone()); } } } @@ -365,15 +369,28 @@ mod tests { } #[test] - fn does_not_preserve_unrelated_request_params() { + fn inherits_reasoning_controls_but_not_provider_specific_params() { let previous = config_with_params( "previous", - HashMap::from([("provider_specific".to_string(), serde_json::json!("old"))]), + HashMap::from([ + ("budget_tokens".to_string(), serde_json::json!(8192)), + ( + "preserve_thinking_context".to_string(), + serde_json::json!(true), + ), + ("anthropic_beta".to_string(), serde_json::json!("beta")), + ]), ); let config = ModelConfig::new("next") .with_inherited_session_settings_from(Some(&previous), None); - assert!(config.request_params.is_none()); + let params = config.request_params.expect("reasoning controls inherited"); + assert_eq!(params.get("budget_tokens"), Some(&serde_json::json!(8192))); + assert_eq!( + params.get("preserve_thinking_context"), + Some(&serde_json::json!(true)) + ); + assert_eq!(params.get("anthropic_beta"), None); } #[test] diff --git a/crates/goose/src/agents/platform_extensions/summon.rs b/crates/goose/src/agents/platform_extensions/summon.rs index 0e128cf4c1..7e3dfcc29a 100644 --- a/crates/goose/src/agents/platform_extensions/summon.rs +++ b/crates/goose/src/agents/platform_extensions/summon.rs @@ -1609,23 +1609,29 @@ impl SummonClient { if let Some(model) = override_model { if model != model_config.model_name { - // Build the new config from scratch so canonical fields - // (context_limit, max_tokens, reasoning) and env-derived - // overrides (GOOSE_CONTEXT_LIMIT, GOOSE_MAX_TOKENS) match the - // overridden model, then preserve session-level state that is - // not model-specific from the parent. + // Build the overridden config through the canonical session-settings + // path. This materializes model-specific fields (context_limit, + // max_tokens, reasoning) and env overrides for the *new* model, and + // inherits only model-family-agnostic session state from the parent: + // reasoning controls like `thinking_effort` and `budget_tokens` carry + // over (with the child > parent > global-default precedence the helper + // applies), while provider-specific request_params such as + // `anthropic_beta` are dropped so they can't bleed into a child + // targeting a different model family and trigger a 400 INVALID_ARGUMENT. let parent = model_config; let mut cfg = - crate::model_config::model_config_from_user_config(provider_name, &model)?; + crate::model_config::model_config_from_user_config_with_session_settings( + provider_name, + &model, + Some(&parent), + None, + None, + )?; + // Remaining model-agnostic session settings the helper doesn't + // touch, copied from the parent explicitly. cfg.toolshim = parent.toolshim; cfg.toolshim_model = parent.toolshim_model; cfg.temperature = cfg.temperature.or(parent.temperature); - if let Some(parent_params) = parent.request_params { - let merged = cfg.request_params.get_or_insert_with(Default::default); - for (k, v) in parent_params { - merged.insert(k, v); - } - } model_config = cfg; } } @@ -2616,13 +2622,17 @@ You review code."#; #[tokio::test] #[serial] - async fn test_resolve_model_config_preserves_parent_request_params_on_override() { + async fn test_resolve_model_config_does_not_inherit_provider_specific_request_params() { let _env = env_lock::lock_env([ ("GOOSE_CONTEXT_LIMIT", None::<&str>), ("GOOSE_MAX_TOKENS", None::<&str>), ("GOOSE_SUBAGENT_MODEL", None::<&str>), ]); + // Parent session is a Claude model with anthropic_beta in request_params. + // When delegate() overrides to a different model (e.g. Gemini), provider- + // specific params like anthropic_beta must not bleed through — they would + // cause a 400 INVALID_ARGUMENT from the target API. let mut parent = parent_config(); parent.request_params = Some(HashMap::from([( "anthropic_beta".to_string(), @@ -2636,7 +2646,57 @@ You review code."#; .request_params .as_ref() .and_then(|p| p.get("anthropic_beta")), - Some(&serde_json::json!("custom-beta-header")), + None, + "anthropic_beta must not be inherited by a child session with a different model" + ); + } + + #[tokio::test] + #[serial] + async fn test_resolve_model_config_inherits_thinking_effort_on_override() { + let _env = env_lock::lock_env([ + ("GOOSE_CONTEXT_LIMIT", None::<&str>), + ("GOOSE_MAX_TOKENS", None::<&str>), + ("GOOSE_SUBAGENT_MODEL", None::<&str>), + ]); + + // Reasoning controls are model-family-agnostic and should be inherited, + // while provider-specific params like anthropic_beta must not. + let mut parent = parent_config(); + parent.request_params = Some(HashMap::from([ + ("thinking_effort".to_string(), serde_json::json!("high")), + ("budget_tokens".to_string(), serde_json::json!(8192)), + ( + "anthropic_beta".to_string(), + serde_json::json!("custom-beta-header"), + ), + ])); + + let resolved = resolve_with_override(Some(OVERRIDE_MODEL), parent); + + assert_eq!( + resolved + .request_params + .as_ref() + .and_then(|p| p.get("thinking_effort")), + Some(&serde_json::json!("high")), + "thinking_effort should be inherited across model families" + ); + assert_eq!( + resolved + .request_params + .as_ref() + .and_then(|p| p.get("budget_tokens")), + Some(&serde_json::json!(8192)), + "budget_tokens should be inherited across model families" + ); + assert_eq!( + resolved + .request_params + .as_ref() + .and_then(|p| p.get("anthropic_beta")), + None, + "anthropic_beta must not be inherited alongside reasoning controls" ); }