feat: add per-model OpenAI Responses API toggle - #27683
Conversation
|
@codex review |
![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: 0421c8bc00
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
3e71f10 to
b03a721
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Documentation CheckUpdates Needed
Not flagged: Automated review via Coder Agents |
DanielleMaywood
left a comment
DanielleMaywood
left a comment
There was a problem hiding this comment.
Lots of comments are pure waffle 🧇 and can be either deleted or massively simplified.
Also a quick question: Is this definitely the simplest way to approach this? Not blocking or anything, just a net 621 lines added feels excessive. Understand that most of that appears to be tests (and comments 😜) though
| // UsesResponsesAPI reports whether a model uses the OpenAI Responses API. | ||
| // Callers must pass the same override the client was built with, otherwise | ||
| // provider options and file parts are prepared for the wrong API. Azure | ||
| // follows the SDK's model list because its provider exposes no override. |
There was a problem hiding this comment.
Trimmed to two lines in the current push.
Mux replied on Mike's behalf.
| func UsesResponsesAPI(provider, modelID string, override *bool) bool { | ||
| switch provider { | ||
| case fantasyopenai.Name: | ||
| if override != nil { | ||
| return *override | ||
| } | ||
| return fantasyopenai.IsResponsesModel(modelID) | ||
| case fantasyazure.Name: | ||
| return fantasyopenai.IsResponsesModel(modelID) | ||
| default: | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Forgive me if there is a justification, but why have we created a new function UsesResponsesAPI despite it not appearing to add any value over UsesResponsesOptions?
There was a problem hiding this comment.
They take different inputs at different stages: UsesResponsesAPI works from (provider, modelID, override) strings and is called during client construction in ModelFromConfig, before any fantasy.LanguageModel exists. UsesResponsesOptions is the wrapper for request preparation, where callers hold a model. Both are transitional: #27704 in this stack deletes the pair entirely in favor of a transport resolved once at construction and carried on the model.
Mux replied on Mike's behalf.
| // OpenAIResponsesAPIOverride returns the configured OpenAI Responses API | ||
| // override, or nil when the model config leaves the choice to the provider | ||
| // SDK's known-model list. | ||
| func OpenAIResponsesAPIOverride(config *codersdk.ChatModelOpenAIConfig) *bool { | ||
| if config == nil { | ||
| return nil | ||
| } | ||
| return config.UseResponsesAPI | ||
| } |
There was a problem hiding this comment.
Does this helper need to exist? Feels a bit unneeded
There was a problem hiding this comment.
It is a nil-safe accessor repeated at six call sites (quickgen, compaction override, generation preparer x2, advisor, aibridge routing); inlining it means repeating the nil check at each. It is also transitional: #27704 removes it together with the recomputation sites it serves.
Mux replied on Mike's behalf.
| // ApplyReasoningEffort uses the override when it must create OpenAI provider | ||
| // options, keeping their type aligned with the selected API. |
There was a problem hiding this comment.
Deleted in the current push, restoring the undocumented state this function has on main.
Mux replied on Mike's behalf.
|
|
||
| // ChatModelOpenAIConfig configures how the OpenAI client is constructed. | ||
| // Unlike ChatModelOpenAIProviderOptions, these settings are not request | ||
| // parameters: they are applied once when the client is built. |
There was a problem hiding this comment.
Shortened to two lines in the current push.
Mux replied on Mike's behalf.
| // VisibleForProviders restricts a general field to the listed raw | ||
| // provider types. Empty means the field applies to every provider. |
There was a problem hiding this comment.
Deleted in the current push; the field name and json tag carry it.
Mux replied on Mike's behalf.
| /** | ||
| * If present, the field only applies to these raw provider types. | ||
| * Absent or empty means it applies to every provider. | ||
| */ |
There was a problem hiding this comment.
Shortened to one line in the current push.
Mux replied on Mike's behalf.
| /** | ||
| * Matches against the raw provider type rather than {@link resolveProvider}, | ||
| * because aliases such as "azure" inherit another provider's request options | ||
| * but not its client construction settings. | ||
| */ |
There was a problem hiding this comment.
Shortened to two lines in the current push.
Mux replied on Mike's behalf.
| return scope.includes(provider.trim().toLowerCase()); | ||
| } | ||
|
|
||
| /** Get only the visible (non-hidden) general fields for a provider. */ |
There was a problem hiding this comment.
Deleted in the current push.
Mux replied on Mike's behalf.
b03a721 to
d1aec0a
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1aec0a96f
ℹ️ 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".
d1aec0a to
507c0b8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 507c0b80e2
ℹ️ 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".
507c0b8 to
0a0ed17
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a0ed17451
ℹ️ 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".
0a0ed17 to
6aa50d8
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aa50d8025
ℹ️ 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".
OpenAI models speak either the Responses API or Chat Completions, and the provider SDK picks per model from a static known-model list. A model absent from that list falls back to Chat Completions with no way to opt in. Add `openai_config.use_responses_api` to the model call config: unset keeps the known-model list, true forces Responses, false forces Chat Completions. It lives in a construction-time section rather than in `provider_options.openai` because it selects the API instead of being a request parameter. The transport decision must agree with the client that was built, so the override is threaded to provider option conversion, reasoning effort injection, and file part acceptance, and to every path that builds its own OpenAI client. `chatopenai.UsesResponsesAPI` is the single predicate. The option schema generator gains a `providers` struct tag, emitted as `visible_for_providers`, so the model editor renders the control only for openai-typed providers. Gating on the raw provider type keeps it out of editors for aliased types such as azure, which cannot honor it.
6aa50d8 to
323675f
Compare
|
@codex review |
3 participants
chatd hardcoded
WithUseResponsesAPI(), so the provider SDK's static known-model list decided whether an OpenAI model spoke the Responses API or Chat Completions. A model absent from that list silently fell back to Chat Completions until the fantasy fork was patched.This exposes the SDK's
WithResponsesAPIFunchook as a per-model setting,openai_config.use_responses_api, stored in the existingchat_model_configs.optionsJSONB. Unset keeps the known-model list,trueforces Responses,falseforces Chat Completions. There is no migration.It sits in a new construction-time
openai_configsection rather than inprovider_options.openaibecause it selects the API when the client is built, whileprovider_optionsholds per-request parameters. That placement is also load-bearing: a config setting only this field would otherwise materialize an OpenAI request-options struct and turn on provider-side response storage, sinceStoredefaults to true there.Three places independently decided the transport and would silently disagree with the client actually built:
ModelFromConfigAcceptsFilePartMediaTypeUsesResponsesOptionsThey share one predicate here,
chatopenai.UsesResponsesAPI, with the override threaded to each. The rest of the stack removes that threading by resolving the transport once and carrying it. Compaction overrides and the quickgen debug model built clients withoutConfigOptions, so they now pass it and pick up both this setting and the existing Anthropic beta headers.The toggle also makes transport-conditional option handling admin-switchable, so two hardening changes ride along.
ServiceTierFromChatnow maps every tier the codersdk enum advertises (auto,default,flex,scale,priority); it previously returned nil fordefaultandscale, so flipping a model to Responses silently dropped a configuredservice_tierthat the API accepts (fantasy forwards the value unchanged). And a newTestProviderOptionsTransportParitypins, perprovider_options.openaifield, which transport honors it, against a table in ARCHITECTURE.md, so a field honored on one transport and silently ignored on the other fails the test unless recorded as intentional.Review rounds also caught two lifecycle gaps around the new field.
isZeroChatModelCallConfignow inspectsOpenAIConfig, so a stored options blob whose only setting is this toggle survives into GET/list responses instead of reading asmodel_config: null;TestIsZeroChatModelCallConfigCoversEveryFieldsets each config field in isolation and fails if any field is invisible to the zero check. And the model editor's update path sends an explicit emptymodel_configwhen an edit clears the last field, since an omitted property preserves the stored options server-side; covered by theEditClearingLastOptionSendsEmptyConfigstory.Azure keeps following the known-model list, because the Azure provider exposes no equivalent hook. The model editor renders Azure with the OpenAI option schema, so instead of shipping a visible but inert control, the option schema generator gains a
providersstruct tag that it emits asvisible_for_providers. Gating uses the raw provider type rather than the alias table, so the control appears only for openai-typed providers. No hand-written frontend field: the editor renders it from the generated schema.Closes https://linear.app/codercom/issue/CODAGT-874/add-completionsresponses-api-toggle-in-model-editor