fix: don't bleed provider-specific request_params into delegate() sessions - #9906
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e8e28fb0e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for k in ["thinking_effort"] { | ||
| if let Some(v) = parent_params.get(k).cloned() { | ||
| let merged = cfg.request_params.get_or_insert_with(Default::default); | ||
| merged.entry(k.to_string()).or_insert(v); |
There was a problem hiding this comment.
When a global/default GOOSE_THINKING_EFFORT is set and the parent session has changed its own thinking effort, model_config_from_user_config() has already seeded cfg.request_params with the default before this merge. Because this uses or_insert, delegate(..., model: ...) ignores the parent session's thinking_effort and runs the subagent with the stale/default effort instead; the normal model-switch path builds without defaults first so the session value wins. For this whitelisted inherited key, overwrite the existing default rather than preserving it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think this might actually be a useful side effect rather than a regression.
There was a problem hiding this comment.
Fixed by routing the override through model_config_from_user_config_with_session_settings, which inherits thinking_effort from the parent with correct precedence (explicit child param > parent session > global default) rather than the old or_insert. A parent that raised its effort above the global default now wins; added test_resolve_model_config_inherits_thinking_effort_on_override to cover it.
kimnamu
left a comment
kimnamu
left a comment
There was a problem hiding this comment.
Thanks for this fix, @kdefreitas — splitting model-family-agnostic keys from provider-specific ones is the right model, and the Databricks parameters correction looks solid. I'm not a maintainer, just another Bedrock/provider user who reviewed this carefully and ran the branch locally. I wanted to add concrete evidence to the open thread between the Codex bot's P2 and @kyledef's reply, because I think it tips the decision.
The Codex P2 is a real regression — reproduced locally. With a global GOOSE_THINKING_EFFORT=low set and a parent session that explicitly raised its own effort to high, the override path now runs the child at low:
REPRO resolved thinking_effort = Some(String("low")) # left = PR behavior
expected Some(String("high")) # right = parent's value
assertion failed: child should inherit parent's raised thinking_effort
Root cause: this override path builds cfg via model_config_from_user_config(provider, &model) -> materialize_model_config(include_default_thinking_effort = true), so when GOOSE_THINKING_EFFORT is set, with_default_thinking_effort() has already seeded cfg.request_params["thinking_effort"] with the global default. The .or_insert(v) then sees the key present and drops the parent's explicit value. The non-delegate model-switch path (model_config_from_user_config_with_session_settings, built with include_default_thinking_effort = false) inherits before applying the default, so the session value wins there — the two paths diverge.
Re @kyledef's "useful side effect": when the parent has no explicit effort, overwriting changes nothing (there's no key to copy, and the default is preserved — verified below). The only case affected is when a user deliberately raised effort on the parent, which is exactly the case where their intent should carry into the subagent. So switching to overwrite preserves the default-when-absent behavior and only fixes the lost-explicit-value case.
The minimal fix (Codex's suggestion) — overwrite for the whitelisted key, at summon.rs:1628:
merged.insert(k.to_string(), v);With that one-line change both cases pass locally, and the existing test_resolve_model_config_* tests stay green:
repro_thinking_effort_default_shadows_parent ... high (parent wins)
repro_thinking_effort_default_kept_when_parent_absent ... low (default kept)
test_resolve_model_config_inherits_thinking_effort_on_override ... ok
test_resolve_model_config_does_not_inherit_provider_specific_request_params ... ok
If it's useful, here's a regression test you could drop next to the existing ones (fails on the current or_insert, passes with insert):
#[tokio::test]
#[serial]
async fn test_resolve_model_config_parent_thinking_effort_overrides_global_default() {
let _env = env_lock::lock_env([
("GOOSE_CONTEXT_LIMIT", None::<&str>),
("GOOSE_MAX_TOKENS", None::<&str>),
("GOOSE_SUBAGENT_MODEL", None::<&str>),
("GOOSE_THINKING_EFFORT", Some("low")),
]);
let mut parent = parent_config();
parent.request_params = Some(HashMap::from([(
"thinking_effort".to_string(),
serde_json::json!("high"),
)]));
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")),
"parent's explicit thinking_effort should win over the global default on override"
);
}One thing I checked and is not a problem: the ["thinking_effort"] allowlist isn't too narrow. thinking_effort is the only key with cross-family inheritance semantics elsewhere (ModelConfig::with_inherited_session_settings_from inherits exactly this one key across models), so limiting the allowlist to it matches the existing convention — nice and consistent.
Both fixes are clean and well-scoped; with the insert tweak I'd love to see this land.
— Reviewed with the help of an AI agent (Claude Code); I ran the branch and the reproduction above locally and verified the results myself before posting.
| for k in ["thinking_effort"] { | ||
| if let Some(v) = parent_params.get(k).cloned() { | ||
| let merged = cfg.request_params.get_or_insert_with(Default::default); | ||
| merged.entry(k.to_string()).or_insert(v); |
There was a problem hiding this comment.
Verified locally: with a global GOOSE_THINKING_EFFORT default set, cfg is already seeded here, so .or_insert drops the parent's explicit value (the child ran low instead of the parent's high). Overwriting for this whitelisted key fixes it and keeps the default-when-parent-absent case intact (confirmed both directions pass).
| merged.entry(k.to_string()).or_insert(v); | |
| merged.insert(k.to_string(), v); |
|
Thanks for chasing this down, Kyle — the analysis is solid and the two bugs are real. The On the
More fundamentally: So my suggestion: pull the databricks fix into its own PR, and rework the summon side to go through the existing session-settings helper (ideally with an explicit |
…n-settings helper delegate() with a model override previously built the child ModelConfig from scratch and hand-copied model-agnostic state back from the parent, then merged parent request_params with a whitelist loop. That was fragile (every new model-agnostic field had to be remembered here) and had a precedence bug: with a global GOOSE_THINKING_EFFORT default set, model_config_from_user_config() seeded request_params before the merge, so the or_insert kept the default and dropped a parent session's explicitly-raised thinking_effort. Route the override through model_config_from_user_config_with_session_settings instead. It materializes model-specific fields for the new model and inherits thinking_effort from the parent with the correct precedence (explicit child param > parent session > global default), while *not* carrying provider-specific request_params (e.g. anthropic_beta) into a child targeting a different model family — which would otherwise trigger a 400 INVALID_ARGUMENT. Addresses review feedback (@DOsinga, @kimnamu, codex). The self-contained Databricks tool-format fix that was previously bundled here is split into its own PR.
4e8e28f to
2f8b474
Compare
|
Done on both counts. Split the self-contained Databricks |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f8b474763
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| provider_name, | ||
| &model, | ||
| Some(&parent), | ||
| None, |
There was a problem hiding this comment.
When delegate() overrides the model but stays on a compatible provider/model family (for example, an ACP/predefined Anthropic/Databricks Claude session carrying supported params such as budget_tokens or preserve_thinking_context and delegating to another Claude model), passing None here makes with_inherited_session_settings_from keep only thinking_effort and drop the rest of the parent request_params. Those keys are consumed by the provider formatters to shape the request, so subagents silently lose required session/request settings even though no cross-family Gemini bleed is happening; please filter only incompatible params or rebuild the target model's own predefined params instead of discarding all non-thinking_effort params.
Useful? React with 👍 / 👎.
Generalize with_inherited_session_settings_from to carry the set of model-family-agnostic reasoning params (thinking_effort, budget_tokens, preserve_thinking_context, etc.) across a model switch or subagent delegation, while still dropping provider-specific keys like anthropic_beta. Addresses the codex review note that overriding to a same-family model previously dropped budget_tokens and related settings.
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
Pushed a follow-up commit (fb2fff0) to address the codex P2 note about same-family overrides dropping reasoning params.
The override path now inherits a defined allowlist of model-family-agnostic reasoning controls — thinking_effort, thinking_budget, budget_tokens, enable_thinking, preserve_thinking_context, preserve_unsigned_thinking — across a model switch / subagent delegation, while still dropping provider-specific keys like anthropic_beta so they can't bleed into a different model family. These are exactly the keys the provider formatters read out of request_params. The change lives in with_inherited_session_settings_from, so the two ACP model-switch call sites benefit too. Added test coverage in both model.rs and summon.rs.
Thanks for the clean rework of the summon path through the canonical helper — LGTM.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb2fff04dc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "thinking_budget", | ||
| "budget_tokens", | ||
| "enable_thinking", | ||
| "preserve_thinking_context", | ||
| "preserve_unsigned_thinking", |
There was a problem hiding this comment.
When a parent session carries provider-specific reasoning params such as budget_tokens, thinking_budget, or preserve_thinking_context and delegate()/ACP switches to an incompatible target, this whitelist now copies those keys into the child config. Several target formatters then forward unknown request params (for example OpenAI chat and Databricks forward everything except thinking_effort), so a Claude parent with a custom budget can still make a Gemini/OpenAI child fail with invalid top-level fields—the same class of cross-family bleed this change is trying to avoid. Please filter these params by the resolved target provider/model instead of treating the whole list as globally safe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fcc006a030
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const INHERITED_SESSION_PARAM_KEYS: &[&str] = &[ | ||
| "thinking_effort", | ||
| "thinking_budget", | ||
| "budget_tokens", |
There was a problem hiding this comment.
Fresh evidence in this revision is that budget_tokens is now explicitly included in the global inherited whitelist. When a Claude/Anthropic parent with budget_tokens delegates or switches to a non-Claude target such as Databricks Gemini or OpenAI chat, with_inherited_session_settings_from copies that key into the child config, and those formatters forward unknown request params while only skipping thinking_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 👍 / 👎.
3 participants
Problem
When
delegate()(the summon extension) overrides the model to a different model family — e.g. the parent session is a Claude model (Anthropic) and the child isgoose-gemini-3-5-flash(Google) — two bugs cause the Databricks Gemini endpoint to reject the request with400 INVALID_ARGUMENT.Bug 1 —
anthropic_betableeds into the child session (summon.rs)resolve_model_config()unconditionally merged all of the parentModelConfig.request_paramsinto the child config when a model override was specified. The parent session's model (e.g.goose-claude-4-6-sonnet) hasanthropic_betain itsrequest_params(populated fromGOOSE_PREDEFINED_MODELSin the app environment). That key was blindly forwarded to the Gemini request body, which the endpoint rejected.Fix: Only inherit the model-family-agnostic key
thinking_effortfrom parentrequest_params. Provider-specific keys likeanthropic_betaare derived fromGOOSE_PREDEFINED_MODELSatModelConfig::new()time for the specific target model and must not be carried over from a different model family.Bug 2 —
parametersJsonSchemaused for Gemini on Databricks (formats/databricks.rs)format_tools()detected"gemini"in the model name and emittedparametersJsonSchema(the Google-native Vertex/Gemini API field) instead ofparameters. However, Databricks serving endpoints — including those backed by Gemini — use the OpenAI-compatible chat completions format, which expectsparameters. The branch was incorrect and caused every tool definition to be rejected.Fix: Remove the model-name branch entirely. Databricks tools always use
parameters.Changes
crates/goose/src/agents/platform_extensions/summon.rs: Only inheritthinking_effortfrom parentrequest_paramson model override. Updated tests: renamed existing test to assert the correct (fixed) behaviour, added a new test assertingthinking_effortis still inherited.crates/goose/src/providers/formats/databricks.rs: Removedis_geminibranch informat_tools(); always useparameters. Updated test assertions accordingly.Testing
cargo test -p goose --lib providers::formats::databricks::tests::test_format_tools— passescargo clippy -p goose --all-targets -- -D warnings— clean (pre-existing errors in unrelated files only)cargo fmt— clean