feat(providers/openai): allow reasoning-model overrides and recognize gpt-6+ - #57
Merged
Merged
Conversation
Signed-off-by: drew <me@andrinoff.com> (cherry picked from commit cfb0530)
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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". |
Author
|
Note on CI: the |
ibetitsmike
added a commit
to coder/coder
that referenced
this pull request
Sep 15, 2026
coder/fantasy#57 squash-merged into coder_2_33 as a511353f83eb. The openai provider is byte-identical to the previously pinned PR head f5b3f253c112; the new pin also carries coder/fantasy#58 (Gemini media tool results) and indirect x/image and x/text bumps that fix the govulncheck findings.
ibetitsmike
added a commit
to coder/coder
that referenced
this pull request
Sep 16, 2026
## Why Bedrock Mantle's OpenAI Responses endpoint rejects `reasoning.summary` values other than `auto` with HTTP 400 (`Unsupported parameter: 'reasoning.summary' is not supported with the 'openai.gpt-5.6-sol' model`). chatd forwards `provider_options.openai.reasoning_summary` from the model config unchanged, so every chat turn and quickgen title call through a Bedrock model config carrying `concise`/`detailed` fails. On dogfood this produced 43 user-visible "chat generation failed" errors and 118 title-generation failures in one day; five Bedrock configs still carry `detailed`. ## What `resolveModelCall` now coerces `provider_options.openai.reasoning_summary` to `auto` for Bedrock providers whose model uses the OpenAI wire format (non-`anthropic.` model IDs), before per-call provider options are built. Mantle accepts `auto` (it returns no summaries today, but encrypted reasoning continuity keeps working). Coercing instead of dropping keeps one config portable between Bedrock and direct OpenAI, and summaries start flowing if AWS adds support. Anthropic models on Bedrock and every non-Bedrock provider are untouched. Only the derived provider options see the coerced value; the resolved call config keeps the configured one, so computer-use rerouting to direct OpenAI is unaffected. A TODO item in `coderd/x/chatd/ARCHITECTURE.md` marks the section for the human author, per the chatd guardrail. ## Validation - `TestAIGatewayModelBedrockReasoningSummary` drives the real `resolveModelCall` path (newModel + ProviderOptionsForCall) with a request-capturing transport: Bedrock + `openai.gpt-5.6-luna` with `detailed` sends `"summary":"auto"`; Bedrock + Anthropic and direct OpenAI are unchanged. `TestBedrockReasoningSummary` covers the helper table. Red-green: removing the resolver call or the helper body fails the body assertion (`expected "auto", actual "detailed"`). - `go test ./coderd/x/chatd/ -run 'TestAIGateway|Bedrock'`, `go test ./coderd/x/chatd/chatprovider/...`, `golangci-lint run ./coderd/x/chatd/...` (0 issues), `make pre-commit`. - Remote dogfood UAT (Coder Agents) against a mock Bedrock upstream and the real OpenAI gateway: Bedrock `openai.gpt-5.6-luna` configs with `detailed`/`concise`/odd-cased values send `"summary":"auto"` and complete chats and titles; direct OpenAI keeps `detailed` with visible summaries; Bedrock Anthropic requests gain no OpenAI fields. UAT ran on the first head (a4878f8); the follow-up commit only stops the coerced config from leaking into provider substitution and swaps the doc paragraph for a TODO. - The local `pre-push` hook failed only on unrelated pre-existing failures in this checkout (`provisioner/terraform TestGetModulesArchive` hash mismatch from the host umask, two untouched site tests), so the push bypassed it; CI is the gate. Related follow-ups: coder/fantasy#57 (gpt-6+ reasoning classification and an override hook) and a coder PR exposing an `openai_config.reasoning_model` flag in the model config UI. > Xum acted on behalf of @ibetitsmike for this PR.
ibetitsmike
added a commit
to coder/coder
that referenced
this pull request
Sep 16, 2026
…9230) ## Why The coder/fantasy fork classifies OpenAI reasoning models by name. The pinned version did not know `gpt-6-astra`, so the Responses client omitted the `reasoning` parameter (the reasoning effort picker did nothing) and forwarded `temperature` (quickgen title calls got HTTP 400 `Unsupported parameter: 'temperature'` and retried). Every new model generation would need a fork release to fix this. Admins need a switch in the model config instead. ## What - Bump `charm.land/fantasy` to coder/fantasy `a511353f83eb`, the `coder_2_33` commit that squash-merged coder/fantasy#57 (the pin also carries coder/fantasy#58, Gemini media tool results): gpt-6 and later generations are recognized as reasoning models, and the provider gains `WithReasoningModelFunc` to override the classification. - New tri-state `openai_config.reasoning_model` on `ChatModelCallConfig`: unset keeps fantasy's name heuristics, `true` forces reasoning-model handling (effort and summary sent, temperature/top_p dropped), `false` forces plain sampling. It is applied once at client construction next to `use_responses_api`. The generated model editor shows it for `openai` and `bedrock` providers, since Bedrock Mantle also serves OpenAI-format models; it is ignored for Anthropic models on Bedrock. - Remove the `IsGPT6Astra` transport fallback in `chatopenai.UsesResponsesAPI`; the bumped SDK recognizes gpt-6 itself (`IsGPT6Astra` stays for the effort clamp). - Regenerated API docs, swagger, TypeScript types, and the model options schema; ARCHITECTURE.md documents the flag. ## Validation - New tests: `TestChatModelCallConfig_ReasoningModelRoundTrip`, `TestModelFromConfig_OpenAIReasoningModelOverride` (request body has `reasoning` and no `temperature` when forced on an unknown model; forced off on gpt-5 drops `reasoning` and keeps `temperature`; unset unchanged), `TestAIGatewayModelBedrockReasoningModelOverride` (Bedrock provider path), `TestIsZeroChatModelCallConfigReasoningModel`, and form-logic cases serializing the field for openai and bedrock. Red-green: removing the constructor wiring fails the request-body assertions. - `go test ./coderd/x/chatd/... -count=1` (full), `go test ./codersdk/... -run ChatModel`, `go test ./coderd/ -run 'ChatModel|Chats'`, `golangci-lint run ./coderd/x/chatd/... ./codersdk/... ./coderd/` (0 issues), `make gen` (stable on rerun), Vitest for `src/modules/aiModels` and `src/api/chatModelOptions` (174 tests), Biome, `tsc --noEmit`, `make pre-commit`. - Remote dogfood UAT (Coder Agents) exercised the editor and chat on the dev AI gateway: the switch shows for OpenAI and Bedrock providers only, persists true/false/unset, and `gpt-daybreak-blue-latest` (an alias the SDK does not recognize) sends `reasoning` without `temperature` when forced on and gets OpenAI's temperature 400 when off or unset. UAT also found the new switch colliding with `Use Responses API` at 375px; tri-state switches now take the full row below `sm`. One open observation for follow-up: in Coder chats on `gpt-6-astra` the upstream stream carried no reasoning-summary events even though `reasoning.summary=detailed` was sent, while the same model returns summaries to a standalone fantasy client; not attributed to this change. - Merged `origin/main` (`eceae296e8d`; the only conflict was the ARCHITECTURE.md TODO block, resolved by keeping main's transport-selection prose and adding a TODO for the GPT-6 Astra sentence this PR makes stale) and moved the pin (`553bfb86bf5`). On that head: `go build ./...`, `go vet ./coderd/x/chatd/... ./codersdk/`, `go test ./coderd/x/chatd/ -run 'ReasoningModel|TestAIGateway|Bedrock|ChatModelCallConfig|Quickgen|Transport'`, `go test ./coderd/x/chatd/chatopenai/... ./coderd/x/chatd/chatprovider/... ./coderd/x/chatd/chatdebug/...`, `go test ./codersdk/ -run 'ChatModel|Reasoning'`, and the pre-commit hook on both commits. The `providers/openai` tree at `a511353f83eb` is byte-identical to the previously pinned PR head `f5b3f253c112`. - The local `pre-push` hook fails in this checkout only on unrelated tests (umask-dependent `TestGetModulesArchive`, two untouched site tests), so the push bypassed it; CI is the gate. For the pin-bump push the hook ran once in full (31,276 tests passed; the only 2 failures were `TestGetModulesArchive`, proven to be this checkout's 0600 file modes: passes after normalizing modes and on an untouched `origin/main` worktree) and was then opted out for this worktree per Mike's instruction. coder/fantasy#57 is merged (`a511353f83eb` on `coder_2_33`) and the pin points at it. Related: #29229 (Bedrock `reasoning_summary` coercion). > Xum acted on behalf of @ibetitsmike for this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Bedrock Mantle and direct OpenAI both expose
gpt-6-astra, but the fork's Responses client still classifies onlygpt-5*(and o-series/codex) as reasoning models. For any newer generation it omits thereasoningparameter entirely (effort and summary are silently dropped) and forwardstemperature, which OpenAI rejects with HTTP 400. Upstream charmbracelet#354 (cherry-picked here as-is) only widened the publicIsResponsesModel/IsResponsesReasoningModelhelpers;getResponsesModelConfig, which gates the request parameters, was left ongpt-5.Every new model generation currently needs a fork release before chatd can use it correctly. Callers need a way to decide the classification themselves.
What
fix: gpt series 6+ (#354)) unchanged.getResponsesModelConfigtreats gpt-5 and every later generation (gpt-6, gpt-10, ...) as reasoning models;-chatvariants stay non-reasoning; gpt-4 is unchanged.openai.WithReasoningModelFunc(fn func(modelID string) bool), mirroringWithResponsesAPIFunc: when set it replaces the built-in name heuristics for both the Responses client (reasoning params, temperature/top_p filtering, developer vs system role) and the Chat Completions client (WithLanguageModelReasoningModelFunc, including the logprobs/logit_bias filtering inDefaultPrepareCallFunc).Validation
go build ./...,go test ./providers/openai/... -count=1,golangci-lint run ./providers/openai/..., gofumpt clean on touched files.TestPrepareParams_ReasoningModelClassification,TestPrepareParams_ChatReasoningModelOverride(plus upstream'sTestIsResponsesModel/TestIsResponsesReasoningModel). Mutation checks confirmed each assertion fails when the corresponding change is removed.gpt-6-astranow sendsreasoning: {"effort":"low"}withouttemperatureand returns 200; forcing non-reasoning ongpt-5.6-lunavia the new option sendstemperatureand gets OpenAI's 400, showing the override takes effect.Follow-up: coder/coder will bump its pin and expose the override as an
openai_config.reasoning_modelmodel-config flag.