-
Notifications
You must be signed in to change notification settings - Fork 6k
fix: don't bleed provider-specific request_params into delegate() sessions #9906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2f8b474
bc9b6e6
fb2fff0
fcc006a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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", | ||
|
Comment on lines
+17
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a parent session carries provider-specific reasoning params such as Useful? React with 👍 / 👎. |
||
| ]; | ||
|
|
||
| #[derive(Debug, Clone, Serialize, ToSchema)] | ||
| pub struct ModelConfig { | ||
| pub model_name: String, | ||
|
|
@@ -187,22 +200,13 @@ impl ModelConfig { | |
| previous: Option<&ModelConfig>, | ||
| request_params: Option<HashMap<String, Value>>, | ||
| ) -> 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] | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| 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" | ||
| ); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fresh evidence in this revision is that
budget_tokensis now explicitly included in the global inherited whitelist. When a Claude/Anthropic parent withbudget_tokensdelegates or switches to a non-Claude target such as Databricks Gemini or OpenAI chat,with_inherited_session_settings_fromcopies that key into the child config, and those formatters forward unknown request params while only skippingthinking_effort, producing unsupported top-level fields and the same cross-family request failure this change is trying to prevent; only inherit this key when the resolved target formatter consumes it.Useful? React with 👍 / 👎.