mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
Michael Suchacz
2bf0f567d8
Adds `provider_options.openai.reasoning_mode` (`pro`) to chat model configs so admins can turn on OpenAI's Pro reasoning mode for a model, alongside the other OpenAI request options on the model form such as `service_tier`. The field is offered for any model on an OpenAI provider; the field description says Pro is supported from the GPT-5.6 Sol generation and that requests fail when Pro is set on a model that does not support it. There is deliberately no supported-model list anywhere: a newly released model that supports Pro works with no code change, and OpenAI rejects the request when the model does not. Validation rejects only values other than `pro`, non-OpenAI providers, and configs that explicitly force plain sampling (`reasoning_model=false`) or Chat Completions (`use_responses_api=false`), including partial updates that move a configured model onto such a provider or transport. A configured mode implies the Responses API and reasoning-model treatment when `openai_config` leaves those unset (`chatprovider.effectiveOpenAIConfig`), so an alias the pinned SDK has never seen, such as `gpt-daybreak-blue-latest`, still gets `effort` and `summary` and drops `temperature`. The same effective config feeds the client options and the recorded transport, so the two cannot disagree. The pinned fantasy OpenAI provider options do not carry `reasoning.mode`, so the mode is applied as a client-scoped `openai-go` request option when the model client is built rather than per call. The SDK merges pre-serialization `WithJSONSet` overlays by top-level key, so the key is escaped (`reasoning\.mode`) to add `mode` to the existing `reasoning` object instead of replacing `effort` and `summary`. `TestResolveModelCallReasoningModeWire` pins the resulting request body for streaming and non-streaming calls and checks that the fixed computer-use model does not inherit the mode; AI Gateway forwarding of the `reasoning` object is covered in `aibridge`. Because the option is bound to the model client, every call made through a Pro model config carries it, including title generation and quick generation when those fall back to the chat's model; deployments that want cheaper auxiliary calls should point the title generation override at another model. Frontend: the field renders through the existing schema-driven provider options with no hand-written field logic. Two generic changes: `visible_for_providers` is now honored for provider fields the way it already was for general fields (render and payload), so the OpenAI-scoped select does not appear or serialize for Azure, which shares the OpenAI field group; and the add/update model save toasts now carry the server's error detail, so a rejected combination such as Pro with Use Responses API at Off tells the admin which field to change. Docs: `docs/ai-coder/agents/models.md` gains a Reasoning Mode row in the OpenAI provider-specific options table (from the documentation check comment https://github.com/coder/coder/pull/29584#issuecomment-5752457808), and its Azure note now says Azure OpenAI lacks that one option. Not changed: `coderd/x/chatd/ARCHITECTURE.md`. Its OpenAI transport selection table needs a `reasoning_mode` row (client-applied, neither transport) to match `TestProviderOptionsTransportParity`; left for the human author. <details> <summary>Delivery record</summary> - Local validation on every pushed head: targeted Go tests (chatopenai, chatprovider, chatd wire test, aibridge integration test, coderd DB tests for persistence, invalid input, and partial updates), red/green on the wire test (injection removed, and unescaped key), Vitest (`modelConfigFormLogic`, `ModelForm`), Storybook tests for `ModelFormProviderConfig`, biome, tsc, oxlint, golangci-lint on touched packages, pre-commit hook. - Codex round 1 on `e7dbaefeafc`: 3 findings (Storybook assertions, ARCHITECTURE.md TODO, `gpt-daybreak-blue-latest` alias). Disposition: assertions moved to Vitest and alias accepted in `8fccf111111`; ARCHITECTURE.md left untouched by author instruction. Threads replied to and resolved. - Codex round 2 on `8fccf111111`: 1 finding (fixture naming, FE9), renamed in `23449e92317`. Round 3 on `23449e92317`: 1 finding (fixture location, FE9), moved to `testHelpers/chatModels.ts` in `c3bd5aa370f`. Round 4 on `c3bd5aa370f`: "Didn't find any major issues" (https://github.com/coder/coder/pull/29584#issuecomment-5735119236). All threads resolved. - Remote UAT round 1 (chat https://dogfood.cdr.dev/agents/f5ed1d16-be1d-4d39-952b-126a6a829ef1) exercised `e7dbaefeafc`: endorsed PASS. Real `gpt-5.6-sol` turns showed `"mode": "pro"` / `"standard"` / no key in the Debug tab with effort and summary intact; 14 POST and 6 PATCH rejections captured; UI select visibility, persistence, reload, and clearing verified. Low, non-blocking notes: the `gpt-5.6` alias revert in the model identifier field is pre-existing; Pro is dropped silently when a model or transport change makes it unsupported; Pro applies to auxiliary calls (see above). - Remote UAT round 2 on `c3bd5aa370f` (same chat, alias and core re-verify): core PASS, alias FAIL. With `gpt-daybreak-blue-latest` the request carried `{"mode": "pro"}` without the configured effort and summary and kept sending `temperature` (provider 400 then retry on quick generation and title calls), because the SDK does not classify the alias as a reasoning model. Fixed in `d4c338de767`: a configured mode now implies a reasoning model unless `openai_config.reasoning_model` is set, and a mode on a config that forces plain sampling is rejected. Alias UI/API behavior, rejections, and known-model Pro/Default turns all passed on `c3bd5aa370f`. - Codex round 5 on `d4c338de767`: "Didn't find any major issues" (https://github.com/coder/coder/pull/29584#issuecomment-5736190356). Zero unresolved threads. CI green on `d4c338de767`. - Remote UAT round 3 on `d4c338de767` (same chat): endorsed PASS. Alias runs are single-attempt 200s with `{"effort", "summary", "mode": "pro"}` and no `temperature` (quick generation and title calls no longer retry); `reasoning_model=false` plus a mode is rejected by POST and PATCH with the stored config unchanged; the Reasoning Model override at Off hides the select and drops the mode on save; `gpt-5.6-sol` Pro turn unchanged. Remaining low notes, none introduced here: rejection detail for a non-OpenAI provider names the model rather than the provider; Pro is dropped silently when an override makes it unsupported; `PATCH model_config` replaces the whole object (pre-existing on main). - coder-frontend-review on `d4c338de767` (8 role reviewers, 3 passes): 13 failing checkpoints, chiefly a TS mirror of the Go model regex, the `json_name === "reasoning_mode"` gate duplicated in the render filter and payload builder, a defaulted `model` parameter, and comment and type nits. Fixed in `554c90d5fd9`: the pattern and provider scope are generated onto the field schema, one predicate is shared by both call sites, `buildModelConfigFromForm(provider, model, form)` requires the model, and the ModelForm test also asserts the select disappears for an unsupported model. Red/green: stripping `supported_models` or `visible_for_providers` from the generated schema fails 21 and 1 Vitest cases respectively. Declined with rationale: RO4 (move the fixture to `ModelsPage/testFixtures.ts`; FE9 in `FRONTEND_PATTERNS.md` names `testHelpers/chatModels.ts`, and Codex round 3 required it) and a DF5 nit to gate on the committed identifier and warn when a saved mode is dropped (behavior change, separate follow-up). Final pass: all checkpoints PASS or N/A. - Remote UAT round 4 on `554c90d5fd9` (same chat, focused regression after the frontend-review refactor): endorsed PASS. All six claims held (Pro round trip with effort and tier intact; unsupported identifiers hide the select and a save drops the stale mode while keeping the other options; Azure never shows or saves it; the alias shows it only with the Responses override On; Reasoning Model Off hides it and drops it; three real `gpt-5.6-sol` Pro turns were single-attempt 200s with `{"effort","summary","mode":"pro"}` and no `temperature`). Probes aimed at the refactor also passed: `gpt-5X6-sol` is rejected (dot escaping survived the JSON round trip), whitespace-padded identifiers still match, the other 11 OpenAI fields and the Anthropic section still render. Remaining low notes are carried over from round 3 plus one naming note ("Reasoning Mode" versus "Reasoning Model"). The final head `abb58df77d8` differs from `554c90d5fd9` only in `ModelForm.test.tsx`, so it ships the same production code. - Codex round 6 on `554c90d5fd9`: 1 finding (https://github.com/coder/coder/pull/29584#pullrequestreview-5260492465): the two `toBeInTheDocument` assertions added in the frontend-review round violate FE1. Confirmed against `site/AGENTS.md`; removed in `abb58df77d8` (test-only, 7 deletions; the payload assertion keeps the behavior coverage and the `ProviderConfigOpenAI` story captures the hidden state). Thread replied to and resolved. - Codex round 7 on `abb58df77d8`: "Didn't find any major issues" (https://github.com/coder/coder/pull/29584#issuecomment-5749626071). Zero unresolved threads. CI green on `abb58df77d8` (23 success, 8 skipped, `required` pass). - Design change on `e99152e5c57` (owner direction: no model gating; "none or pro" for any model; tooltip states the GPT-5.6 Sol constraint and that requests fail on unsupported models). Removed `codersdk.ChatModelReasoningModeModels`, the generated `supported_models`, `isFieldApplicableToModel`, the `gpt-daybreak-blue-latest` special case, and the `standard` enum value; `ValidateReasoningMode(provider, config)` no longer takes the model; `effectiveOpenAIConfig` makes a mode imply Responses and reasoning-model treatment. PR footprint went from +1130/-22 to +769/-18. Local validation: `go test` for chatopenai, chatprovider, chatd wire test, and the coderd API tests (persistence now covers `gpt-5.6-sol`, `gpt-4o`, `gpt-daybreak-blue-latest`, `gpt-7-nova`; invalid input now rejects `standard`), golangci-lint on touched packages, Vitest (187 tests across `modules/aiModels` and `ModelsPage`), biome, tsc, pre-commit hook. - coder-frontend-review on `e99152e5c57` (8 roles, 1 pass plus a TS and CD confirmation pass): 9 failing checkpoints (SD4, CD1, CD3, CD4, TS3, TS7 x2, RO3, RO5, TN4, DF5). Fixed in `e06bfbe4299`: the ModelForm test edits `mockGPT5` so its provider matches the selected provider state, types the update mock like the sibling story, and drops an unused route; the story spreads `mockGPT5` with a saved mode instead of a dedicated shared fixture (`MockGPT56Pro` removed, `testHelpers/chatModels.ts` back to base); a `ProviderConfigAzure` story captures the panel without `reasoning_mode`; comment nits. Left as a nit with justification: RO3 (the one-line `!hidden && isFieldVisibleForProvider` predicate now appears in both `getVisibleProviderFields` and `getVisibleGeneralFields`; hoisting a helper for two call sites is not worth it). Confirmation pass: TS all PASS; CD flagged one wording nit, fixed before commit. - CI green on `e06bfbe4299` (23 success, 8 skipped, `required` pass). - Codex round 8 on `e06bfbe4299`: 1 P2 finding (https://github.com/coder/coder/pull/29584#discussion_r4057659673): the form still offers Pro with Use Responses API or Reasoning Model at Off, a combination the server rejects, and the failure was not actionable. Disposition: the select stays visible by owner direction (no client-side gating of this field beyond the provider scope; an explicit rejection is preferred over the earlier silent drop), and `e2b43c3aee7` makes the add/update save toasts show the server detail so the admin sees which field to change. Thread replied to and resolved; zero unresolved threads. - Remote UAT round 5 on `e06bfbe4299` (same chat): endorsed PASS with two UI issues to land before release. All seven probes passed: the select offers only Default and Pro for `gpt-5.6-sol` and `gpt-4o` with the tooltip text verbatim; Pro saves on `gpt-4o` and the real turn fails visibly with the provider's "reasoning.mode is not supported with this model" after a single 400 attempt, and works again once Pro is cleared; `gpt-daybreak-blue-latest` with Pro and no overrides is a single 200 to `/v1/responses` with `{effort, summary, mode: "pro"}` and no `temperature`; the `gpt-5.6-sol` Pro turn is unchanged; POST and PATCH reject `standard`, `PRO`, empty, Responses Off, Reasoning Model Off, and Azure, openai-compat, and Anthropic providers with the stored config unchanged, while a model-only PATCH to `gpt-4o` now succeeds; the Azure form shows no Reasoning Mode; the alias with an explicit Responses override still works. Reservations: (1) medium, the UI conflict path only showed "Invalid model config." without the detail (fixed in `e2b43c3aee7`, not yet UAT-exercised); (2) low-medium, a dev-DB row still holding `standard` from round 2 renders an empty red select whose message is the whole field description plus "has an invalid value." and blocks saving unrelated fields (pre-existing generic behavior of `yupTestForField` using the description as the label; no released data can hold `standard`, so no migration); (3) low, no hint next to older identifiers (documented design); (4) low, a chat on such a stored row fails with a generic headline (pre-existing). - Final head `e2b43c3aee7` differs from `e06bfbe4299` only in the two save-toast handlers (`AddModelPage.tsx`, `UpdateModelPage.tsx`, 8 insertions). CI is green on `e2b43c3aee7` (skipped=8, success=23, `required` success). Codex has not run on `e2b43c3aee7`. - Remote UAT round 6 on `e2b43c3aee7` (same chat, https://dogfood.cdr.dev/agents/f5ed1d16-be1d-4d39-952b-126a6a829ef1, settled 2026-09-20T19:02Z): endorsed PASS on the exact final head. Build proven by `/api/v2/buildinfo` (`v2.37.2-devel+e2b43c3aee`, `external_url` at the full SHA) and the UI badge. The round 5 toast issue is resolved: Add and Update saves with Pro plus Use Responses API Off or Reasoning Model Off show "Invalid model config." with the server detail as the description, one 400 each, nothing stored. Compact regression of every round 5 probe held (dropdown and tooltip; Pro on `gpt-4o` fails visibly with the provider's `reasoning.mode is not supported with this model` and works after clearing; alias with and without the Responses override and `gpt-5.6-sol` all send `{effort, summary, mode: "pro"}` with no `temperature` in a single 200; POST and PATCH rejection matrices with stored config unchanged; Azure form without the field). Legacy `standard` row: fails closed on chat, and re-picking Pro or Default saves. Remaining notes, none blocking: the malformed validation copy for an invalid stored enum (pre-existing `yupTestForField` label choice, dev-DB rows only), Pro offered for models that cannot accept it (documented design), and a pre-existing `quickgen` status quirk on `gpt-4o` unrelated to this field. Secret scan clean. - Final head `4f3c6106dfe` adds one docs-only commit on top of `e2b43c3aee7` (`docs/ai-coder/agents/models.md`, 9 insertions, 8 deletions, no code). Remote UAT round 6 exercised `e2b43c3aee7`; the docs commit is not UAT-relevant. Validated with `markdown-table-formatter`, `markdownlint-cli2`, and the pre-commit hook (`fmt/markdown`, `lint/markdown`, `lint/emdash`, `lint/typos`). CI is green on `4f3c6106dfe` (success=24, skipped=7, `required` success) and the documentation check re-run marked its item addressed (https://github.com/coder/coder/pull/29584#issuecomment-5752457808). - Review tally for this PR: Codex 8 completed rounds; coder-frontend-review 34 role passes (24 on `d4c338de767`, 10 on `e99152e5c57`); remote UAT 5 rounds; no human review yet. This is far past the six-review guardrail, so no further Codex or agent review has been requested. Whether to spend another Codex pass on `e2b43c3aee7` is an owner decision; human review is still required before merge. </details> > Xum acted on behalf of @ibetitsmike.