feat(vision): add chat and Google sidecars - #1161
Conversation
📝 WalkthroughWalkthroughThe PR adds ChangesChat vision support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant VisionPlanner
participant describeImageChat
participant ProviderAPI
VisionPlanner->>describeImageChat: Send image, context, provider, and model
describeImageChat->>ProviderAPI: Request streamed image description
ProviderAPI-->>describeImageChat: Return streamed response events
describeImageChat-->>VisionPlanner: Return description outcome
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: 5
🤖 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/pages/dashboard-shared.ts`:
- Around line 185-189: The unresolved model fallback in sidecarBackendForModel
must return a distinct unresolved result rather than "chat"; update
gui/src/pages/dashboard-shared.ts lines 185-189 accordingly. In
gui/src/pages/dashboard-overview-sections.tsx lines 330-337, retain the current
model only when it resolves to an enabled model whose backend matches the
selected backend; otherwise select a discovered namespaced model consistent with
the management API response.
In `@src/cli/agent.ts`:
- Line 23: Update the sidecar usage text in the agent CLI help to document
backend choices by status: show openai|anthropic for web and
openai|anthropic|chat for vision, while preserving the existing model and status
syntax.
In `@src/vision/describe-chat.ts`:
- Around line 34-40: Require HTTPS through one shared guard before either OAuth
flow obtains or sends a bearer token: update the Google OAuth path in
src/vision/describe-chat.ts:34-40 and the generic chat OAuth path in
src/vision/describe-chat.ts:127-130 to return a non-sensitive error for
non-HTTPS URLs before token retrieval or Authorization construction. Add
coverage in tests/vision-chat.test.ts:31-81 for Google and OpenAI-compatible
OAuth, asserting HTTP URLs do not call fetch.
In `@src/vision/index.ts`:
- Around line 180-220: Add focused regression tests near the existing
vision/chat tests for planVisionSidecar: verify a provider-qualified model
selects the named chat/google provider with the expected model, and verify
disabled or unauthenticated providers produce no chat plan. Reuse existing test
fixtures and assertions without changing describeImageChat request-formatting
coverage.
In `@tests/vision-chat.test.ts`:
- Around line 31-81: Add regression coverage in the existing chat vision tests
for OAuth-configured Google and OpenAI-compatible providers using an http:
baseUrl. For each case, assert describeImageChat returns an error and verify the
fetch mock was not called, while preserving the existing successful HTTPS
adapter tests.
🪄 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: 40d779d5-757f-4c69-ab7a-b731bf456a7f
📒 Files selected for processing (19)
gui/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/claude-code-sections.tsxgui/src/pages/claude-manual-env.tsgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/src/pages/use-dashboard-data.tssrc/cli/agent.tssrc/cli/integrations.tssrc/server/management/agent-settings-routes.tssrc/server/management/config-routes.tssrc/types.tssrc/vision/describe-chat.tssrc/vision/index.tstests/vision-chat.test.ts
| export function sidecarBackendForModel(models: ModelInfo[], modelId: string): SidecarBackend { | ||
| return models.find(model => model.id === modelId)?.provider === "anthropic" ? "anthropic" : "openai"; | ||
| const model = models.find(item => item.id === modelId || item.namespaced === modelId); | ||
| if (model?.provider === "anthropic") return "anthropic"; | ||
| if (model?.provider === "openai") return "openai"; | ||
| return "chat"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not classify an unresolved model ID as the chat backend.
A missing configured vision model is retained in gui/src/pages/use-dashboard-data.ts. The current helper maps that unresolved ID to chat. Selecting Chat API then preserves the stale ID instead of selecting a discovered, namespaced Chat API model. The backend cannot resolve that stale model to a configured provider.
gui/src/pages/dashboard-shared.ts#L185-L189: return a distinct unresolved result instead of usingchatas the fallback.gui/src/pages/dashboard-overview-sections.tsx#L330-L337: retain the current model only when it resolves to an enabled model with the selected backend.
As per path instructions, GUI state changes must stay consistent with management API responses.
📍 Affects 2 files
gui/src/pages/dashboard-shared.ts#L185-L189(this comment)gui/src/pages/dashboard-overview-sections.tsx#L330-L337
🤖 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/dashboard-shared.ts` around lines 185 - 189, The unresolved
model fallback in sidecarBackendForModel must return a distinct unresolved
result rather than "chat"; update gui/src/pages/dashboard-shared.ts lines
185-189 accordingly. In gui/src/pages/dashboard-overview-sections.tsx lines
330-337, retain the current model only when it resolves to an enabled model
whose backend matches the selected backend; otherwise select a discovered
namespaced model consistent with the management API response.
Source: Path instructions
| ocx agent subagents <status|set|clear> [model,model...] [--json] | ||
| ocx agent fallback <status|set|clear> [model,model...] [--poll-ms <5000-600000>] [--json] | ||
| ocx agent sidecar <status|web|vision> [--model <id|->] [--backend <openai|anthropic|->] | ||
| ocx agent sidecar <status|web|vision> [--model <id|->] [--backend <openai|anthropic|chat|->] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Separate the web and vision backend choices in the usage text.
Line 23 advertises chat for ocx agent sidecar web. The management API rejects webSearch.backend: "chat". Document openai|anthropic for web and openai|anthropic|chat for vision.
Proposed fix
- ocx agent sidecar <status|web|vision> [--model <id|->] [--backend <openai|anthropic|chat|->]
+ ocx agent sidecar web [--model <id|->] [--backend <openai|anthropic|->]
+ ocx agent sidecar vision [--model <id|->] [--backend <openai|anthropic|chat|->]📝 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.
| ocx agent sidecar <status|web|vision> [--model <id|->] [--backend <openai|anthropic|chat|->] | |
| ocx agent sidecar web [--model <id|->] [--backend <openai|anthropic|->] | |
| ocx agent sidecar vision [--model <id|->] [--backend <openai|anthropic|chat|->] |
🤖 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/cli/agent.ts` at line 23, Update the sidecar usage text in the agent CLI
help to document backend choices by status: show openai|anthropic for web and
openai|anthropic|chat for vision, while preserving the existing model and status
syntax.
| if (provider.authMode === "oauth") { | ||
| try { | ||
| const token = await getValidAccessToken(providerName); | ||
| const project = provider.googleMode === "cloud-code-assist" | ||
| ? getOAuthCredentialProjectId(providerName) | ||
| : provider.project; | ||
| requestProvider = { ...provider, apiKey: token, ...(project ? { project } : {}) }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require HTTPS before obtaining or sending an OAuth bearer token.
The provider URL contract permits http: for operator-controlled providers. Both OAuth paths can send bearer credentials to that URL. Add one shared HTTPS guard before the Google dispatch at Line 114. Return a non-sensitive error when the URL is not HTTPS.
src/vision/describe-chat.ts#L34-L40: route Google OAuth through the shared HTTPS guard before token retrieval.src/vision/describe-chat.ts#L127-L130: route generic chat OAuth through the shared HTTPS guard before buildingAuthorization.tests/vision-chat.test.ts#L31-L81: add Google and OpenAI-compatible OAuth tests that assert an HTTP URL does not callfetch.
Based on learnings: OAuth adapters that attach Bearer credentials must enforce HTTPS when cleartext transmission is unacceptable.
📍 Affects 2 files
src/vision/describe-chat.ts#L34-L40(this comment)src/vision/describe-chat.ts#L127-L130tests/vision-chat.test.ts#L31-L81
🤖 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/vision/describe-chat.ts` around lines 34 - 40, Require HTTPS through one
shared guard before either OAuth flow obtains or sends a bearer token: update
the Google OAuth path in src/vision/describe-chat.ts:34-40 and the generic chat
OAuth path in src/vision/describe-chat.ts:127-130 to return a non-sensitive
error for non-HTTPS URLs before token retrieval or Authorization construction.
Add coverage in tests/vision-chat.test.ts:31-81 for Google and OpenAI-compatible
OAuth, asserting HTTP URLs do not call fetch.
Sources: Path instructions, Learnings
| /** Find the chat-adapter or google-adapter provider for `model` with usable auth. */ | ||
| function findChatVisionProvider(config: OcxConfig, model: string): { provider: OcxProviderConfig; providerName: string; model: string } | undefined { | ||
| let providerName = ""; | ||
| let bareModel = model; | ||
| if (model.includes("/")) { | ||
| const sep = model.indexOf("/"); | ||
| providerName = model.slice(0, sep); | ||
| bareModel = model.slice(sep + 1); | ||
| } | ||
| const isChatLike = (p: OcxProviderConfig) => p.adapter === "openai-chat" || p.adapter === "google"; | ||
| const hasAuth = (p: OcxProviderConfig) => { | ||
| if (p.apiKey ?? p.apiKeyPool?.[0]?.key) return true; | ||
| if (p.authMode === "oauth") return true; | ||
| return false; | ||
| }; | ||
| if (providerName) { | ||
| const provider = config.providers[providerName]; | ||
| if (provider && provider.disabled !== true && isChatLike(provider) && hasAuth(provider)) { | ||
| return { provider, providerName, model: bareModel }; | ||
| } | ||
| return undefined; | ||
| } | ||
| for (const [name, provider] of Object.entries(config.providers)) { | ||
| if (provider.disabled === true) continue; | ||
| if (!isChatLike(provider)) continue; | ||
| if (!hasAuth(provider)) continue; | ||
| const models = provider.models ?? []; | ||
| const defaultModel = provider.defaultModel; | ||
| if (bareModel === defaultModel || models.includes(bareModel) || models.some(m => m.endsWith("/" + bareModel) || m === bareModel)) { | ||
| return { provider, providerName: name, model: bareModel }; | ||
| } | ||
| } | ||
| for (const [name, provider] of Object.entries(config.providers)) { | ||
| if (provider.disabled === true) continue; | ||
| if (!isChatLike(provider)) continue; | ||
| if (!hasAuth(provider)) continue; | ||
| if (provider.liveModels === true) { | ||
| return { provider, providerName: name, model: bareModel }; | ||
| } | ||
| } | ||
| return undefined; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add plan-selection regression tests for the chat backend.
The added tests cover describeImageChat request formatting only. Add tests for planVisionSidecar that verify provider-qualified models resolve to the selected provider and that disabled or unauthenticated providers produce no chat plan.
As per path instructions, “A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem.”
Also applies to: 288-297
🤖 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/vision/index.ts` around lines 180 - 220, Add focused regression tests
near the existing vision/chat tests for planVisionSidecar: verify a
provider-qualified model selects the named chat/google provider with the
expected model, and verify disabled or unauthenticated providers produce no chat
plan. Reuse existing test fixtures and assertions without changing
describeImageChat request-formatting coverage.
Source: Path instructions
| describe("chat vision sidecar", () => { | ||
| test("sends an image_url through an OpenAI-compatible provider", async () => { | ||
| let capturedUrl = ""; | ||
| let capturedBody: Record<string, any> | undefined; | ||
| globalThis.fetch = (async (url, init) => { | ||
| capturedUrl = String(url); | ||
| capturedBody = JSON.parse(String(init?.body)); | ||
| return chatSse("Mimo description"); | ||
| }) as typeof fetch; | ||
| const provider: OcxProviderConfig = { | ||
| adapter: "openai-chat", | ||
| baseUrl: "https://vision.example/v1", | ||
| authMode: "key", | ||
| apiKey: "test-key", | ||
| }; | ||
|
|
||
| const result = await describeImageChat(image, "high", "describe this", provider, "mimo", settings); | ||
|
|
||
| expect(result).toEqual({ text: "Mimo description" }); | ||
| expect(capturedUrl).toBe("https://vision.example/v1/chat/completions"); | ||
| expect(capturedBody?.model).toBe("vision-test"); | ||
| expect(capturedBody?.messages[0].content).toEqual([ | ||
| { type: "text", text: "describe this" }, | ||
| { type: "image_url", image_url: { url: image, detail: "high" } }, | ||
| ]); | ||
| }); | ||
|
|
||
| test("uses the native Google adapter wire format", async () => { | ||
| let capturedUrl = ""; | ||
| let capturedBody: Record<string, any> | undefined; | ||
| globalThis.fetch = (async (url, init) => { | ||
| capturedUrl = String(url); | ||
| capturedBody = JSON.parse(String(init?.body)); | ||
| return geminiSse("Gemini description"); | ||
| }) as typeof fetch; | ||
| const provider: OcxProviderConfig = { | ||
| adapter: "google", | ||
| baseUrl: "https://generativelanguage.googleapis.com", | ||
| authMode: "key", | ||
| apiKey: "test-key", | ||
| }; | ||
|
|
||
| const result = await describeImageChat(image, "high", "describe this", provider, "gemini", { model: "gemini-test", timeoutMs: 5000 }); | ||
|
|
||
| expect(result).toEqual({ text: "Gemini description" }); | ||
| expect(capturedUrl).toBe("https://generativelanguage.googleapis.com/v1beta/models/gemini-test:streamGenerateContent?alt=sse"); | ||
| expect(capturedBody?.contents?.[0]?.parts).toEqual([ | ||
| { text: "describe this" }, | ||
| { inline_data: { mime_type: "image/png", data: "aGVsbG8=" } }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Add a regression test for OAuth transport enforcement.
After adding the HTTPS guard, test both Google and OpenAI-compatible OAuth providers with an http: base URL. Assert that describeImageChat returns an error and does not call fetch.
🤖 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/vision-chat.test.ts` around lines 31 - 81, Add regression coverage in
the existing chat vision tests for OAuth-configured Google and OpenAI-compatible
providers using an http: baseUrl. For each case, assert describeImageChat
returns an error and verify the fetch mock was not called, while preserving the
existing successful HTTPS adapter tests.
Sources: Path instructions, Learnings
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
|
✅ Deterministic PR hygiene checks passed. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97d0c7dbf9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const project = provider.googleMode === "cloud-code-assist" | ||
| ? getOAuthCredentialProjectId(providerName) | ||
| : provider.project; | ||
| requestProvider = { ...provider, apiKey: token, ...(project ? { project } : {}) }; |
There was a problem hiding this comment.
When the new Google vision sidecar is used with an OAuth provider such as google-antigravity, this line attaches the access token to the raw configured provider. Unlike the existing main routing/image fallback paths, no registry transport pinning happens first, so a stale or tampered baseUrl in config causes createGoogleAdapter to send the bearer token to that host. Resolve or pin the provider transport before adding apiKey.
AGENTS.md reference: AGENTS.md:L218-L223
Useful? React with 👍 / 👎.
| method: request.method, | ||
| headers: request.headers, | ||
| body: request.body, | ||
| signal: abortSignal, |
There was a problem hiding this comment.
For Google AI Studio, adapter.fetchResponse is absent, so this fallback fetch uses only the caller's abort signal and never applies settings.timeoutMs; the subsequent parseStream also runs without a timeout. If the sidecar opens a stream and then stalls, the main text-only request waits indefinitely instead of degrading after the configured vision timeout. Use the same linked timeout signal/body-cancel pattern as the other vision sidecars.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| const compatible = target === null | ||
| ? visionSidecarModels.find(option => sidecarBackendForModel(models, option.value) === "openai") |
There was a problem hiding this comment.
When users switch the vision backend to Auto on a setup with an Anthropic OAuth sidecar, this branch clears backend but persists the first OpenAI model. Runtime auto resolution still prefers Anthropic when available, so the next image request asks the Anthropic sidecar to run an OpenAI model id and the description fails. Auto should preserve a backend-compatible model or clear model so the runtime can use its backend default.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| return { text: "", error: `oauth token failed: ${e instanceof Error ? e.message : String(e)}` }; | ||
| } | ||
| } else { | ||
| const apiKey = provider.apiKey ?? provider.apiKeyPool?.[0]?.key ?? provider.headers?.Authorization?.replace(/^Bearer\s+/i, ""); |
There was a problem hiding this comment.
When a chat vision sidecar uses a key provider configured with an env reference such as apiKey = "$MIMO_API_KEY", this reads the raw config value and builds Authorization: Bearer $MIMO_API_KEY, while the normal routing path resolves env-backed keys first. That makes env-based provider configs fail only for the new sidecar; resolve the value before composing the header.
AGENTS.md reference: src/AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
| const hasAuth = (p: OcxProviderConfig) => { | ||
| if (p.apiKey ?? p.apiKeyPool?.[0]?.key) return true; | ||
| if (p.authMode === "oauth") return true; | ||
| return false; |
There was a problem hiding this comment.
With the new vision model picker, users can select OpenAI-compatible local vision models such as ollama/llava, but this auth check rejects every keyless authMode: "local" or keyOptional provider before a plan is built. Those requests strip the image as unavailable instead of calling the local endpoint; treat keyless providers as usable and omit Authorization in the executor.
Useful? React with 👍 / 👎.
| if (providerName) { | ||
| const provider = config.providers[providerName]; | ||
| if (provider && provider.disabled !== true && isChatLike(provider) && hasAuth(provider)) { | ||
| return { provider, providerName, model: bareModel }; |
There was a problem hiding this comment.
When users choose a chat sidecar model whose native id contains / (for example OpenRouter models), the dashboard persists the Codex-facing slug provider/<encoded-id>. This branch strips the provider and returns the encoded slug without decoding it against provider.models, so the sidecar sends anthropic-claude... upstream instead of anthropic/claude... and the request fails. Decode the routed slug before storing it in the plan.
Useful? React with 👍 / 👎.
| if (model.provider === "openai" || model.provider === "anthropic") { | ||
| out.push({ value: model.id, label: `${model.provider}/${model.id}` }); | ||
| } else { | ||
| out.push({ value: model.namespaced, label: model.namespaced }); |
There was a problem hiding this comment.
This branch adds every non-OpenAI/Anthropic row to the vision picker and sidecarBackendForModel classifies it as chat, but the runtime chat sidecar only accepts providers whose adapter is openai-chat or google. Selecting rows such as anthropic-apikey/..., combos, or other adapters saves a model the backend can never plan, so text-only requests strip the image as unavailable. Source this list from runtime-eligible providers or include adapter capability data.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
|
|
||
| const content: unknown[] = [ | ||
| { type: "text", text: contextText || "Describe this image." }, | ||
| toChatImagePart(imageUrl, detail), |
There was a problem hiding this comment.
The new chat backend forwards imageUrl directly as an image_url part without the validation used by the existing vision sidecars. If a request supplies an unsupported scheme or an oversized data URL, the sidecar now passes it to the configured OpenAI-compatible provider instead of producing the bounded local error marker, which can break local or strict providers and bypass the existing image-size guard. Validate before building this part.
Useful? React with 👍 / 👎.
1 participant
Problem
Text-only routed models such as DeepSeek need image descriptions before they can reason about an attached image. The existing vision sidecar only supported OpenAI and Anthropic backends, so users could not use an OpenAI-compatible provider such as Mimo or the existing Google Antigravity provider.
Changes
chatvision sidecar backend for OpenAI-compatible providers.image_urlcontent parts, matching the Hermes auxiliary vision flow.Compatibility
Verification
bun x tsc --noEmitbun test ./tests/vision-chat.test.ts ./tests/vision-sidecar-e2e.test.ts ./tests/vision-anthropic.test.ts ./tests/vision-fail-closed.test.ts ./tests/vision-cache.test.ts— 31 passednpm run buildingui— passedbun test ./gui/tests/dashboard-tabs.test.ts ./gui/tests/dashboard-contracts.test.ts ./gui/tests/dashboard-sync-feedback.test.tsx— 28 passedThe live local verification also exercised DeepSeek image requests through Mimo and Gemini Antigravity.
Summary by CodeRabbit
chatsupport to Claude Code configuration and command-line options.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.