hard disable remote models refresh when feature is disabled#8687
hard disable remote models refresh when feature is disabled#8687aibrahim-oai merged 3 commits intomainfrom
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: 3bfe34025f
ℹ️ 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".
| if !remote_models_feature && self.auth_manager.get_auth_mode() == Some(AuthMode::ApiKey) { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
When RemoteModels is disabled (the default), refresh_if_new_etag now forwards remote_models_feature=false, but refresh_available_models_no_cache only short‑circuits when the auth mode is ApiKey. For ChatGPT auth, the guard doesn’t trigger and the code still calls list_models, so a ResponseEvent::ModelsEtag with an X-Models-Etag header will still cause a network refresh even though the feature is off. This defeats the stated goal of hard‑disabling refresh when the feature is disabled. The guard should short‑circuit on the feature flag regardless of auth mode (and still exclude ApiKey if that remains desired).
Useful? React with 👍 / 👎.
| if self.auth_manager.get_auth_mode() == Some(AuthMode::ApiKey) { | ||
| pub(crate) async fn refresh_available_models_no_cache( | ||
| &self, | ||
| remote_models_feature: bool, |
There was a problem hiding this comment.
Should this config go into model manager ctor?
There was a problem hiding this comment.
Does the value ever change mid session?
There was a problem hiding this comment.
ideally yeah but app server mutates it after construction unfortunately. Plus, we don't have access to config on construction.
There was a problem hiding this comment.
Why do you pass config into some and exact value into other refresh methods?
There was a problem hiding this comment.
just so I don't keep propagating config everywhere. will just use exact value in all methods
2 participants
We were refreshing on tag mismatch even with feature is disabled