fix(gui,combos): restore default effort picker and catalog combos - #1092
fix(gui,combos): restore default effort picker and catalog combos#1092eachann1024 wants to merge 4 commits into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
github-actions
Bot
changed the title
📝 WalkthroughWalkthroughCombo catalog resolution now synthesizes incomplete targets from provider metadata. Image-input policies flow through configuration, catalog derivation, persistence, request validation, and workspace controls. Effort intersections tolerate missing metadata. The workspace previews and copies public model identifiers. ChangesCombo capabilities and catalog resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ComboWorkspace
participant ComboCatalog
participant ProviderConfiguration
participant ComboRouter
participant UpstreamProvider
ComboWorkspace->>ComboCatalog: request combo target metadata
ComboCatalog->>ProviderConfiguration: resolve incomplete target
ProviderConfiguration-->>ComboCatalog: modalities, efforts, and context limits
ComboCatalog-->>ComboWorkspace: capability data and public model preview
ComboWorkspace->>ComboRouter: submit combo request
ComboRouter->>ComboRouter: detect image input
ComboRouter->>UpstreamProvider: forward request when image input is allowed
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/components/combo-workspace-detail-panel.tsx`:
- Line 14: Update the component’s copyModel implementation to use the shared
useCopyFeedback hook, or its copyTextToClipboard helper, instead of calling
navigator.clipboard.writeText inside a silently swallowed try/catch. Ensure the
header “Copy id” chip receives the same “copied” and “unavailable” feedback
behavior as PublicModelPreview, and adjust the related hook usage/imports as
needed.
In `@src/codex/catalog/provider-fetch.ts`:
- Around line 664-676: Clamp the synthesized fallback used to assign
contextWindow in the provider catalog path with providerContextCaps before
assignment, rather than advertising the uncapped 128,000 window. When the cap
lowers that fallback, propagate contextCapped as true instead of retaining the
value from applyProviderConfigHints. Add a direct regression test covering a
provider cap below 128,000 with no configured or discovered context window.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f901c86c-bdf1-47db-8f1b-f37fb967ee3c
📒 Files selected for processing (17)
gui/src/combo-workspace-data.tsgui/src/components/combo-workspace-add-modal.tsxgui/src/components/combo-workspace-controls.tsxgui/src/components/combo-workspace-detail-panel.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/styles-combos-workspace.csssrc/codex/catalog.tssrc/codex/catalog/provider-fetch.tssrc/combos/request.tstests/codex-catalog.test.tstests/combo-workspace-data.test.tstests/combos.test.ts
![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: bd7729a914
ℹ️ 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 contextWindow = hintedContext | ||
| ?? (existing || prov ? COMBO_MEMBER_CONTEXT_FALLBACK : undefined); |
There was a problem hiding this comment.
When a combo target has no discovered/configured context, hintedContext is undefined and this fallback assigns 128_000 after applyProviderConfigHints has already had its only chance to apply contextCap. In a config with providerContextCaps[target.provider] below 128k, the synthesized member returns a contextWindow above the user cap (and can carry contextCapped: false), so the derived combo catalog advertises more context than the provider cap allows. Clamp the fallback through the supplied contextCap before returning it.
Useful? React with 👍 / 👎.
| // Unknown ladder (`undefined`): inject optimistically — catalog metadata is | ||
| // incomplete more often than the provider truly lacks the effort. Explicit | ||
| // arrays still gate: missing default → omit + debug warn. | ||
| if (targetReasoningEfforts !== undefined && !targetReasoningEfforts.includes(defaultEffort)) { |
There was a problem hiding this comment.
This changes defaultEffort behavior from omitting when the target ladder is unknown to injecting optimistically for undefined, but docs-site/src/content/docs/guides/combos.md still says unknown target capability omits the default and the routing reference still says the target must advertise the rung. Users configuring combos from the docs will expect the opposite runtime behavior, so update the docs-site pages and translated locales to describe the new unknown-ladder semantics.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
| const base: CatalogModel = existing ?? { | ||
| id: target.model, | ||
| provider: target.provider, | ||
| }; |
There was a problem hiding this comment.
For providers whose /models fetch succeeds authoritatively, a combo target that is absent from the live rows has no memberByKey entry because the configured id was intentionally dropped above. This fallback now invents a member for any target on a known provider anyway, so a typo or plan-unavailable model gets listed as a healthy combo instead of being marked catalog-omitted; requests then fail only at dispatch time. Limit missing-member synthesis to targets with explicit config/registry capability hints or non-authoritative discovery fallback.
AGENTS.md reference: src/AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
| * When hints still omit contextWindow, apply COMBO_MEMBER_CONTEXT_FALLBACK so a | ||
| * live row without ctx (common for LiteLLM / custom xai ids) does not drop the | ||
| * whole combo from the public catalog. |
There was a problem hiding this comment.
This new fallback means a combo member with no live/configured context can still be synthesized with a 128k window, but the routing reference still says a positive contextWindow from live metadata, registry, or provider config is required and that bare relay ids with no context metadata are removed from the catalog. That now contradicts the model list and dashboard attention behavior for these recovered combos, so update the docs-site routing pages and translations to document the fallback.
AGENTS.md reference: AGENTS.md:L231-L232
Useful? React with 👍 / 👎.
bd7729a to
422f8eb
Compare
…nd context cap Use shared useCopyFeedback for the combo detail header chip (copied + unavailable), clamp COMBO_MEMBER_CONTEXT_FALLBACK with provider contextCap, and document optimistic defaultEffort injection for unknown ladders.
github-actions
Bot
added
the
intake: hygiene-blocked
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd7729a914
ℹ️ 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 contextWindow = hintedContext | ||
| ?? (existing || prov ? COMBO_MEMBER_CONTEXT_FALLBACK : undefined); |
There was a problem hiding this comment.
When an incomplete live/configured member has no contextWindow but does have maxInputTokens (for example a /models row with max_input_tokens: 8192), this branch still falls through to the 128k fallback and the synthesized combo advertises a 128k contextWindow. That makes /api/models and routing capability checks treat an 8k target as a 128k combo member; use the known positive maxInputTokens as the fallback (or min it with the fallback/cap) before defaulting to 128k.
Useful? React with 👍 / 👎.
| const reasoningEfforts = hinted.reasoningEfforts | ||
| ?? (prov ? configuredReasoningEfforts(prov, target.model) : undefined) | ||
| ?? base.reasoningEfforts; |
There was a problem hiding this comment.
When a member is recovered by this new path but has no reasoning-effort hint, reasoningEfforts remains undefined; deriveComboCatalogModel intersects that as [], so one unknown recovered target zeros the combo ladder and drops defaultReasoningEffort from the catalog. In a combo with one target advertising low/medium/high and another recovered with unknown metadata, /v1/models no longer exposes effort choices/default even though the GUI and request path treat the unknown target as non-constraining; carry that wildcard semantics into derivation or synthesize a usable ladder from known evidence.
Useful? React with 👍 / 👎.
| const base: CatalogModel = existing ?? { | ||
| id: target.model, | ||
| provider: target.provider, |
There was a problem hiding this comment.
For a configured target whose provider row was intentionally removed from all by shouldExposeRoutedModel (for example image/video generation IDs such as Sora or Grok media models), memberByKey has no entry and this fallback invents a text member anyway. A combo containing that media-only target is then published in the Codex model picker despite the underlying model being filtered as unsupported; distinguish filtered-out rows from genuinely missing metadata before synthesizing.
Useful? React with 👍 / 👎.
eachann1024
changed the title
Follow-upGate
Review feedback addressed in
|
github-actions
Bot
removed
the
intake: hygiene-blocked
|
Thanks for this — the UI half is a real bug and your diagnosis of it is right. The runtime half is where I have to ask for a change. What the diff reverses. In if (targetReasoningEfforts !== undefined && !targetReasoningEfforts.includes(defaultEffort)) {so an capability: targetReasoningEfforts === undefined ? "unknown" : "unsupported",That ternary is the tell. Someone specifically wanted "we have no metadata" to be separable from "the provider said no" — the two states were already understood as different, and the current behavior is a decision rather than an oversight. The diff collapses them to the literal The concrete failure mode. A combo containing a target with no discovery metadata can now save I want to be fair to the reasoning in your comment: you are right that incomplete catalog metadata is more common than a genuine lack of support. That is an argument for improving discovery or for letting a user assert the capability — not for the proxy assuming it. When the assumption is wrong the user gets a 400 they cannot explain, and the failure surfaces far from this line. What I am asking for. Keep
If you want the optimistic behavior available, the shape that fits this codebase is an explicit per-model capability assertion — the same thing Two smaller notes:
Happy to look again quickly once the runtime guard is restored — the UI fix is worth landing and I would rather it not wait. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/combos/request.ts (1)
43-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail closed for unknown reasoning capability metadata. The current implementation, test, and guide treat an absent effort ladder as confirmation that an upstream accepts
reasoning.effort. That can turn a usable combo target into a rejected upstream request.
src/combos/request.ts#L43-L58: omitdefaultEffortwhentargetReasoningEffortsisundefined; retain separate"unknown"and"unsupported"debug states.tests/combos.test.ts#L237-L248: expect no injected effort for an unknown ladder; test optimistic behavior only behind an explicit capability assertion.docs-site/src/content/docs/guides/combos.md#L156-L167: document unknown ladders as fail-closed and preserve the distinction from known unsupported ladders.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/combos/request.ts` around lines 43 - 58, Fail closed when reasoning capability metadata is unknown: update the default-effort injection logic in src/combos/request.ts lines 43-58 to omit defaultEffort for undefined targetReasoningEfforts while retaining distinct “unknown” and “unsupported” debug states. Update tests/combos.test.ts lines 237-248 to expect no injection for an unknown ladder and cover optimistic injection only with an explicit capability assertion. Update docs-site/src/content/docs/guides/combos.md lines 156-167 to document unknown ladders as fail-closed and distinguish them from known unsupported ladders.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/combo-workspace-data.ts`:
- Around line 31-43: Update the combo effort computation around the relevant
workspace-data function to distinguish unknown targets from known targets with
an explicit empty effort ladder: return separate unknown and unsupported status
with selectable efforts, keep supported efforts available for mixed failover
combinations, and ensure an all-unsupported combination does not fall back to
the full picker. Update tests/combo-workspace-data.test.ts lines 186-204 to
assert the distinct empty-ladder behavior and all-unsupported default state.
In `@gui/src/components/combo-workspace-controls.tsx`:
- Around line 266-298: Update the copyLabel logic in PublicModelPreview to
handle all three outcomes: use cws.copiedPublicModel for "copied",
cws.copyUnavailable for "unavailable", and retain cws.copyPublicModel as the
default.
- Around line 266-298: Define and export a shared NO_COMBO_ID_PLACEHOLDER
constant in combo-workspace-controls.tsx, use it in PublicModelPreview’s canCopy
comparison, and import it in combo-workspace-add-modal.tsx (lines 142-144) and
combo-workspace-detail-panel.tsx (lines 215-217) to replace their duplicated
placeholder literals.
In `@src/codex/catalog/provider-fetch.ts`:
- Around line 660-680: Require discovered or explicitly configured model
capability evidence before synthesizing a catalog member in the provider-fetch
flow around uncappedContext; remove the known-provider/existing-row fallback for
unknown targets while preserving explicit metadata and context-cap handling. In
tests/codex-catalog.test.ts:771-806, replace ghost-model fallback expectations
with unresolved-target assertions and retain cap coverage using explicit model
metadata. In
docs-site/src/content/docs/reference/configuration/routing.md:185-193, document
that unknown target capabilities do not make a combo catalog-eligible and that
catalog visibility does not guarantee upstream support.
---
Outside diff comments:
In `@src/combos/request.ts`:
- Around line 43-58: Fail closed when reasoning capability metadata is unknown:
update the default-effort injection logic in src/combos/request.ts lines 43-58
to omit defaultEffort for undefined targetReasoningEfforts while retaining
distinct “unknown” and “unsupported” debug states. Update tests/combos.test.ts
lines 237-248 to expect no injection for an unknown ladder and cover optimistic
injection only with an explicit capability assertion. Update
docs-site/src/content/docs/guides/combos.md lines 156-167 to document unknown
ladders as fail-closed and distinguish them from known unsupported ladders.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 07af4235-c1e7-4f96-8d2e-ed4a7186f3fe
📒 Files selected for processing (19)
docs-site/src/content/docs/guides/combos.mddocs-site/src/content/docs/reference/configuration/routing.mdgui/src/combo-workspace-data.tsgui/src/components/combo-workspace-add-modal.tsxgui/src/components/combo-workspace-controls.tsxgui/src/components/combo-workspace-detail-panel.tsxgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/styles-combos-workspace.csssrc/codex/catalog.tssrc/codex/catalog/provider-fetch.tssrc/combos/request.tstests/codex-catalog.test.tstests/combo-workspace-data.test.tstests/combos.test.ts
|
|
||
| /** Effective public model id clients will request — mono value + copy. */ | ||
| export function PublicModelPreview({ model }: { model: string }) { | ||
| const t = useT(); | ||
| const { outcomeFor, copy } = useCopyFeedback<string>(); | ||
| const canCopy = model.trim().length > 0 && model !== "…"; | ||
| const outcome = outcomeFor(model); | ||
| const copyLabel = outcome === "copied" ? t("cws.copiedPublicModel") : t("cws.copyPublicModel"); | ||
| // Split around a sentinel so the model token stays mono in any locale word order. | ||
| const sentinel = "\u0001"; | ||
| const [before, after = ""] = t("cws.field.publicModelPreview", { model: sentinel }).split(sentinel); | ||
|
|
||
| return ( | ||
| <div className="cwi-public-model-preview"> | ||
| <p className="muted cwi-public-model-preview-text"> | ||
| {before} | ||
| <code className="mono cwi-public-model-preview-value">{model}</code> | ||
| {after} | ||
| </p> | ||
| <button | ||
| type="button" | ||
| className="btn btn-ghost btn-sm cwi-public-model-preview-copy" | ||
| disabled={!canCopy} | ||
| onClick={() => { | ||
| if (canCopy) copy(model, model); | ||
| }} | ||
| title={copyLabel} | ||
| > | ||
| <span aria-live="polite">{copyLabel}</span> | ||
| </button> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the missing "unavailable" clipboard state to PublicModelPreview.
Line 273 only checks outcome === "copied". It falls back to the default t("cws.copyPublicModel") label for every other outcome, including "unavailable". useCopyFeedback (see gui/src/components/use-copy-feedback.ts:17-58) explicitly returns "unavailable" when copyTextToClipboard fails (denied permission, insecure context, older browser). In that case, the button silently reverts to the plain "Copy" label instead of telling the user the write failed.
Compare this to DetailPanel's own header copy chip, which you fixed correctly in the same PR (gui/src/components/combo-workspace-detail-panel.tsx:129-133):
const copyLabel = copyOutcome === "copied"
? t("cws.copied")
: copyOutcome === "unavailable"
? t("cws.copyUnavailable")
: t("cws.copyModel");
PublicModelPreview should follow the same three-way branch using the existing cws.copyUnavailable string (already defined in every locale file in this PR) so users get consistent failure feedback everywhere a copy button appears.
🐛 Proposed fix
- const copyLabel = outcome === "copied" ? t("cws.copiedPublicModel") : t("cws.copyPublicModel");
+ const copyLabel = outcome === "copied"
+ ? t("cws.copiedPublicModel")
+ : outcome === "unavailable"
+ ? t("cws.copyUnavailable")
+ : t("cws.copyPublicModel");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Effective public model id clients will request — mono value + copy. */ | |
| export function PublicModelPreview({ model }: { model: string }) { | |
| const t = useT(); | |
| const { outcomeFor, copy } = useCopyFeedback<string>(); | |
| const canCopy = model.trim().length > 0 && model !== "…"; | |
| const outcome = outcomeFor(model); | |
| const copyLabel = outcome === "copied" ? t("cws.copiedPublicModel") : t("cws.copyPublicModel"); | |
| // Split around a sentinel so the model token stays mono in any locale word order. | |
| const sentinel = "\u0001"; | |
| const [before, after = ""] = t("cws.field.publicModelPreview", { model: sentinel }).split(sentinel); | |
| return ( | |
| <div className="cwi-public-model-preview"> | |
| <p className="muted cwi-public-model-preview-text"> | |
| {before} | |
| <code className="mono cwi-public-model-preview-value">{model}</code> | |
| {after} | |
| </p> | |
| <button | |
| type="button" | |
| className="btn btn-ghost btn-sm cwi-public-model-preview-copy" | |
| disabled={!canCopy} | |
| onClick={() => { | |
| if (canCopy) copy(model, model); | |
| }} | |
| title={copyLabel} | |
| > | |
| <span aria-live="polite">{copyLabel}</span> | |
| </button> | |
| </div> | |
| ); | |
| } | |
| /** Effective public model id clients will request — mono value + copy. */ | |
| export function PublicModelPreview({ model }: { model: string }) { | |
| const t = useT(); | |
| const { outcomeFor, copy } = useCopyFeedback<string>(); | |
| const canCopy = model.trim().length > 0 && model !== "…"; | |
| const outcome = outcomeFor(model); | |
| const copyLabel = outcome === "copied" | |
| ? t("cws.copiedPublicModel") | |
| : outcome === "unavailable" | |
| ? t("cws.copyUnavailable") | |
| : t("cws.copyPublicModel"); | |
| // Split around a sentinel so the model token stays mono in any locale word order. | |
| const sentinel = "\u0001"; | |
| const [before, after = ""] = t("cws.field.publicModelPreview", { model: sentinel }).split(sentinel); | |
| return ( | |
| <div className="cwi-public-model-preview"> | |
| <p className="muted cwi-public-model-preview-text"> | |
| {before} | |
| <code className="mono cwi-public-model-preview-value">{model}</code> | |
| {after} | |
| </p> | |
| <button | |
| type="button" | |
| className="btn btn-ghost btn-sm cwi-public-model-preview-copy" | |
| disabled={!canCopy} | |
| onClick={() => { | |
| if (canCopy) copy(model, model); | |
| }} | |
| title={copyLabel} | |
| > | |
| <span aria-live="polite">{copyLabel}</span> | |
| </button> | |
| </div> | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/components/combo-workspace-controls.tsx` around lines 266 - 298,
Update the copyLabel logic in PublicModelPreview to handle all three outcomes:
use cws.copiedPublicModel for "copied", cws.copyUnavailable for "unavailable",
and retain cws.copyPublicModel as the default.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the "no id yet" placeholder into one shared constant instead of three duplicated "…" literals.
PublicModelPreview compares model !== "…" to decide whether copy is enabled, and two separate call sites each hardcode the exact same literal to signal "no id yet." This string is now a functional sentinel, not just display text — a future edit to the placeholder in one file without updating the comparison would silently produce the wrong canCopy result.
gui/src/components/combo-workspace-controls.tsx#L266-L298: export a named constant (for exampleNO_COMBO_ID_PLACEHOLDER = "…") and use it in thecanCopycomparison instead of the bare literal.gui/src/components/combo-workspace-add-modal.tsx#L142-L144: import the constant and pass it instead of the literal"…".gui/src/components/combo-workspace-detail-panel.tsx#L215-L217: import the constant and pass it instead of the literal"…".
♻️ Proposed fix
--- a/gui/src/components/combo-workspace-controls.tsx
+++ b/gui/src/components/combo-workspace-controls.tsx
@@
+export const NO_COMBO_ID_PLACEHOLDER = "…";
+
/** Effective public model id clients will request — mono value + copy. */
export function PublicModelPreview({ model }: { model: string }) {
const t = useT();
const { outcomeFor, copy } = useCopyFeedback<string>();
- const canCopy = model.trim().length > 0 && model !== "…";
+ const canCopy = model.trim().length > 0 && model !== NO_COMBO_ID_PLACEHOLDER;--- a/gui/src/components/combo-workspace-add-modal.tsx
+++ b/gui/src/components/combo-workspace-add-modal.tsx
@@
-import { EffortSelect, PublicModelPreview, StrategySeg, TargetEditor } from "./combo-workspace-controls";
+import { EffortSelect, NO_COMBO_ID_PLACEHOLDER, PublicModelPreview, StrategySeg, TargetEditor } from "./combo-workspace-controls";
@@
<PublicModelPreview
- model={draft.id.trim() ? comboPublicModelId(draft.id, draft.alias) : "…"}
+ model={draft.id.trim() ? comboPublicModelId(draft.id, draft.alias) : NO_COMBO_ID_PLACEHOLDER}
/>--- a/gui/src/components/combo-workspace-detail-panel.tsx
+++ b/gui/src/components/combo-workspace-detail-panel.tsx
@@
-import { EffortSelect, PublicModelPreview, StrategySeg, TargetEditor } from "./combo-workspace-controls";
+import { EffortSelect, NO_COMBO_ID_PLACEHOLDER, PublicModelPreview, StrategySeg, TargetEditor } from "./combo-workspace-controls";
@@
<PublicModelPreview
- model={draft.id.trim() ? comboPublicModelId(draft.id, draft.alias) : "…"}
+ model={draft.id.trim() ? comboPublicModelId(draft.id, draft.alias) : NO_COMBO_ID_PLACEHOLDER}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** Effective public model id clients will request — mono value + copy. */ | |
| export function PublicModelPreview({ model }: { model: string }) { | |
| const t = useT(); | |
| const { outcomeFor, copy } = useCopyFeedback<string>(); | |
| const canCopy = model.trim().length > 0 && model !== "…"; | |
| const outcome = outcomeFor(model); | |
| const copyLabel = outcome === "copied" ? t("cws.copiedPublicModel") : t("cws.copyPublicModel"); | |
| // Split around a sentinel so the model token stays mono in any locale word order. | |
| const sentinel = "\u0001"; | |
| const [before, after = ""] = t("cws.field.publicModelPreview", { model: sentinel }).split(sentinel); | |
| return ( | |
| <div className="cwi-public-model-preview"> | |
| <p className="muted cwi-public-model-preview-text"> | |
| {before} | |
| <code className="mono cwi-public-model-preview-value">{model}</code> | |
| {after} | |
| </p> | |
| <button | |
| type="button" | |
| className="btn btn-ghost btn-sm cwi-public-model-preview-copy" | |
| disabled={!canCopy} | |
| onClick={() => { | |
| if (canCopy) copy(model, model); | |
| }} | |
| title={copyLabel} | |
| > | |
| <span aria-live="polite">{copyLabel}</span> | |
| </button> | |
| </div> | |
| ); | |
| } | |
| export const NO_COMBO_ID_PLACEHOLDER = "…"; | |
| /** Effective public model id clients will request — mono value + copy. */ | |
| export function PublicModelPreview({ model }: { model: string }) { | |
| const t = useT(); | |
| const { outcomeFor, copy } = useCopyFeedback<string>(); | |
| const canCopy = model.trim().length > 0 && model !== NO_COMBO_ID_PLACEHOLDER; | |
| const outcome = outcomeFor(model); | |
| const copyLabel = outcome === "copied" ? t("cws.copiedPublicModel") : t("cws.copyPublicModel"); | |
| // Split around a sentinel so the model token stays mono in any locale word order. | |
| const sentinel = "\u0001"; | |
| const [before, after = ""] = t("cws.field.publicModelPreview", { model: sentinel }).split(sentinel); | |
| return ( | |
| <div className="cwi-public-model-preview"> | |
| <p className="muted cwi-public-model-preview-text"> | |
| {before} | |
| <code className="mono cwi-public-model-preview-value">{model}</code> | |
| {after} | |
| </p> | |
| <button | |
| type="button" | |
| className="btn btn-ghost btn-sm cwi-public-model-preview-copy" | |
| disabled={!canCopy} | |
| onClick={() => { | |
| if (canCopy) copy(model, model); | |
| }} | |
| title={copyLabel} | |
| > | |
| <span aria-live="polite">{copyLabel}</span> | |
| </button> | |
| </div> | |
| ); | |
| } |
| /** Effective public model id clients will request — mono value + copy. */ | |
| export function PublicModelPreview({ model }: { model: string }) { | |
| const t = useT(); | |
| const { outcomeFor, copy } = useCopyFeedback<string>(); | |
| const canCopy = model.trim().length > 0 && model !== "…"; | |
| const outcome = outcomeFor(model); | |
| const copyLabel = outcome === "copied" ? t("cws.copiedPublicModel") : t("cws.copyPublicModel"); | |
| // Split around a sentinel so the model token stays mono in any locale word order. | |
| const sentinel = "\u0001"; | |
| const [before, after = ""] = t("cws.field.publicModelPreview", { model: sentinel }).split(sentinel); | |
| return ( | |
| <div className="cwi-public-model-preview"> | |
| <p className="muted cwi-public-model-preview-text"> | |
| {before} | |
| <code className="mono cwi-public-model-preview-value">{model}</code> | |
| {after} | |
| </p> | |
| <button | |
| type="button" | |
| className="btn btn-ghost btn-sm cwi-public-model-preview-copy" | |
| disabled={!canCopy} | |
| onClick={() => { | |
| if (canCopy) copy(model, model); | |
| }} | |
| title={copyLabel} | |
| > | |
| <span aria-live="polite">{copyLabel}</span> | |
| </button> | |
| </div> | |
| ); | |
| } | |
| <PublicModelPreview | |
| model={draft.id.trim() ? comboPublicModelId(draft.id, draft.alias) : NO_COMBO_ID_PLACEHOLDER} | |
| /> |
| /** Effective public model id clients will request — mono value + copy. */ | |
| export function PublicModelPreview({ model }: { model: string }) { | |
| const t = useT(); | |
| const { outcomeFor, copy } = useCopyFeedback<string>(); | |
| const canCopy = model.trim().length > 0 && model !== "…"; | |
| const outcome = outcomeFor(model); | |
| const copyLabel = outcome === "copied" ? t("cws.copiedPublicModel") : t("cws.copyPublicModel"); | |
| // Split around a sentinel so the model token stays mono in any locale word order. | |
| const sentinel = "\u0001"; | |
| const [before, after = ""] = t("cws.field.publicModelPreview", { model: sentinel }).split(sentinel); | |
| return ( | |
| <div className="cwi-public-model-preview"> | |
| <p className="muted cwi-public-model-preview-text"> | |
| {before} | |
| <code className="mono cwi-public-model-preview-value">{model}</code> | |
| {after} | |
| </p> | |
| <button | |
| type="button" | |
| className="btn btn-ghost btn-sm cwi-public-model-preview-copy" | |
| disabled={!canCopy} | |
| onClick={() => { | |
| if (canCopy) copy(model, model); | |
| }} | |
| title={copyLabel} | |
| > | |
| <span aria-live="polite">{copyLabel}</span> | |
| </button> | |
| </div> | |
| ); | |
| } | |
| <PublicModelPreview | |
| model={draft.id.trim() ? comboPublicModelId(draft.id, draft.alias) : NO_COMBO_ID_PLACEHOLDER} | |
| /> |
📍 Affects 3 files
gui/src/components/combo-workspace-controls.tsx#L266-L298(this comment)gui/src/components/combo-workspace-add-modal.tsx#L142-L144gui/src/components/combo-workspace-detail-panel.tsx#L215-L217
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/components/combo-workspace-controls.tsx` around lines 266 - 298,
Define and export a shared NO_COMBO_ID_PLACEHOLDER constant in
combo-workspace-controls.tsx, use it in PublicModelPreview’s canCopy comparison,
and import it in combo-workspace-add-modal.tsx (lines 142-144) and
combo-workspace-detail-panel.tsx (lines 215-217) to replace their duplicated
placeholder literals.
Source: Path instructions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f97b9c2003
ℹ️ 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".
| When the target's catalog effort ladder is unknown (metadata missing), the default is injected | ||
| optimistically — incomplete discovery is more common than a provider that truly lacks the effort. | ||
| When a *known* ladder is present and does not include the configured value, opencodex omits the |
There was a problem hiding this comment.
This paragraph now documents that unknown effort ladders receive the default, but the localized docs still ship the opposite rule (for example docs-site/src/content/docs/ko/guides/combos.md:128-132 and the localized routing tables still require an advertised rung). Fresh evidence since the earlier note is that the English source was updated here while the translated pages were not, so non-English users will configure combos expecting defaults to be omitted when runtime now injects them; update the translated combo guide/routing reference and the remaining defaultEffort table text with the same unknown-ladder semantics.
AGENTS.md reference: docs-site/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| - a positive `contextWindow`, from live metadata, registry hints, provider | ||
| `modelContextWindows` / `contextWindow`, or — when the provider is known but every source | ||
| omits a window — a conservative 128,000-token fallback (clamped by `providerContextCaps` when | ||
| set); and |
There was a problem hiding this comment.
This English fallback now allows known-provider targets without context metadata to catalog at 128k, but localized routing references still state that a positive contextWindow must come only from live/registry/provider config and that bare relay IDs with no context metadata are omitted (for example docs-site/src/content/docs/ko/reference/configuration/routing.md:81-84 and docs-site/src/content/docs/zh-cn/reference/configuration/routing.md:85-86). Fresh evidence: only the English page changed in this commit, leaving non-English docs to describe the old Needs attention behavior for recovered combos; update the translated routing pages to include the fallback and cap semantics.
AGENTS.md reference: docs-site/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| // Skip unknown / empty ladders so they do not empty the picker. | ||
| if (listed === undefined || listed.length === 0) continue; |
There was a problem hiding this comment.
When a selected model has a known empty ladder (for example a noReasoningModels catalog row, where runtime supportedLadderFor returns [] and concreteComboRequestBody omits any combo default), this branch skips it the same as unknown metadata. If all complete targets are known no-reasoning, common stays null and the picker offers the full ladder even though every saved default will be dropped at request time; distinguish undefined from [] so known empty ladders produce no selectable efforts.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| When the target's catalog effort ladder is unknown (metadata missing), the default is injected | ||
| optimistically — incomplete discovery is more common than a provider that truly lacks the effort. | ||
| When a *known* ladder is present and does not include the configured value, opencodex omits the |
There was a problem hiding this comment.
This section now says unknown target ladders receive the configured default, but the unchanged configuration tables still say defaultEffort applies only when the target advertises support (docs-site/src/content/docs/guides/combos.md:260 and docs-site/src/content/docs/reference/configuration/routing.md:65). Fresh evidence since the earlier docs fix is that only the explanatory paragraph was updated here, so readers scanning the reference tables will still configure combos using the old omission rule; update those table cells to match the new unknown-ladder behavior.
AGENTS.md reference: docs-site/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
Unknown or empty member effort ladders no longer empty the combo default effort picker, and runtime injects the default when the target ladder is unknown. Incomplete discovery rows get a conservative context-window fallback so multi-target combos stay in the public catalog. Combo ID and public model fields now show the effective client model name with copy.
…nd context cap Use shared useCopyFeedback for the combo detail header chip (copied + unavailable), clamp COMBO_MEMBER_CONTEXT_FALLBACK with provider contextCap, and document optimistic defaultEffort injection for unknown ladders.
Address PR lidge-jun#1092 maintainer feedback: restore request-time fail-closed defaultEffort injection for unknown ladders while keeping unknown vs unsupported debug discrimination, and mark unknown-capability targets in the GUI effort picker. Also add per-combo image/multimodal control (imageInput auto|disabled, default auto/checked): catalog strips image when disabled, management API persists only explicit disabled, and requests with image content are rejected before dispatch.
f97b9c2 to
0f9c7a0
Compare
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Follow-up (maintainer review + imageInput)Addressed @lidge-jun's request on runtime defaults:
Also shipped the optional capability assertion shape for multimodal:
Verification
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/combos.md`:
- Around line 168-174: Add the missing imageInput row to the configuration
reference table in combos.md, matching the corresponding zh-cn table entry and
documenting its supported values and default. Keep the description consistent
with the Image / multimodal capability section and current CLI/API behavior.
In `@docs-site/src/content/docs/zh-cn/reference/configuration/routing.md`:
- Line 60: Update the Chinese catalog-eligibility section to match the English
behavior: known providers may use the conservative 128,000-token context
fallback, clamped by providerContextCaps when configured. State that exclusion
applies only to unknown or disabled providers lacking a discovery row, or to
incompatible modalities, and remove the claim that missing context metadata
alone excludes the combo.
In `@gui/src/combo-capabilities.ts`:
- Around line 5-14: Add unit coverage for comboImagesSupported with three cases:
return false for no targets, true when every complete target’s catalog model
supports image input, and false when any target is absent from the catalog. Keep
the tests focused on validating the empty guard and all-targets requirement.
In `@gui/src/pages/Combos.tsx`:
- Around line 136-147: Add an optional inputModalities string-array property to
the ModelOption type, then retain the existing inputModalities assignment in the
model parsing and models.push flow so the capability metadata satisfies
TypeScript.
In `@src/codex/catalog/provider-fetch.ts`:
- Around line 633-646: Update the member resolution flow around existing and
prov so provider policy is evaluated before returning a complete member: return
undefined when the provider is disabled, otherwise apply applyProviderContextCap
to complete existing members before returning them so contextWindow and
maxInputTokens honor configured caps. Add regression coverage for both a capped
complete member and a complete member whose provider is disabled, using the
shared routing/configuration layers.
In `@src/server/responses/core.ts`:
- Around line 994-996: Update handleResponsesInner and the pre-dispatch
validation around combo.imageInput to expand previous_response_id input before
handleComboResponses and reject any replayed images when imageInput is
"disabled"; preserve the existing 400 invalid_request_error response and ensure
validation occurs before any upstream call. Add an end-to-end test covering an
image-bearing prior response followed by a previous_response_id-only
continuation after disabling images, asserting 400 and no additional upstream
invocation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3195269e-5804-40f5-8701-0d1e3c215652
📒 Files selected for processing (32)
docs-site/src/content/docs/guides/combos.mddocs-site/src/content/docs/reference/configuration/routing.mddocs-site/src/content/docs/zh-cn/guides/combos.mddocs-site/src/content/docs/zh-cn/reference/configuration/routing.mdgui/src/combo-capabilities.tsgui/src/combo-workspace-data.tsgui/src/components/combo-workspace-add-modal.tsxgui/src/components/combo-workspace-controls.tsxgui/src/components/combo-workspace-detail-panel.tsxgui/src/components/combo-workspace-types.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/pages/Combos.tsxgui/src/styles-combos-workspace.csssrc/codex/catalog.tssrc/codex/catalog/aggregation.tssrc/codex/catalog/provider-fetch.tssrc/combos/index.tssrc/combos/request.tssrc/combos/types.tssrc/server/management/combo-routes.tssrc/server/responses/core.tssrc/types.tstests/codex-catalog.test.tstests/combo-management-api.test.tstests/combo-workspace-data.test.tstests/combos.test.tstests/server-combo-failover-e2e.test.ts
| ## Image / multimodal capability | ||
|
|
||
| By default a combo publishes the **intersection** of its targets' input modalities (image is | ||
| enabled only when every target advertises it). Set `imageInput: "disabled"` to force text-only | ||
| even when every target supports images — the catalog drops `image` from `inputModalities`, and | ||
| image-bearing requests are rejected with HTTP 400 before any target is called. `"auto"` (or | ||
| omitting the field) keeps the automatic intersection. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add imageInput to the configuration reference table.
This section documents imageInput, but the field table at Lines 261-268 omits it. Users who rely on the configuration reference will not see the supported values or default. Add the same imageInput row that exists in docs-site/src/content/docs/zh-cn/guides/combos.md Line 217.
As per path instructions, user-facing documentation must stay in sync with actual CLI and API behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/guides/combos.md` around lines 168 - 174, Add the
missing imageInput row to the configuration reference table in combos.md,
matching the corresponding zh-cn table entry and documenting its supported
values and default. Keep the description consistent with the Image / multimodal
capability section and current CLI/API behavior.
Source: Path instructions
| | `strategy?` | `"failover" \| "round-robin"` | `"failover"` | 选择策略。目标顺序表示故障切换优先级；权重会影响平滑加权轮询。 | | ||
| | `stickyLimit?` | `number` | `1` | 在单个轮询批次中保留的成功请求数。范围 1–100。 | | ||
| | `defaultEffort?` | `"low" \| "medium" \| "high" \| "xhigh" \| "max" \| "ultra" \| null` | unset | 仅在调用方省略 effort 且所选目标声明了请求的档位时应用。 | | ||
| | `imageInput?` | `"auto" \| "disabled"` | `"auto"` | `"disabled"` 会从对外能力中去掉图片，并在分发前拒绝带图请求；不能在目标不支持时强开图片。 | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the Chinese catalog-eligibility text.
Lines 86-92 still state that a target without context metadata removes the combo from the catalog. docs-site/src/content/docs/reference/configuration/routing.md Lines 186-195 now allow a known provider to use a conservative 128,000-token fallback, clamped by providerContextCaps when configured. Update the Chinese section to describe that fallback and retain exclusion only for unknown or disabled providers without a discovery row, or incompatible modalities.
As per path instructions, translated locale pages must not contradict the English source. Based on learnings, flag localized content that conflicts with the English behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/zh-cn/reference/configuration/routing.md` at line
60, Update the Chinese catalog-eligibility section to match the English
behavior: known providers may use the conservative 128,000-token context
fallback, clamped by providerContextCaps when configured. State that exclusion
applies only to unknown or disabled providers lacking a discovery row, or to
incompatible modalities, and remove the claim that missing context metadata
alone excludes the combo.
Sources: Path instructions, Learnings
| export function comboImagesSupported(targets: ComboTarget[], models: ModelOption[]): boolean { | ||
| const complete = targets.filter((target) => target.provider.trim() && target.model.trim()); | ||
| if (complete.length === 0) return false; | ||
| return complete.every((target) => { | ||
| const model = models.find( | ||
| (row) => row.provider === target.provider.trim() && row.id === target.model.trim(), | ||
| ); | ||
| return !!model?.inputModalities?.includes("image"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a unit test for the fail-closed branches of comboImagesSupported.
No test file covers this function in the current batch. This function gates whether the image-input toggle appears enabled in the workspace UI, so a regression here (for example, accidentally using .some instead of .every, or dropping the complete.length === 0 guard) would silently misreport capability support. Add a small test with three cases: no targets, all targets supporting images, and one target missing from the catalog.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/combo-capabilities.ts` around lines 5 - 14, Add unit coverage for
comboImagesSupported with three cases: return false for no targets, true when
every complete target’s catalog model supports image input, and false when any
target is absent from the catalog. Keep the tests focused on validating the
empty guard and all-targets requirement.
| const inputModalities = Array.isArray(model.inputModalities) | ||
| ? model.inputModalities | ||
| .filter((modality): modality is string => typeof modality === "string") | ||
| .map((modality) => modality.trim()) | ||
| .filter(Boolean) | ||
| : undefined; | ||
| models.push({ | ||
| provider, | ||
| id, | ||
| namespaced: typeof model.namespaced === "string" ? model.namespaced : undefined, | ||
| ...(reasoningEfforts ? { reasoningEfforts } : {}), | ||
| ...(inputModalities && inputModalities.length > 0 ? { inputModalities } : {}), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 'type ModelOption|inputModalities' \
gui/src/pages/Combos.tsx \
gui/src/components/combo-workspace-controls.tsxRepository: lidge-jun/opencodex
Length of output: 2183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- Combos.tsx relevant types and mappings ---\n'
sed -n '1,220p' gui/src/pages/Combos.tsx | nl -ba | sed -n '18,180p'
printf '\n--- model list usages ---\n'
rg -n -C 2 'models|ModelOption|ComboCapabilities|useQuery|fetch.*model|models\.' gui/src/pages/Combos.tsx
printf '\n--- package TypeScript config availability ---\n'
for f in gui/package.json gui/tsconfig.json gui/tsconfig.node.json gui/vite.config.ts; do
[ -f "$f" ] && { echo "### $f"; sed -n '1,120p' "$f"; }
doneRepository: lidge-jun/opencodex
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Combos.tsx relevant types and mappings ---'
sed -n '1,220p' gui/src/pages/Combos.tsx | nl -ba | sed -n '18,180p'
printf '%s\n' ''
printf '%s\n' '--- model list usages ---'
rg -n -C 2 'models|ModelOption|ComboCapabilities|useQuery|fetch.*model|models\.' gui/src/pages/Combos.tsx
printf '%s\n' ''
printf '%s\n' '--- package TypeScript config availability ---'
for f in gui/package.json gui/tsconfig.json gui/tsconfig.node.json gui/vite.config.ts; do
[ -f "$f" ] && { echo "### $f"; sed -n '1,120p' "$f"; }
doneRepository: lidge-jun/opencodex
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Combos.tsx relevant types and mappings ---'
awk 'NR>=18 && NR<=180 { printf "%6d %s\n", NR, $0 }' gui/src/pages/Combos.tsx
echo
echo '--- model list usages ---'
rg -n -C 2 'models|ModelOption|ComboCapabilities|useQuery|fetch.*model|models\.' gui/src/pages/Combos.tsx || true
echo
echo '--- package TypeScript config availability ---'
for f in gui/package.json gui/tsconfig.json gui/tsconfig.node.json gui/vite.config.ts; do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,120p' "$f"
fi
doneRepository: lidge-jun/opencodex
Length of output: 13587
Add inputModalities to ModelOption.
gui/src/pages/Combos.tsx:142-147 pushes parsed model capability metadata into models: ModelOption[], but ModelOption at gui/src/pages/Combos.tsx:24 only allows provider, id, namespaced, and reasoningEfforts. TypeScript rejects this assignment when building tsc -b.
Proposed fix
-type ModelOption = { provider: string; id: string; namespaced?: string; reasoningEfforts?: string[] };
+type ModelOption = {
+ provider: string;
+ id: string;
+ namespaced?: string;
+ reasoningEfforts?: string[];
+ inputModalities?: string[];
+};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/pages/Combos.tsx` around lines 136 - 147, Add an optional
inputModalities string-array property to the ModelOption type, then retain the
existing inputModalities assignment in the model parsing and models.push flow so
the capability metadata satisfies TypeScript.
| const existing = memberByKey.get(targetKey(target)); | ||
| if ( | ||
| existing | ||
| && typeof existing.contextWindow === "number" | ||
| && existing.contextWindow > 0 | ||
| ) { | ||
| return existing; | ||
| } | ||
|
|
||
| const prov = providers.get(target.provider); | ||
| // Disabled providers cannot contribute usable members. Missing provider with | ||
| // an incomplete existing row still needs the conservative fallback below so | ||
| // a configured target is not silently dropped solely for missing ctx. | ||
| if (prov?.disabled === true) return existing; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Apply provider policy before returning a complete member.
Lines 633-640 return a complete existing member before Lines 642-646 check whether its provider is disabled. This can advertise a combo that contains a disabled provider.
The same return bypasses contextCap. This affects injected native OpenAI members because they enter memberByKey with a positive contextWindow. A configured provider cap must lower their advertised contextWindow and maxInputTokens.
Return undefined for a disabled provider. Apply applyProviderContextCap to complete members before returning them. Add regression coverage for a complete member with a cap and for a complete member on a disabled provider.
Proposed fix
- if (
- existing
- && typeof existing.contextWindow === "number"
- && existing.contextWindow > 0
- ) {
- return existing;
- }
-
const prov = providers.get(target.provider);
- if (prov?.disabled === true) return existing;
+ if (prov?.disabled === true) return undefined;
+ if (
+ existing
+ && typeof existing.contextWindow === "number"
+ && existing.contextWindow > 0
+ ) {
+ const contextWindow = applyProviderContextCap(existing.contextWindow, contextCap)
+ ?? existing.contextWindow;
+ if (contextWindow === existing.contextWindow) return existing;
+ return {
+ ...existing,
+ contextWindow,
+ maxInputTokens: Math.min(existing.maxInputTokens ?? contextWindow, contextWindow),
+ contextCap,
+ contextCapped: true,
+ };
+ }As per path instructions, src/** changes must not bypass shared routing/config layers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const existing = memberByKey.get(targetKey(target)); | |
| if ( | |
| existing | |
| && typeof existing.contextWindow === "number" | |
| && existing.contextWindow > 0 | |
| ) { | |
| return existing; | |
| } | |
| const prov = providers.get(target.provider); | |
| // Disabled providers cannot contribute usable members. Missing provider with | |
| // an incomplete existing row still needs the conservative fallback below so | |
| // a configured target is not silently dropped solely for missing ctx. | |
| if (prov?.disabled === true) return existing; | |
| const existing = memberByKey.get(targetKey(target)); | |
| const prov = providers.get(target.provider); | |
| // Disabled providers cannot contribute usable members. Missing provider with | |
| // an incomplete existing row still needs the conservative fallback below so | |
| // a configured target is not silently dropped solely for missing ctx. | |
| if (prov?.disabled === true) return undefined; | |
| if ( | |
| existing | |
| && typeof existing.contextWindow === "number" | |
| && existing.contextWindow > 0 | |
| ) { | |
| const contextWindow = applyProviderContextCap(existing.contextWindow, contextCap) | |
| ?? existing.contextWindow; | |
| if (contextWindow === existing.contextWindow) return existing; | |
| return { | |
| ...existing, | |
| contextWindow, | |
| maxInputTokens: Math.min(existing.maxInputTokens ?? contextWindow, contextWindow), | |
| contextCap, | |
| contextCapped: true, | |
| }; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/codex/catalog/provider-fetch.ts` around lines 633 - 646, Update the
member resolution flow around existing and prov so provider policy is evaluated
before returning a complete member: return undefined when the provider is
disabled, otherwise apply applyProviderContextCap to complete existing members
before returning them so contextWindow and maxInputTokens honor configured caps.
Add regression coverage for both a capped complete member and a complete member
whose provider is disabled, using the shared routing/configuration layers.
Source: Path instructions
| if (combo.imageInput === "disabled" && comboRequestHasImageInput(rawBody)) { | ||
| return formatErrorResponse(400, "invalid_request_error", `Combo "${comboId}" does not accept image input`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject replay-expanded images before target dispatch.
Lines 994-996 inspect only the raw request body. A continuation request can contain no inline input_image and reference a prior response whose stored input contains images.
handleResponsesInner expands previous_response_id after combo dispatch. The child request then expands the image-bearing input with comboAttempt: true, so it does not run this combo policy again. The target can receive images even when imageInput is "disabled".
Expand and validate replayed input before dispatching to handleComboResponses, or pass the disabled-image policy into child dispatch and validate after expansion. Add an end-to-end test that creates an image-bearing response, changes the combo to "disabled", then sends a continuation containing only previous_response_id. The continuation must return 400 with no additional upstream call.
As per path instructions, docs-site/src/content/docs/reference/adapters.md requires unsupported image requests to be rejected before upstream invocation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/responses/core.ts` around lines 994 - 996, Update
handleResponsesInner and the pre-dispatch validation around combo.imageInput to
expand previous_response_id input before handleComboResponses and reject any
replayed images when imageInput is "disabled"; preserve the existing 400
invalid_request_error response and ensure validation occurs before any upstream
call. Add an end-to-end test covering an image-bearing prior response followed
by a previous_response_id-only continuation after disabling images, asserting
400 and no additional upstream invocation.
Source: Path instructions
Second loop of the bug campaign, scoped to author corrected replacements for
four contributor PRs and close theirs as absorbed. Three adversarial audit
rounds refuted the premise, and the plan now records that instead of the
outcome it was written to produce.
The decisive finding: I judged "has the author responded to our review" by
`updatedAt`, which moves when WE comment and therefore can never show author
activity. Comparing last-commit time against review time gives the real
picture:
#1092 commits 09:38:19Z review 09:09:51Z -> acted, in under 30 minutes
#1068 commits 08:52:23Z review 09:13:15Z -> predates the review
#1036 commits 08-05 review 09:12:51Z -> no response yet
#997 commits 02:51Z review 09:16:02Z -> no response yet
Under the wrong reading I was about to close #1092 as absorbed — taking
credit for work its author did in direct response to my own request — and to
credit #1068 with agreeing to a review it had not yet read. Neither is a
process nit; both would have been visible to the contributor.
So the plan changes shape. #1092 and #1068 become re-reviews. #1036 and #997
get a stated 72-hour response window with a mandatory head re-check before
anything is authored or closed; "has not replied within an hour" is not
abandonment, and #997's author was active at 02:51Z. This loop therefore
authors no absorbed layer at all, and says so.
#1068's re-review carries a finding neither side has: its new test asserts
all three DeepSeek ids are in `noVisionModels`, but routing merges the
registry list, which holds only the `-free` one. Reproduced with the test's
own routeModel config — Pro=false, Flash=false, Flash-free=true — so two of
three cases fail. Latent because no check currently runs that suite.
Also corrected: the layers were called dependency-ordered when they share no
files, so s3/s4 are independent heads off `origin/dev` and the
`--update-refs` cascade is reserved for genuine chains; and #978's exclusion
no longer claims it is "already correct" when it needs an author-side docs
change.
Terminal outcome is deferred, not DONE. Shrinking the criteria to match what
finished would have hidden exactly the thing worth recording.
|
You turned this around in under half an hour, and the fix is exactly right. Recording that plainly, because a fast correct response deserves to be acknowledged rather than met with silence. The guard is back: // Fail closed for both unknown (`undefined`) and known-but-missing ladders.
// Unknown is kept distinct in debug so we can measure thin catalog rows without
// guessing the provider accepts a reasoning field.
if (!targetReasoningEfforts?.includes(defaultEffort)) {Keeping Two things left, neither of them about the picker. 1. Please split the 2. One case worth double-checking in the intersection. Make sure a known but empty ladder still constrains the result. Skipping on On the mechanics: your branch is now on Good work. The picker bug was real, your diagnosis of |
Records what the stack-and-absorb campaign actually produced once live data replaced its assumptions. #1068 merged at 10:45:57Z while I was writing its review, and the merged code is not what I reviewed: `noVisionModels: [...OPENCODE_ZEN_TEXT_ONLY_MODELS, ...DEEPSEEK_THINKING_MODELS]` — the union the review asked for. Running the merged suite against `origin/dev` gives 9 pass / 0 fail, including a new test pinning the six probed text-only models. My 10:53Z comment claiming a failing test was true of the head I had fetched and false of what landed, so it got a public correction eight minutes later. That is the second process error in this unit, and both are now written down rather than quietly fixed. First: judging author activity by `updatedAt`, which moves when WE comment. Second: commenting against a stale fetched head. Each produced a wrong public statement to a contributor. The rules are re-fetch immediately before commenting, and compare last-commit time against review time. #1092's author restored the fail-closed guard within 30 minutes of the review, with a comment keeping `unknown` distinct in debug. Credited explicitly; asked only to split the unrelated `imageInput` scope. #1036 and #997 are deferred, not absorbed. Their last commits (08-05 07:18Z, 08-06 02:51Z) predate our reviews and neither carries the requested change, but roughly two hours have passed and #997's author was active that morning. 050 states a 72-hour response window with a mandatory head re-check before anything is authored or closed. The window exists because this unit demonstrated the alternative: two of four absorb targets were being actively fixed by their authors while we drafted replacements. #994 updated with the dev-only landing (merge 7d0c02d, ancestor of `origin/dev`) and deliberately left open pending the reporter's provider confirmation and a release. Zero contributor PRs closed by this loop.
2 participants
Summary
concreteComboRequestBodyno longer injectsdefaultEffortwhen the target ladder isundefined; debug logs keep distinctunknownvsunsupported.imageInput: "auto" | "disabled"(defaultauto, GUI checked when every target supports image). Disabled stripsimagefrom catalog modalities and rejects image-bearing requests with HTTP 400 before dispatch.Change graph
Screenshots
Local ego-browser verification on branch
fix/combo-effort-persist-ux(ephemeral proxy; not production:10100).Verification
bun test tests/combo-workspace-data.test.ts tests/combos.test.ts tests/codex-catalog.test.ts tests/combo-management-api.test.ts tests/server-combo-failover-e2e.test.ts(248 pass)upstream/devChecklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I fixed all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation