fix: prioritize predefined model context_limit over canonical registry#7888
Conversation
GOOSE_PREDEFINED_MODELS context_limit was never used when the model also existed in the canonical registry, because the canonical lookup ran first and always set context_limit to its default (e.g. 200k). Move the predefined models check before the canonical registry lookup so explicitly configured limits take priority. Fixes block#7839 Signed-off-by: Extra Small <[email protected]>
extrasmall0
force-pushed
the
fix/predefined-model-context-limit-priority
branch
from
9b4551d to
0b82cac
Compare
![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: 0b82cac67e
ℹ️ 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".
| pub fn with_canonical_limits(mut self, provider_name: &str) -> Self { | ||
| // Check predefined models first — these are explicitly configured by the | ||
| // operator/deployment and should take priority over generic canonical defaults. | ||
| if let Some(pm) = find_predefined_model(&self.model_name) { |
There was a problem hiding this comment.
Because this predefined lookup now runs before canonical defaults and matches only on model_name, a GOOSE_PREDEFINED_MODELS entry for one provider can override context_limit for a different provider that exposes the same model name (for example, gpt-4o appears under multiple providers in the canonical mappings). In multi-provider setups, this can raise the effective limit above a provider’s real window, delaying compaction and causing avoidable upstream request failures once token usage passes that provider’s actual max; the predefined match should be scoped to provider_name before applying.
Useful? React with 👍 / 👎.
`ModelConfig::new_base()` read `GOOSE_CONTEXT_LIMIT` via `std::env::var()`, which only checks environment variables. Users who set the value in config.yaml (e.g. `GOOSE_CONTEXT_LIMIT: 1000000`) had it silently ignored for the lead model. Switch to `Config::global().get_param()` which checks environment variables first (preserving existing behaviour) then falls back to config.yaml. This matches how `GOOSE_MAX_TOKENS` is already read in the same file and how `GOOSE_CONTEXT_LIMIT` is read for the worker model in `init.rs`. Partially addresses block#7839 (Option B). Complements block#7888 which fixes predefined model priority (Option A).
Signed-off-by: Douwe Osinga <[email protected]>
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
Pushed one small cleanup — removed a comment that restated what the code does. Everything else looks good. Thanks!
block#7888) Signed-off-by: Extra Small <[email protected]> Signed-off-by: Douwe Osinga <[email protected]> Co-authored-by: Douwe Osinga <[email protected]> Signed-off-by: Vincenzo Palazzo <[email protected]>
2 participants
When a model is configured via
GOOSE_PREDEFINED_MODELSwith a customcontext_limit(e.g., 1M for extended context), the value is ignored if the same model exists in the canonical registry. The canonical lookup runs first inwith_canonical_limits()and setscontext_limitto its default (200k for most models), so the predefined check never fires sincecontext_limitis alreadySome(...).This causes auto-compaction to trigger at ~160k tokens instead of the configured 1M, destroying conversation history mid-session.
The fix moves the predefined models check before the canonical registry lookup. Since predefined models represent explicit operator configuration, they should take priority over generic defaults. The env var (
GOOSE_CONTEXT_LIMIT) still has the highest priority since it is resolved innew_base()beforewith_canonical_limits()runs.Resolution chain after this fix:
GOOSE_CONTEXT_LIMITenv varnew_base(), never overwrittenGOOSE_PREDEFINED_MODELSwith_canonical_limits()Fixes #7839