feat(combos): add imageInput auto|disabled capability control - #1165
feat(combos): add imageInput auto|disabled capability control#1165eachann1024 wants to merge 3 commits into
Conversation
⏳ DRAFT
What to do
Review readiness checklist
3/4 boxes ticked. GitHub CI is not green on the current head Hygiene✅ Deterministic PR hygiene checks passed. |
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughCombos now support automatic or disabled image input. The server validates image requests before dispatch, and the catalog, persistence layer, GUI, 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 💡 1🛠️ Fix failing CI checks 💡
🧪 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 👍 / 👎.
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
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 👍 / 👎.
Wibias
added
stale
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The per-combo capability is useful, and splitting it from #1092 keeps the server behavior reviewable. One runtime blocker remains on the current head, in addition to the branch conflicts with current dev.
comboRequestHasImageInput recursively walks the entire request object and returns true for any nested object whose type equals input_image. That includes unrelated surfaces such as tool JSON schemas, tool arguments, metadata, or extension payloads. An image-disabled combo can therefore reject a text-only request with HTTP 400 merely because a declared tool schema contains an enum/default/example like { "type": "input_image" }. Restrict detection to the actual Responses input message/content shapes after continuation expansion, rather than scanning every object value. Add a negative regression where a text-only request declares a tool schema or metadata containing the same string and confirm no rejection or pre-dispatch false positive.
Please also rebase onto the latest dev, resolve the current conflicts, and rerun the combo catalog/management/failover tests plus exact-head CI. The GUI control, copy, and visual placement still require @lidge-jun's final UI review; this request is limited to the runtime false-positive and integration state.
|
@eachann1024 please update this PR if you still want this or I will have to close it very soon. Thanks for your contribution! |
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.
Restrict image-disabled detection to materialised Responses input after previous_response_id expansion so tool schemas and metadata cannot false- positive HTTP 400. Treat unresolved continuations by object identity, strip previous_response_id after a successful expand to avoid double replay on child dispatch, and align docs with capability-intersection wording.
1beda09 to
3db3c78
Compare
|
Addressed the runtime blocker and rebased onto current Runtime
Integration
Verification
Head: |
|
|
|
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. |
|
CI still needs to be fixed! |
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 `@tests/codex-catalog.test.ts`:
- Around line 159-168: Add a focused case to the existing imageInput
disabled-mode test using members whose inputModalities are only ["image"], and
assert deriveComboCatalogModel returns null with imageInput set to "disabled".
Keep the current shared text/image assertions unchanged.
In `@tests/server-combo-failover-e2e.test.ts`:
- Around line 1374-1404: Add a focused regression test near the existing
disabled-image continuation test, storing response state whose materialized
input contains input_image content, then continuing it through a combo
configured with imageInput disabled. Assert the request returns HTTP 400 and the
target server receives zero requests, proving replay expansion occurs before
disabled-image validation.
🪄 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: 68c7e9ef-89e8-471b-8b9e-f9f85ef5b0b8
📒 Files selected for processing (30)
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/combos.test.tstests/server-combo-failover-e2e.test.ts
| test("imageInput disabled strips image even when every member supports it", () => { | ||
| const visionMembers = [ | ||
| { provider: "a", id: "m1", contextWindow: 128_000, maxInputTokens: 100_000, inputModalities: ["text", "image"], reasoningEfforts: ["low"] }, | ||
| { provider: "b", id: "m2", contextWindow: 128_000, maxInputTokens: 100_000, inputModalities: ["text", "image"], reasoningEfforts: ["low"] }, | ||
| ]; | ||
| expect(deriveComboCatalogModel("text-only", normalizedCombo({ imageInput: "disabled" }), visionMembers)) | ||
| .toEqual(expect.objectContaining({ inputModalities: ["text"] })); | ||
| expect(deriveComboCatalogModel("vision", normalizedCombo({ imageInput: "auto" }), visionMembers)) | ||
| .toEqual(expect.objectContaining({ inputModalities: expect.arrayContaining(["text", "image"]) })); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an image-only disabled-mode regression.
These assertions cover members that share text and image. They do not cover the new inputModalities.length === 0 branch in src/codex/catalog/aggregation.ts after disabled mode removes an image-only intersection.
Add a case with all members exposing only ["image"]. Assert that deriveComboCatalogModel() returns null when imageInput is "disabled".
As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
🤖 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 `@tests/codex-catalog.test.ts` around lines 159 - 168, Add a focused case to
the existing imageInput disabled-mode test using members whose inputModalities
are only ["image"], and assert deriveComboCatalogModel returns null with
imageInput set to "disabled". Keep the current shared text/image assertions
unchanged.
Source: Path instructions
| test("disabled image input expands text-only previous_response_id and dispatches once", async () => { | ||
| const { rememberResponseState } = await import("../src/responses/state"); | ||
| rememberResponseState( | ||
| { model: "combo/free", input: [{ role: "user", content: "earlier text" }] }, | ||
| { | ||
| id: "resp_combo_text_prev", | ||
| status: "completed", | ||
| output: [{ type: "message", role: "assistant", content: "ack" }], | ||
| }, | ||
| ); | ||
| const bodies: Array<Record<string, unknown>> = []; | ||
| const a = serve(async request => { | ||
| bodies.push(await request.json() as Record<string, unknown>); | ||
| return chatSuccess("continued", "m1"); | ||
| }); | ||
| const config = comboConfig({ a: provider("openai-chat", baseUrl(a), "key-a") }, undefined, { | ||
| imageInput: "disabled", | ||
| }); | ||
| const response = await post(config, { | ||
| previous_response_id: "resp_combo_text_prev", | ||
| input: [{ role: "user", content: "next turn" }], | ||
| }); | ||
| expect(response.status).toBe(200); | ||
| expect(bodies).toHaveLength(1); | ||
| const child = bodies[0]!; | ||
| // Parent already expanded; child must not keep previous_response_id (would double-prepend). | ||
| expect(child.previous_response_id).toBeUndefined(); | ||
| const inputText = JSON.stringify(child.input ?? child.messages ?? child); | ||
| expect(inputText).toContain("earlier text"); | ||
| expect(inputText).toContain("next turn"); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a stored-image continuation regression.
Lines 1374-1404 prove that text-only replay dispatches once. They do not prove that replay occurs before the disabled-image check. Direct-image coverage still passes if a future change validates before replay expansion.
Store a response with materialized input_image content. Continue it through an image-disabled combo. Assert HTTP 400 and zero target requests.
As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
🤖 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 `@tests/server-combo-failover-e2e.test.ts` around lines 1374 - 1404, Add a
focused regression test near the existing disabled-image continuation test,
storing response state whose materialized input contains input_image content,
then continuing it through a combo configured with imageInput disabled. Assert
the request returns HTTP 400 and the target server receives zero requests,
proving replay expansion occurs before disabled-image validation.
Source: Path instructions
Wibias
removed
the
stale
3 participants
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