Repository navigation
feat(combo): adapt reasoning effort to target capabilities (#2731) - #3190
Conversation
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. |
181bb60 to
5f8cd24
Compare
|
✅ Deterministic PR hygiene checks passed. |
![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: 181bb60729
ℹ️ 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".
| const omitReasoningEffortWithTools = !!tools | ||
| && modelInList(provider.omitReasoningEffortWithToolsModels, parsed.modelId); |
There was a problem hiding this comment.
When a client calls /v1/chat/completions directly with both tools and reasoning_effort, src/server/chat-native.ts uses buildOpenAIChatPassthroughRequest, which copies both fields without consulting omitReasoningEffortWithToolsModels; this new guard runs only on the translated adapter path. Consequently, the exact configured model can still receive the rejected combination and return an upstream error for native Chat clients. Apply the same capability gate in the passthrough builder and add coverage for that entry point.
Useful? React with 👍 / 👎.
| const advertisedLadders = combo.reasoningEffortMode === "adaptive" | ||
| ? knownLadders.filter(ladder => ladder.length > 0) |
There was a problem hiding this comment.
When an adaptive combo contains an openai-responses target with reasoningEfforts: [], excluding that ladder advertises the siblings' efforts to Codex, so ordinary requests now carry an effort. If that target is selected, concreteComboRequestBody preserves caller-supplied effort and the Responses path has no equivalent of the Chat-only stripEmptyLadderEffort call, allowing an unsupported field to reach the upstream and fail the combo attempt. Strip effort according to the selected concrete target before Responses dispatch, or do not publish it for combinations where dispatch cannot safely adapt.
Useful? React with 👍 / 👎.
| if (listed === undefined) continue; | ||
| // Adaptive mirrors the served catalog: a target advertising no effort control is | ||
| // excluded from the intersection rather than collapsing it for every sibling. | ||
| if (reasoningEffortMode === "adaptive" && listed.length === 0) continue; |
There was a problem hiding this comment.
When every selected target explicitly advertises reasoningEfforts: [], adaptive mode skips every target, leaves common as null, and the function consequently returns the full COMBO_EFFORTS list. The server-side derivation deliberately returns [] for this same case, so the dashboard offers and can save unsupported defaults even though the served catalog has no effort picker. Track whether empty known ladders were skipped and return [] when none of the targets supplies a surviving non-empty ladder.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
리뷰 · 우선순위 60 / 80현재 바로 전에 들어온 #3172는 콤보 기본 단계를 남은 사다리 안에서 아래로 맞춥니다. 선택기 자체가 비면 맞출 곳이 없어서, 빈 사다리가 선택기를 지우는 문제는 그대로입니다. 이 PR은 그 규칙을 바꾸는 스위치를 넣습니다. 콤보에 두 번째 구멍은 openai-chat입니다. 어떤 모델은 도구 없는 요청에는 추론 단계를 받지만, 함수 도구가 있으면 그 필드를 거절합니다. 지금은 원래 기여자 PR #2734는 이 두 구멍을 봤지만 types.ts/config.ts 분리는 지킵니다. 타입은 테스트가 분기를 직접 밟습니다. adaptive는 빈 사다리를 빼고, 비어 있지 않은 사다리는 계속 교집합하고, 전부 빈 사다리면 선택기가 비어 있습니다. 도구가 있을 때만 와이어 단계를 빼고, 목록에 없는 형제는 그대로 둡니다. PUT에서 디스크로 다시 GET까지 adaptive가 남고, strict는 파일에 안 쓰이고, 모르는 값은 400입니다. src/codex/catalog/aggregation.ts 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Adopts the design from #2734 and completes the surfaces it left open. Two defects, both opt-in fixes that default to today's behavior: - A combo intersects its targets' effort ladders, and an explicitly empty ladder (a target with no effort control) collapsed the intersection for the whole group. reasoningEffortMode: "adaptive" excludes empty ladders from the published intersection; "strict" remains the default. - openai-chat could declare per-model effort support but not a per-request rule, so a model that rejects effort only alongside tools had to use noReasoningModels and lose its picker entirely. omitReasoningEffortWithToolsModels drops the wire field for tool-bearing requests only. Beyond #2734: the dashboard round-trip. PUT /api/combos replaces the stored combo wholesale and the GUI serializer is a field allowlist, so a hand-edited reasoningEffortMode was silently destroyed by any dashboard combo edit. The field is now parsed, serialized, dirty-checked and sync-keyed, and the shared sparse helper keeps GET, the PUT response and disk consistent so an unset default is never materialized into config.json. The opt-in is a switch in the combo Capabilities section beside image input, where combo-wide policy already lives - not under Default reasoning, which is a value chosen from the intersection rather than a rule that changes it. Both the create modal and the detail panel share that component, so the create flow shows the corrected picker too. Refs #2731. Supersedes #2734.
Activation-grounded regressions for both halves of #2731, plus the dashboard round-trip the original PR left uncovered. - codex-catalog: adaptive keeps the surviving sibling ladder where strict empties it, still intersects non-empty ladders, and yields nothing when every target is empty. - openai-chat-hardening: effort dropped only with tools present, kept without them, untouched for an unlisted sibling and for an unset provider list, and suppressed on the gateway-object branch that writes its own reasoning field. - combo-management-api: adaptive survives PUT to disk to GET; the strict default is never materialized; turning it back off clears the stored field; an unknown value is rejected. - combo-workspace-data: the GUI intersection honors the mode, parse and toPutBody round-trip it, and draftEquals treats a change as dirty so Save enables. - management-provider-validation: validates, exposes and normalizes the new provider list.
5f8cd24 to
64ff03a
Compare
…#2731) (lidge-jun#3190) * feat(combo): adapt reasoning effort to target capabilities Adopts the design from lidge-jun#2734 and completes the surfaces it left open. Two defects, both opt-in fixes that default to today's behavior: - A combo intersects its targets' effort ladders, and an explicitly empty ladder (a target with no effort control) collapsed the intersection for the whole group. reasoningEffortMode: "adaptive" excludes empty ladders from the published intersection; "strict" remains the default. - openai-chat could declare per-model effort support but not a per-request rule, so a model that rejects effort only alongside tools had to use noReasoningModels and lose its picker entirely. omitReasoningEffortWithToolsModels drops the wire field for tool-bearing requests only. Beyond lidge-jun#2734: the dashboard round-trip. PUT /api/combos replaces the stored combo wholesale and the GUI serializer is a field allowlist, so a hand-edited reasoningEffortMode was silently destroyed by any dashboard combo edit. The field is now parsed, serialized, dirty-checked and sync-keyed, and the shared sparse helper keeps GET, the PUT response and disk consistent so an unset default is never materialized into config.json. The opt-in is a switch in the combo Capabilities section beside image input, where combo-wide policy already lives - not under Default reasoning, which is a value chosen from the intersection rather than a rule that changes it. Both the create modal and the detail panel share that component, so the create flow shows the corrected picker too. Refs lidge-jun#2731. Supersedes lidge-jun#2734. * test(combo): cover adaptive effort mode and the tool-bearing opt-out Activation-grounded regressions for both halves of lidge-jun#2731, plus the dashboard round-trip the original PR left uncovered. - codex-catalog: adaptive keeps the surviving sibling ladder where strict empties it, still intersects non-empty ladders, and yields nothing when every target is empty. - openai-chat-hardening: effort dropped only with tools present, kept without them, untouched for an unlisted sibling and for an unset provider list, and suppressed on the gateway-object branch that writes its own reasoning field. - combo-management-api: adaptive survives PUT to disk to GET; the strict default is never materialized; turning it back off clears the stored field; an unknown value is rejected. - combo-workspace-data: the GUI intersection honors the mode, parse and toPutBody round-trip it, and draftEquals treats a change as dirty so Save enables. - management-provider-validation: validates, exposes and normalizes the new provider list. * docs(devlog): record the 3190 cherry-pick onto privacy-clean dev --------- Co-authored-by: jun <jun@lidge.dev>
…#2731) (lidge-jun#3190) * feat(combo): adapt reasoning effort to target capabilities Adopts the design from lidge-jun#2734 and completes the surfaces it left open. Two defects, both opt-in fixes that default to today's behavior: - A combo intersects its targets' effort ladders, and an explicitly empty ladder (a target with no effort control) collapsed the intersection for the whole group. reasoningEffortMode: "adaptive" excludes empty ladders from the published intersection; "strict" remains the default. - openai-chat could declare per-model effort support but not a per-request rule, so a model that rejects effort only alongside tools had to use noReasoningModels and lose its picker entirely. omitReasoningEffortWithToolsModels drops the wire field for tool-bearing requests only. Beyond lidge-jun#2734: the dashboard round-trip. PUT /api/combos replaces the stored combo wholesale and the GUI serializer is a field allowlist, so a hand-edited reasoningEffortMode was silently destroyed by any dashboard combo edit. The field is now parsed, serialized, dirty-checked and sync-keyed, and the shared sparse helper keeps GET, the PUT response and disk consistent so an unset default is never materialized into config.json. The opt-in is a switch in the combo Capabilities section beside image input, where combo-wide policy already lives - not under Default reasoning, which is a value chosen from the intersection rather than a rule that changes it. Both the create modal and the detail panel share that component, so the create flow shows the corrected picker too. Refs lidge-jun#2731. Supersedes lidge-jun#2734. * test(combo): cover adaptive effort mode and the tool-bearing opt-out Activation-grounded regressions for both halves of lidge-jun#2731, plus the dashboard round-trip the original PR left uncovered. - codex-catalog: adaptive keeps the surviving sibling ladder where strict empties it, still intersects non-empty ladders, and yields nothing when every target is empty. - openai-chat-hardening: effort dropped only with tools present, kept without them, untouched for an unlisted sibling and for an unset provider list, and suppressed on the gateway-object branch that writes its own reasoning field. - combo-management-api: adaptive survives PUT to disk to GET; the strict default is never materialized; turning it back off clears the stored field; an unknown value is rejected. - combo-workspace-data: the GUI intersection honors the mode, parse and toPutBody round-trip it, and draftEquals treats a change as dirty so Save enables. - management-provider-validation: validates, exposes and normalizes the new provider list. * docs(devlog): record the 3190 cherry-pick onto privacy-clean dev --------- Co-authored-by: jun <jun@lidge.dev>
1 participant
Summary
Adopts the design from #2734 (thanks @TooSpace) and completes the surfaces it left open. Closes #2731.
Two independent defects, each fixed behind an opt-in that defaults to today's behavior:
1. An empty effort ladder poisons the whole combo picker. A combo publishes the intersection of its targets' advertised effort ladders.
deriveComboCatalogModeltreated an explicitly empty ladder — a target that advertises no effort control — as a participant in that intersection, so one such target emptied the picker for every sibling that does support tuning, and took the combo'sdefaultEffortwith it.reasoningEffortMode: "adaptive"excludes empty ladders from the published intersection."strict"remains the default and preserves the existing contract.2. openai-chat could not express a per-request rule. A model that accepts reasoning effort on an ordinary turn but rejects it once function tools are present had only
noReasoningModels, which strips reasoning from every request and costs the model its picker entirely.omitReasoningEffortWithToolsModelsomits the wire field for tool-bearing requests only. The guard is applied on both the plainreasoning_effortpath and thegateway-objectbranch, which writes its own reasoning field.Beyond #2734: the dashboard round-trip
PUT /api/combosreplaces the stored combo wholesale, and the GUI serializertoPutBodyis a field allowlist. A field the dashboard does not round-trip is therefore silently destroyed by any combo edit — a user who setreasoningEffortModeby hand would rename the combo, reorder a target, and watch their picker go empty again with no error. #2734 touches nogui/file, so it shipped that hole.This PR closes it: the field is parsed, serialized, dirty-checked and sync-keyed, and
sparseComboConfignow covers it so GET, the PUT response and disk agree — an unset default is never materialized intoconfig.json, and a GET→PUT round-trip cannot write it back.UI
The opt-in is a switch in a combo's Capabilities section beside image input, where combo-wide policy already lives — deliberately not under "Default reasoning", which is a value chosen from the intersection rather than a rule that changes what the intersection is. The detail panel and the create modal share that component, so the create flow shows the corrected picker too, and the GUI intersection honors the mode instead of contradicting the served catalog. Labels and hints are translated for all 9 locales.
Verification
Regressions are activation-grounded rather than status-only — each new conditional branch has a case that triggers it and observes its effect:
adaptivesurvives PUT→disk→GET; the strict default is never persisted; turning it off clears the field; an unknown value is rejected 400toPutBodyround-trip it;draftEqualsmarks a change dirty so Save enablesBack-compat: with nothing configured, catalog rows and wire bodies are unchanged.
normalizeComboConfigmaps anything that is not the literal"adaptive"to"strict", andmodelInListis false for an absent list.Checklist
bun x tsc --noEmitpasses (root and gui)bun run privacy:scanpassesdocs-site/(combos guide, routing reference, providers reference)devFollow-ups (deliberately out of scope)
concreteComboRequestBodydoes not strip a caller-supplied effort for a target that cannot use it; Responses does not runstripEmptyLadderEffort(Chat-only). Pre-existing, orthogonal to the picker/wire defects here.buildOpenAIChatPassthroughRequestignores this key exactly as it already ignoresnoReasoningModels.