fix(summon): re-apply canonical limits when delegate overrides model - #9183
Conversation
When the `delegate` tool overrides the subagent's model name, the parent session's ModelConfig was cloned and only the model_name string was swapped. with_canonical_limits was not re-run, so the subagent inherited the parent model's context_limit, max_tokens, reasoning flag, and predefined request_params — but ran under a completely different model. This caused real misbehavior, e.g. a parent on claude-3-5-sonnet (200k ctx / 8k output / reasoning=false) delegating to claude-opus-4-6 would still get 200k context and reasoning=false instead of the new model's 1M context and reasoning=true. Fix: when an override is present, build a fresh ModelConfig for the new model name and call with_canonical_limits for the resolved provider. Toolshim settings and fast_model_config are still inherited from the parent since those are operator-level choices, not model-level. Also extract resolve_provider_name and resolve_model_config as pure helpers so the resolution logic is unit-testable without spinning up a real provider. Tests: - test_resolve_model_config_applies_canonical_limits_to_overridden_model verifies the new model's canonical context_limit, max_tokens, and reasoning flag are applied (was the bug). - test_resolve_model_config_keeps_parent_when_no_override verifies the parent's resolved config is preserved when no override is provided. Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73472ca6c5
ℹ️ 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".
| let mut cfg = crate::model::ModelConfig::new(&model)?; | ||
| cfg.toolshim = parent_model_config.toolshim; | ||
| cfg.toolshim_model = parent_model_config.toolshim_model.clone(); | ||
| cfg.fast_model_config = parent_model_config.fast_model_config.clone(); |
There was a problem hiding this comment.
Preserve parent request_params on model override
When delegate overrides model, this branch rebuilds ModelConfig from ModelConfig::new(&model) and only copies toolshim, toolshim_model, and fast_model_config. That drops any session-level request_params previously stored in session.model_config (for example ACP-set provider params like anthropic_beta), so subagents can silently lose required provider flags even though the parent session is configured to use them. Before this commit, overriding the model kept these params because the full parent config was cloned.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1521124.
The override path now keeps the wholesale clone of the parent's ModelConfig (preserving request_params, toolshim, toolshim_model, fast_model_config) and, on a real model change, only resets the canonical fields before re-running with_canonical_limits for the new model.
Followed up in 66cb1c44a / latest revision: also rebuilds ModelConfig via ModelConfig::new(&model) so GOOSE_CONTEXT_LIMIT / GOOSE_MAX_TOKENS env vars are re-read for the overridden model, then merges the parent's request_params over the new model's predefined ones (parent wins on conflicts). New regression test test_resolve_model_config_preserves_parent_request_params_on_override asserts anthropic_beta set on the parent survives a model override.
Address PR review: when delegate overrides the model, keep the parent session's request_params, toolshim, and fast_model_config by cloning the full parent ModelConfig and resetting only the canonical fields (context_limit, max_tokens, reasoning) before re-running with_canonical_limits for the new model. This preserves explicitly-set session params (e.g. ACP-set anthropic_beta headers) while still applying the new model's canonical limits and reasoning flag. Also drops the resolve_provider_name helper which was unnecessary indirection. Adds a regression test for request_params preservation. Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
|
Addressed in 1521124. The new path keeps the wholesale clone of the parent's Also dropped the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1521124d6a
ℹ️ 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".
| model_config.context_limit = None; | ||
| model_config.max_tokens = None; | ||
| model_config.reasoning = None; | ||
| model_config = model_config.with_canonical_limits(provider_name); |
There was a problem hiding this comment.
Preserve explicit limits when overriding subagent model
Resetting context_limit and max_tokens to None before with_canonical_limits drops non-canonical limits that were intentionally set on the parent session (for example GOOSE_CONTEXT_LIMIT, GOOSE_MAX_TOKENS, or ACP-provided context_limit already stored in session.model_config). In override flows, this changes precedence from “user override wins” to “canonical wins/None”, so delegated subagents can silently ignore hard token/context caps and run with larger defaults than the caller configured.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 66cb1c44a.
The override path now builds a fresh ModelConfig::new(&model) (which re-reads GOOSE_CONTEXT_LIMIT / GOOSE_MAX_TOKENS via Config::global()), then applies with_canonical_limits for the overridden model and only fills gaps the env didn't cover. Parent-session non-canonical state (toolshim, toolshim_model, fast_model_config, request_params) is preserved.
The previous reset-to-None approach made env-level caps invisible — that's now fixed. New regression test test_resolve_model_config_honors_env_token_limits_on_override sets GOOSE_CONTEXT_LIMIT=50000 and GOOSE_MAX_TOKENS=1024 and asserts both still apply after a model override.
Codex P2: resetting context_limit/max_tokens to None before with_canonical_limits dropped GOOSE_CONTEXT_LIMIT / GOOSE_MAX_TOKENS that the user had set on the parent session. Override flows silently ignored hard caps and ran with larger canonical defaults. Rebuild ModelConfig via ModelConfig::new(&model) on override so env limits are re-read, then preserve parent session's non-canonical state (toolshim, toolshim_model, fast_model_config) and merge parent request_params over the new model's predefined ones. Tests refactored with small helpers (parent_config, session_with, resolve_with_override) to remove boilerplate. ~210 lines -> ~140 lines of tests with the same coverage, plus a new regression test test_resolve_model_config_honors_env_token_limits_on_override. Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 712825405a
ℹ️ 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".
Codex P2: rebuilding ModelConfig via ModelConfig::new(&model) only picks up GOOSE_TEMPERATURE from env, so a parent session's persisted/custom temperature was silently dropped on override. Fall back to the parent's temperature when neither env nor predefined config sets one for the new model. cfg.temperature = cfg.temperature.or(parent.temperature) Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com>
…limits-on-model-override
Remove test_resolve_model_config_keeps_parent_when_no_override (tests the no-op path) and test_resolve_model_config_honors_env_token_limits_on_override (tests ModelConfig::new reads env vars, not the override logic itself). Signed-off-by: Douwe Osinga <douwe@squareup.com>
DOsinga
left a comment
There was a problem hiding this comment.
LGTM. Real bug fix — the old code swapped model_name without re-deriving canonical limits, so subagents inherited the parent model's context_limit/max_tokens/reasoning. The fix correctly rebuilds from scratch for the overridden model while preserving session-level state.
I dropped the two trivial tests (no-op path and env-var-reading) and kept the two meaningful ones (canonical limits regression + request_params preservation). Tests pass, clippy clean.
* main: (66 commits) Switch GH pages deploy to actions/artifact workflow (#9025) fix(summon): re-apply canonical limits when delegate overrides model (#9183) Split code signing from build (#8587) refactor(logging): consolidate logging setup into shared helper in goose crate (#8817) fix(cli): report cumulative total_tokens in stream-json/json output (#8910) plugins: add open plugins (just skills for now) (#9063) fix(providers): refresh GCP metadata server token on expiration (#8929) chore(deps): bump the cargo-minor-and-patch group across 1 directory with 14 updates (#9178) chore(deps): bump bzip2 from 0.5.2 to 0.6.1 (#8964) chore(deps): bump tauri from 2.10.3 to 2.11.1 in /ui/goose2/src-tauri (#9066) chore(deps): bump hono from 4.12.14 to 4.12.18 in /evals/open-model-gym/mcp-harness (#9073) localize hardcoded strings in provider settings UI (#8931) chore(deps): bump @babel/plugin-transform-modules-systemjs from 7.28.5 to 7.29.4 in /documentation (#9122) move settings into app shell (#9047) Add Location column to CLI skills table (#8785) (feat): add routstr as a declarative provider (#9175) Add FuturMix provider (#8840) fix: convert quoted numeric config values to numbers if needed (#8844) fix(ui): keep SSE reconnect loop alive on long disconnects (#8717) (#8846) fix(openai): apply request_params to outgoing API payload (#9151) ...
* main: (102 commits) Dynamically refresh skill instructions each turn (#9217) Build non-vulkan linux variants using ubuntu 22.04 (#9211) fix(ui): show tool name in approval prompt (#9216) feat: add Atomic Chat as declarative OpenAI-compatible provider (#9210) chore: bump package.json versions from 0.19.1 to 0.20.0 (#9218) feat: support GOOSE_OAUTH_CALLBACK_PORT for stable OAuth redirect_uri (#9209) [RFC] feat(oauth): proactive token refresh to avoid re-auth on every session (#8386) fix: resolve Azure CLI on Windows by using az.cmd (#9215) fix: handle non-interactive terminal in goose configure on Windows (#9214) Better parsing of pasted html as markdown so agents understand (#9190) fix: persist accumulated cost in session DB to survive reload (#9191) fix(publish-npm): build binary from current SHA + add compat check (#9212) feat(desktop): add goose://new-session deep link to open fresh chat (#9196) Add PR previews using cloudflare pages (#9208) fix: prevent tool-use marker leakage in toolshim output (#8310) Prompt injection mitigation: update pattern-based detection (#9198) remove goose2 related skills (#9189) Switch GH pages deploy to actions/artifact workflow (#9025) fix(summon): re-apply canonical limits when delegate overrides model (#9183) Split code signing from build (#8587) ...
…aif-goose#9183) Signed-off-by: Kyle De Freitas <kdefreitas@squareup.com> Signed-off-by: Douwe Osinga <douwe@squareup.com> Co-authored-by: Douwe Osinga <douwe@squareup.com>
Summary
When the
delegatetool in the summon platform extension overrides the model for a subagent (via themodelparameter,recipe.settings.goose_model, or theGOOSE_SUBAGENT_MODELenv var), the canonical limits for the new model were not applied. The subagent kept running with the parent session'scontext_limit,max_tokens, andreasoningflag, just with a differentmodel_namestring.That happened because
resolve_providercloned the parent'sModelConfig, mutatedmodel_namein place, and never re-ranwith_canonical_limits.Concrete impact: if you started a session with, say,
claude-3-5-sonnet(200k ctx, 8k output, reasoning=false) and delegated toclaude-opus-4-6(1M ctx, 128k output, reasoning=true), the subagent was capped at the sonnet limits and had reasoning disabled.Fix
Split provider/model resolution into a pure
resolve_model_confighelper. When an override is present, construct a freshModelConfigfor the new model and callwith_canonical_limitson it, while preserving parent settings that are not derived from the canonical lookup (toolshim,toolshim_model,fast_model_config). When there is no override, behavior is unchanged.Tests
test_resolve_model_config_applies_canonical_limits_to_overridden_model— regression test. Verified it fails on the buggy version (parent'scontext_limit=200_000leaks into aclaude-opus-4-6subagent that should be1_000_000) and passes after the fix.test_resolve_model_config_keeps_parent_when_no_override— guards the no-override path.Both use
env_lockto isolateGOOSE_CONTEXT_LIMIT/GOOSE_MAX_TOKENS/GOOSE_SUBAGENT_MODEL.Verification
cargo fmtcargo clippy --all-targets -- -D warnings— cleancargo test -p goose— no new failures (pre-existing failures onmainare unchanged)