feat(combos): add imageInput auto|disabled capability control - #1165
feat(combos): add imageInput auto|disabled capability control#1165eachann1024 wants to merge 2 commits into
Conversation
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
✅ Deterministic PR hygiene checks passed. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughCombos now support automatic or disabled image input. The server validates and rejects disallowed image requests before dispatch. The catalog, persistence layer, GUI controls, localized strings, documentation, and tests reflect the new setting. ChangesCombo image-input support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ComboResponseHandler
participant ImageInputDetector
participant ComboTarget
Client->>ComboResponseHandler: Submit combo request
ComboResponseHandler->>ComboResponseHandler: Expand previous response state
ComboResponseHandler->>ImageInputDetector: Inspect expanded request body
ImageInputDetector-->>ComboResponseHandler: Return image-input presence
ComboResponseHandler-->>Client: Return HTTP 400 when image input is disabled
ComboResponseHandler->>ComboTarget: Dispatch expanded request when allowed
Possibly related PRs
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
gui/src/components/combo-workspace-types.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the canonical
ModelOptiontype instead of redefining it.
gui/src/components/combo-workspace-types.tsexportsModelOption, butgui/src/pages/Combos.tsxdeclares a separate, identical local type. This PR had to addinputModalities?: string[]by hand in both places (and, per the codebase graph, in several other files too). A future field addition can silently drift out of sync in one of the copies.
gui/src/components/combo-workspace-types.ts#L11-17: keep this as the single canonicalModelOptionexport.gui/src/pages/Combos.tsx#L24-24: replace the localtype ModelOption = { ... }declaration withimport type { ModelOption } from "../components/combo-workspace-types";.♻️ Proposed fix
-type ModelOption = { provider: string; id: string; namespaced?: string; reasoningEfforts?: string[]; inputModalities?: string[] }; +import type { ModelOption } from "../components/combo-workspace-types";🤖 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-types.ts` at line 1, Use the canonical ModelOption type exported by combo-workspace-types.ts as the single source of truth. In Combos.tsx, remove the local ModelOption declaration and import the type from ../components/combo-workspace-types, preserving all existing usages.
🤖 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-capabilities.ts`:
- Around line 5-12: Update comboImagesSupported so incomplete targets are not
filtered out before evaluation: require every target to have nonblank provider
and model values and a matching model advertising image input, while preserving
false for an empty target list. Add a regression test covering one
vision-capable target alongside one incomplete target and verify the result is
false.
In `@src/server/management/combo-routes.ts`:
- Around line 125-134: Separate management API response objects from
runtime-normalized combo objects in the GET /api/combos flow and the PUT
response path. Ensure both responses omit imageInput when it is "auto" and
include it only when explicitly "disabled", while preserving normalized values
for runtime and persistence. Update the affected test expectation in
combo-management-api.test.ts accordingly.
In `@src/server/responses/core.ts`:
- Around line 994-1007: Update the continuation handling around
expandPreviousResponseInput and previousResponseReplayFailure so an unresolved
previous_response_id is rejected in this handler before image policy validation
or child dispatch; ensure image-bearing stored continuations are expanded and
still fail when imageInput is disabled, and add regressions covering both that
case and an unavailable previous_response_id.
In `@src/types.ts`:
- Around line 844-849: Update the normalizedCombo() fixture in
tests/codex-catalog.test.ts to include the required imageInput property, using
the normalized default value expected by NormalizedComboConfig. Preserve the
fixture’s existing object spread and other fields so it remains type-correct.
In `@tests/combo-workspace-data.test.ts`:
- Around line 437-468: Add regression coverage for the disabled image-input
state: test that parseComboList preserves "disabled", draftEquals treats
"disabled" and "auto" as different, and toPutBody emits imageInput: "disabled"
only for disabled drafts while omitting the default auto value. Anchor the tests
to the existing parseComboList, draftEquals, and toPutBody symbols and retain
the current auto-state expectations.
---
Outside diff comments:
In `@gui/src/components/combo-workspace-types.ts`:
- Line 1: Use the canonical ModelOption type exported by
combo-workspace-types.ts as the single source of truth. In Combos.tsx, remove
the local ModelOption declaration and import the type from
../components/combo-workspace-types, preserving all existing usages.
🪄 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: c1cddfda-f280-47e8-9b07-d59f09001dce
📒 Files selected for processing (29)
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/aggregation.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/server-combo-failover-e2e.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: 4849dde79f
ℹ️ 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 targetRoute = routeModel(config, `${pick.target.provider}/${pick.target.model}`); | ||
| const childBody = concreteComboRequestBody( | ||
| rawBody, | ||
| body, |
There was a problem hiding this comment.
When a combo request includes previous_response_id, this now passes an already-expanded body to the child request while leaving previous_response_id in place; the child handleResponsesInner reads that JSON and calls expandPreviousResponseInput again, so every remembered input item is prepended twice before the adapter parses it. This corrupts combo continuations and inflates context/tokens; strip previous_response_id after the parent expansion or only use the expanded body for the image check before dispatching.
Useful? React with 👍 / 👎.
| 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(), |
There was a problem hiding this comment.
For a combo target that uses a chatgpt/ChatGPT-forward provider, the target picker can show GPT models from the openai catalog row, but this exact provider match cannot find that row and reports imagesSupported=false. In that dashboard path, even vision-capable GPT targets show the image capability as unavailable, so users cannot turn on imageInput: "disabled" from the UI for those combos; reuse the same provider-key expansion as the picker or pass provider metadata into this check.
AGENTS.md reference: gui/AGENTS.md:L7-L10
Useful? React with 👍 / 👎.
| if (record.type === "input_image") return true; | ||
| return Object.values(record).some(visit); | ||
| }; | ||
| return visit(body); |
There was a problem hiding this comment.
With imageInput disabled this walks the entire request object, not just the actual input being sent to the model. A valid text-only Responses request that carries metadata or a tool schema/example containing { type: "input_image" } is therefore rejected with the combo image error even though no image input would be dispatched; scan only the expanded input content instead of the whole body.
Useful? React with 👍 / 👎.
Per-combo image/multimodal policy: default auto keeps the target intersection; disabled strips image from catalog modalities and rejects image-bearing requests (including previous_response_id expansions) with HTTP 400 before dispatch. Management API sparsifies only explicit disabled. GUI exposes a capability switch when every target supports images.
Fail closed for incomplete targets in comboImagesSupported; omit default imageInput auto from management GET/PUT responses; reject unresolved previous_response_id when images are disabled; cover disabled draft persistence and normalizedCombo fixture.
01694ef to
1beda09
Compare
|
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. |
|
|
|
|
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
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 `@docs-site/src/content/docs/guides/combos.md`:
- Line 267: Update the imageInput descriptions to use capability-intersection
wording: in docs-site/src/content/docs/guides/combos.md:267, state that "auto"
publishes image support only when every target supports images; apply the
equivalent automatic intersection rule in
docs-site/src/content/docs/reference/configuration/routing.md:66; use the same
meaning in Chinese at docs-site/src/content/docs/zh-cn/guides/combos.md:217 and
docs-site/src/content/docs/zh-cn/reference/configuration/routing.md:60. Do not
imply an "enabled" mode or add a separate validation rule.
In `@src/server/responses/core.ts`:
- Around line 1021-1029: Update the continuation check around
expandPreviousResponseInput and unresolvedPrevious so a non-empty
previous_response_id is considered unresolved only when expansion returned the
original body, not when it returned a successfully expanded continuation.
Preserve rejection for missing or corrupt state, allow stored text-only
continuations when combo.imageInput is disabled, and add a regression verifying
the target receives the expanded continuation.
🪄 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: 5febc4dc-fec4-4f69-a801-bcfe96737d52
📒 Files selected for processing (29)
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/aggregation.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/server-combo-failover-e2e.test.ts
| | `strategy` | No | `"failover"` | `"failover"` or `"round-robin"`. | | ||
| | `stickyLimit` | No | `1` | Integer from 1 to 100 successful requests per round-robin selection. | | ||
| | `defaultEffort` | No | `null` | `low`, `medium`, `high`, `xhigh`, `max`, or `ultra`; applied only when the caller omits effort and the target advertises support. | | ||
| | `imageInput` | No | `"auto"` | `"auto"` or `"disabled"`. `"disabled"` drops image from published modalities and rejects image-bearing requests before dispatch. Cannot enable image when a target lacks it. | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use capability-intersection wording in all configuration tables.
imageInput has no "enabled" value. "auto" derives image support from the intersection of target modalities, while "disabled" forces text-only behavior. The current wording can imply an unsupported configuration mode or an additional validation rule.
docs-site/src/content/docs/guides/combos.md#L267-L267: State that"auto"publishes image support only when every target supports images.docs-site/src/content/docs/reference/configuration/routing.md#L66-L66: Replace the enablement wording with the automatic intersection rule.docs-site/src/content/docs/zh-cn/guides/combos.md#L217-L217: Use the same capability-intersection wording in Chinese.docs-site/src/content/docs/zh-cn/reference/configuration/routing.md#L60-L60: Use the same capability-intersection wording in the localized reference table.
📍 Affects 4 files
docs-site/src/content/docs/guides/combos.md#L267-L267(this comment)docs-site/src/content/docs/reference/configuration/routing.md#L66-L66docs-site/src/content/docs/zh-cn/guides/combos.md#L217-L217docs-site/src/content/docs/zh-cn/reference/configuration/routing.md#L60-L60
🤖 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` at line 267, Update the
imageInput descriptions to use capability-intersection wording: in
docs-site/src/content/docs/guides/combos.md:267, state that "auto" publishes
image support only when every target supports images; apply the equivalent
automatic intersection rule in
docs-site/src/content/docs/reference/configuration/routing.md:66; use the same
meaning in Chinese at docs-site/src/content/docs/zh-cn/guides/combos.md:217 and
docs-site/src/content/docs/zh-cn/reference/configuration/routing.md:60. Do not
imply an "enabled" mode or add a separate validation rule.
Source: Path instructions
| const unresolvedPrevious = typeof (body as { previous_response_id?: unknown } | null)?.previous_response_id === "string" | ||
| && (body as { previous_response_id: string }).previous_response_id.trim().length > 0; | ||
| if (combo.imageInput === "disabled" && unresolvedPrevious) { | ||
| return formatErrorResponse( | ||
| 400, | ||
| "previous_response_not_found", | ||
| "Continuation state is unavailable or corrupt; resend the full conversation without previous_response_id.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not reject successfully expanded continuations.
expandPreviousResponseInput() preserves previous_response_id on its successful expanded object. Lines 1021-1022 therefore set unresolvedPrevious for both missing state and successfully materialized state. An image-disabled combo rejects every continuation, including a continuation whose prior input contains only text.
Treat the ID as unresolved only when expansion returned the original body. Add a regression with a stored text-only response and verify that the target receives the expanded continuation.
Proposed fix
- const unresolvedPrevious = typeof (body as { previous_response_id?: unknown } | null)?.previous_response_id === "string"
+ const unresolvedPrevious = body === rawBody
+ && typeof (body as { previous_response_id?: unknown } | null)?.previous_response_id === "string"
&& (body as { previous_response_id: string }).previous_response_id.trim().length > 0;📝 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 unresolvedPrevious = typeof (body as { previous_response_id?: unknown } | null)?.previous_response_id === "string" | |
| && (body as { previous_response_id: string }).previous_response_id.trim().length > 0; | |
| if (combo.imageInput === "disabled" && unresolvedPrevious) { | |
| return formatErrorResponse( | |
| 400, | |
| "previous_response_not_found", | |
| "Continuation state is unavailable or corrupt; resend the full conversation without previous_response_id.", | |
| ); | |
| } | |
| const unresolvedPrevious = body === rawBody | |
| && typeof (body as { previous_response_id?: unknown } | null)?.previous_response_id === "string" | |
| && (body as { previous_response_id: string }).previous_response_id.trim().length > 0; | |
| if (combo.imageInput === "disabled" && unresolvedPrevious) { | |
| return formatErrorResponse( | |
| 400, | |
| "previous_response_not_found", | |
| "Continuation state is unavailable or corrupt; resend the full conversation without previous_response_id.", | |
| ); | |
| } |
🤖 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 1021 - 1029, Update the
continuation check around expandPreviousResponseInput and unresolvedPrevious so
a non-empty previous_response_id is considered unresolved only when expansion
returned the original body, not when it returned a successfully expanded
continuation. Preserve rejection for missing or corrupt state, allow stored
text-only continuations when combo.imageInput is disabled, and add a regression
verifying the target receives the expanded continuation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1beda09f6e
ℹ️ 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 unresolvedPrevious = typeof (body as { previous_response_id?: unknown } | null)?.previous_response_id === "string" | ||
| && (body as { previous_response_id: string }).previous_response_id.trim().length > 0; | ||
| if (combo.imageInput === "disabled" && unresolvedPrevious) { |
There was a problem hiding this comment.
When imageInput is disabled and a client sends a previous_response_id that is still present in the local replay store, expandPreviousResponseInput() returns a new expanded body but leaves previous_response_id on it. This predicate still treats that successful expansion as unresolved and returns 400 before comboRequestHasImageInput() can inspect the replayed input, so even text-only continuations through a text-only combo are impossible. Track whether expansion missed, or strip the id after successful expansion, before applying this unresolved-state rejection.
Useful? React with 👍 / 👎.
| * Omitted / `"auto"` keeps automatic capability derivation (default: enabled when | ||
| * the target intersection includes image). | ||
| */ | ||
| imageInput?: "auto" | "disabled"; |
There was a problem hiding this comment.
Once a combo has imageInput: "disabled" from the dashboard/API/config, a later ocx combo set cannot include this new field: the CLI builds a full replacement body in src/cli/combo.ts with targets/strategy/sticky/effort/alias only, and the PUT path normalizes an omitted imageInput back to auto. In that CLI update scenario the text-only guard is silently removed, so add a CLI flag or round-trip the existing value before replacing the combo.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
1 participant
Summary
imageInput: "auto" | "disabled"(defaultauto)."disabled"stripsimagefrom published modalities and rejects image-bearing requests with HTTP 400 before dispatch.previous_response_idfirst so a continuation that only references prior images still fails closed."disabled".Split from #1092 per maintainer request (
imageInputmust not ride with the effort-picker fix).Change graph
Screenshots
GUI capability switch (from the original #1092 verification session; same control surface):
Verification
bun test tests/codex-catalog.test.ts tests/combo-management-api.test.ts tests/combo-workspace-data.test.ts tests/server-combo-failover-e2e.test.ts— 211 passbun run typecheck— exit 0Checklist
Related: #1092
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 resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Documentation