feat(vision): make sidecar reasoning configurable - #1002
Conversation
📝 WalkthroughWalkthroughThis PR adds a configurable ChangesVision Reasoning Effort Implementation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard as Dashboard GUI
participant ConfigAPI as Management API
participant VisionIndex as vision/index.ts
participant Describe as vision/describe.ts
participant Backend as OpenAI/Anthropic Backend
Dashboard->>ConfigAPI: PUT visionSidecar.reasoning
ConfigAPI->>ConfigAPI: validate and sanitize reasoning
ConfigAPI-->>Dashboard: 200 with persisted reasoning
Note over VisionIndex: later, on image description request
VisionIndex->>VisionIndex: build plan with configured reasoning
VisionIndex->>VisionIndex: compute cache key: backend + model + reasoning + details
VisionIndex->>Describe: describeImage(settings with reasoning)
Describe->>Backend: send request with reasoning effort
Backend-->>Describe: description result
Describe-->>VisionIndex: description or error
VisionIndex-->>Dashboard: return cached or fresh description
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: a1122fe5cd
ℹ️ 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".
| settings: { model: resolveOpenAiVisionModel(config), timeoutMs: cfg.timeoutMs ?? DEFAULT_TIMEOUT_MS }, | ||
| settings: { | ||
| model: resolveOpenAiVisionModel(config), | ||
| reasoning: cfg.reasoning ?? DEFAULT_REASONING, |
There was a problem hiding this comment.
When visionSidecar.reasoning is written through ocx config set/import or by editing config.json, configSchema passes the sidecar object through and validateConfigCandidate() therefore accepts arbitrary values such as "ultra"; this line then forwards that value as reasoning.effort, causing every OpenAI vision description to be rejected upstream even though the management endpoint rejects the same input. Add this enum to the persisted-config validation/normalization path, or normalize it before constructing VisionSettings.
Useful? React with 👍 / 👎.
| key: JSON.stringify([ | ||
| plan.backend, | ||
| plan.settings.model, | ||
| plan.settings.reasoning, |
There was a problem hiding this comment.
When the selected vision backend is Anthropic and the operator changes this dashboard-exposed reasoning setting, the Anthropic request remains identical (anthropic-describe.ts always sends thinking: { type: "disabled" } and never reads settings.reasoning), but this key changes. Consequently the same image and context bypass an otherwise valid cached description and trigger another potentially billed OAuth sidecar call; include reasoning in the identity only for the OpenAI backend, or key on the effective Anthropic settings.
Useful? React with 👍 / 👎.
| /> | ||
| <Select | ||
| value={sidecar?.vision.reasoning ?? "low"} | ||
| options={VISION_REASONING_LEVELS.map(value => ({ value, label: value }))} |
There was a problem hiding this comment.
This picker offers max for every OpenAI vision model, but the repository's native catalog deliberately gives only GPT-5.6 models the max rung and limits native gpt-5.5, gpt-5.4, and gpt-5.4-mini to low through xhigh (src/codex/catalog/sync.ts). Selecting max with one of those models therefore persists a value that passes the management API's universal enum check and is sent raw as reasoning.effort, causing the vision request to fail; derive the choices from the selected model's reasoningEfforts metadata or validate/clamp the pair server-side.
AGENTS.md reference: gui/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/sidecars.md`:
- Around line 79-81: Rewrite the sidecar image-processing descriptions to
distinguish provider-specific transports: in
docs-site/src/content/docs/guides/sidecars.md lines 79-81, explain ChatGPT-login
passthrough uses the OpenAI Responses endpoint and that Anthropic uses the
Messages path with its thinking-budget mapping; apply the equivalent provider
split in Japanese at docs-site/src/content/docs/ja/guides/sidecars.md lines
78-80, Korean at docs-site/src/content/docs/ko/guides/sidecars.md lines 79-81,
Russian at docs-site/src/content/docs/ru/guides/sidecars.md lines 90-92, and
Chinese at docs-site/src/content/docs/zh-cn/guides/sidecars.md lines 72-73,
preserving each language’s accuracy and alignment with the provider contracts.
In `@docs-site/src/content/docs/ja/reference/configuration/server.md`:
- Line 147: Update the reasoning configuration references to add one concise
upstream-support caveat in each affected table or adjacent paragraph:
docs-site/src/content/docs/ja/reference/configuration/server.md:147-147 in
Japanese,
docs-site/src/content/docs/ko/reference/configuration/server.md:147-147 in
Korean, docs-site/src/content/docs/reference/configuration/server.md:179-179 in
English, docs-site/src/content/docs/ru/reference/configuration/server.md:182-182
in Russian, and
docs-site/src/content/docs/zh-cn/reference/configuration/server.md:162-162 in
Chinese. State that supported reasoning levels are gated by upstream provider
capabilities and the provider’s available reasoning ladder.
In `@gui/src/i18n/ru.ts`:
- Line 217: Update the dash.visionReasoning translation to replace the English
“vision” with the file’s established Russian wording “для изображений,” matching
the phrasing pattern used by dash.searchReasoning and related vision keys.
In `@src/server/management/config-routes.ts`:
- Around line 347-352: Extract the shared vision reasoning allowlist into a
single constant in the config-routes module, then reuse it in both the
validation branch around the vision.reasoning check and the persistence branch
around the second repeated check. Replace both duplicated OR chains with
membership checks against that constant so both paths always accept the same
five values.
🪄 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: 006a539f-e1bf-41f8-987e-3ffa15d8d192
📒 Files selected for processing (27)
docs-site/src/content/docs/guides/sidecars.mddocs-site/src/content/docs/ja/guides/sidecars.mddocs-site/src/content/docs/ja/reference/configuration/server.mddocs-site/src/content/docs/ko/guides/sidecars.mddocs-site/src/content/docs/ko/reference/configuration/server.mddocs-site/src/content/docs/reference/configuration/server.mddocs-site/src/content/docs/ru/guides/sidecars.mddocs-site/src/content/docs/ru/reference/configuration/server.mddocs-site/src/content/docs/zh-cn/guides/sidecars.mddocs-site/src/content/docs/zh-cn/reference/configuration/server.mdgui/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/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/tests/dashboard-contracts.test.tssrc/server/management/config-routes.tssrc/types.tssrc/vision/describe.tssrc/vision/index.tstests/claude-sidecar-override.test.tstests/vision-anthropic.test.tstests/vision-cache.test.tstests/vision-sidecar-e2e.test.ts
|
Review feedback addressed in 47e2540.
Validation: full server suite 8217 passed / 0 failed, GUI suite 585 passed / 0 failed, typecheck, GUI lint+build, docs build (216 pages), and privacy scan all green. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs-site/src/content/docs/guides/sidecars.md (1)
95-96: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the cache-key documentation across all locales.
The runtime includes reasoning in the description cache identity only for the OpenAI backend. Anthropic intentionally excludes the OpenAI-specific setting. Update each affected sentence to state this backend-specific behavior.
docs-site/src/content/docs/guides/sidecars.md#L95-L96: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ja/guides/sidecars.md#L93-L94: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ko/guides/sidecars.md#L93-L96: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ru/guides/sidecars.md#L107-L108: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/zh-cn/guides/sidecars.md#L85-L87: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/reference/configuration/server.md#L186-L188: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ja/reference/configuration/server.md#L151-L151: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ko/reference/configuration/server.md#L151-L151: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/ru/reference/configuration/server.md#L189-L192: qualify reasoning as an OpenAI-only cache-key component.docs-site/src/content/docs/zh-cn/reference/configuration/server.md#L166-L166: qualify reasoning as an OpenAI-only cache-key component.As per path instructions, keep translated documentation synchronized with actual runtime 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/sidecars.md` around lines 95 - 96, Update the cache-key sentences to state that reasoning is included only for the OpenAI backend, while preserving the existing exclusion for Anthropic. Apply this wording correction in docs-site/src/content/docs/guides/sidecars.md:95-96, docs-site/src/content/docs/ja/guides/sidecars.md:93-94, docs-site/src/content/docs/ko/guides/sidecars.md:93-96, docs-site/src/content/docs/ru/guides/sidecars.md:107-108, docs-site/src/content/docs/zh-cn/guides/sidecars.md:85-87, docs-site/src/content/docs/reference/configuration/server.md:186-188, docs-site/src/content/docs/ja/reference/configuration/server.md:151-151, docs-site/src/content/docs/ko/reference/configuration/server.md:151-151, docs-site/src/content/docs/ru/reference/configuration/server.md:189-192, and docs-site/src/content/docs/zh-cn/reference/configuration/server.md:166-166, keeping each translation synchronized with the runtime behavior.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 `@docs-site/src/content/docs/guides/sidecars.md`:
- Around line 79-82: Add a caveat after each provider-specific transport
description explaining that the reasoning ladder support is constrained by the
selected provider and model, and that the Dashboard clamps persisted values when
a model does not advertise the selected reasoning level. This caveat must be
added at the following locations with equivalent translations or localized
versions: English version in docs-site/src/content/docs/guides/sidecars.md at
lines 79-82 (anchor), Japanese version in
docs-site/src/content/docs/ja/guides/sidecars.md at lines 78-81, Korean version
in docs-site/src/content/docs/ko/guides/sidecars.md at lines 79-82, Russian
version in docs-site/src/content/docs/ru/guides/sidecars.md at lines 90-93, and
Simplified Chinese version in
docs-site/src/content/docs/zh-cn/guides/sidecars.md at lines 72-74. Ensure the
caveat does not imply that advertised reasoning levels are universally supported
across all models or providers.
In `@src/config.ts`:
- Around line 1657-1662: Preserve an untouched copy of parsed before
sanitizeVisionSidecarForLoad mutates it, then pass that original data to both
warnDegradedVisionReasoning calls while continuing to use the sanitized parsed
object for schema parsing and other behavior. Update the relevant load paths
around sanitizeVisionSidecarForLoad and both warning-call sites without changing
the sanitizer’s existing validation behavior.
---
Outside diff comments:
In `@docs-site/src/content/docs/guides/sidecars.md`:
- Around line 95-96: Update the cache-key sentences to state that reasoning is
included only for the OpenAI backend, while preserving the existing exclusion
for Anthropic. Apply this wording correction in
docs-site/src/content/docs/guides/sidecars.md:95-96,
docs-site/src/content/docs/ja/guides/sidecars.md:93-94,
docs-site/src/content/docs/ko/guides/sidecars.md:93-96,
docs-site/src/content/docs/ru/guides/sidecars.md:107-108,
docs-site/src/content/docs/zh-cn/guides/sidecars.md:85-87,
docs-site/src/content/docs/reference/configuration/server.md:186-188,
docs-site/src/content/docs/ja/reference/configuration/server.md:151-151,
docs-site/src/content/docs/ko/reference/configuration/server.md:151-151,
docs-site/src/content/docs/ru/reference/configuration/server.md:189-192, and
docs-site/src/content/docs/zh-cn/reference/configuration/server.md:166-166,
keeping each translation synchronized with the runtime behavior.
🪄 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: 5a0ab5cb-72fb-40da-9e5d-9946acf3960d
📒 Files selected for processing (23)
docs-site/src/content/docs/guides/sidecars.mddocs-site/src/content/docs/ja/guides/sidecars.mddocs-site/src/content/docs/ja/reference/configuration/server.mddocs-site/src/content/docs/ko/guides/sidecars.mddocs-site/src/content/docs/ko/reference/configuration/server.mddocs-site/src/content/docs/reference/configuration/server.mddocs-site/src/content/docs/ru/guides/sidecars.mddocs-site/src/content/docs/ru/reference/configuration/server.mddocs-site/src/content/docs/zh-cn/guides/sidecars.mddocs-site/src/content/docs/zh-cn/reference/configuration/server.mdgui/src/i18n/ru.tsgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/tests/dashboard-contracts.test.tssrc/config.tssrc/reasoning-effort.tssrc/server/management/config-routes.tssrc/vision/describe.tssrc/vision/index.tstests/config.test.tstests/settings-stream-mode.test.tstests/vision-anthropic.test.tstests/vision-cache.test.ts
| - On the OpenAI path (ChatGPT-login passthrough), each image is sent to the configured vision model | ||
| over the Responses endpoint with the selected `reasoning.effort` (`low` by default), and its | ||
| description replaces the image part inline. The Anthropic path uses the Messages endpoint with its | ||
| own thinking-budget mapping and ignores this OpenAI-specific setting. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add model-dependent support and clamping caveats to every guide.
The shared reasoning ladder is constrained by the selected provider and model. The Dashboard also clamps persisted values when a model does not advertise the selected level. Add this caveat next to the provider-specific transport description in every guide.
docs-site/src/content/docs/guides/sidecars.md#L79-L82: add the provider/model support and Dashboard clamping caveat.docs-site/src/content/docs/ja/guides/sidecars.md#L78-L81: add the equivalent Japanese caveat.docs-site/src/content/docs/ko/guides/sidecars.md#L79-L82: add the equivalent Korean caveat.docs-site/src/content/docs/ru/guides/sidecars.md#L90-L93: add the equivalent Russian caveat.docs-site/src/content/docs/zh-cn/guides/sidecars.md#L72-L74: add the equivalent Chinese caveat.
As per path instructions, do not imply that advertised reasoning levels are universally supported.
📍 Affects 5 files
docs-site/src/content/docs/guides/sidecars.md#L79-L82(this comment)docs-site/src/content/docs/ja/guides/sidecars.md#L78-L81docs-site/src/content/docs/ko/guides/sidecars.md#L79-L82docs-site/src/content/docs/ru/guides/sidecars.md#L90-L93docs-site/src/content/docs/zh-cn/guides/sidecars.md#L72-L74
🤖 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/sidecars.md` around lines 79 - 82, Add a
caveat after each provider-specific transport description explaining that the
reasoning ladder support is constrained by the selected provider and model, and
that the Dashboard clamps persisted values when a model does not advertise the
selected reasoning level. This caveat must be added at the following locations
with equivalent translations or localized versions: English version in
docs-site/src/content/docs/guides/sidecars.md at lines 79-82 (anchor), Japanese
version in docs-site/src/content/docs/ja/guides/sidecars.md at lines 78-81,
Korean version in docs-site/src/content/docs/ko/guides/sidecars.md at lines
79-82, Russian version in docs-site/src/content/docs/ru/guides/sidecars.md at
lines 90-93, and Simplified Chinese version in
docs-site/src/content/docs/zh-cn/guides/sidecars.md at lines 72-74. Ensure the
caveat does not imply that advertised reasoning levels are universally supported
across all models or providers.
Source: Path instructions
| sanitizeVisionSidecarForLoad(parsed); | ||
| const result = configSchema.safeParse(parsed); | ||
| if (result.success) { | ||
| const config = normalizeApiKeyIds(result.data as OcxConfig); | ||
| warnDegradedStreamMode(parsed, config); | ||
| warnDegradedVisionReasoning(parsed, config); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the raw reasoning value before sanitization.
sanitizeVisionSidecarForLoad deletes the invalid value in place at Line 1340. The later warnDegradedVisionReasoning(parsed, config) call receives the same object at Lines 1662 and 1683. Its raw value is already undefined, so invalid persisted reasoning never produces the intended warning.
Copy the parsed object before sanitization, or return the removed value from the sanitizer, and pass the untouched data to both warning calls.
Proposed fix
const parsed = JSON.parse(raw);
+const rawParsed = structuredClone(parsed);
sanitizeRetryOn429ForLoad(parsed);
sanitizeVisionSidecarForLoad(parsed);
...
- warnDegradedVisionReasoning(parsed, config);
+ warnDegradedVisionReasoning(rawParsed, config);
...
- warnDegradedVisionReasoning(parsed, config);
+ warnDegradedVisionReasoning(rawParsed, config);Also applies to: 1327-1353, 1683-1683
🧰 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/config.ts` around lines 1657 - 1662, Preserve an untouched copy of parsed
before sanitizeVisionSidecarForLoad mutates it, then pass that original data to
both warnDegradedVisionReasoning calls while continuing to use the sanitized
parsed object for schema parsing and other behavior. Update the relevant load
paths around sanitizeVisionSidecarForLoad and both warning-call sites without
changing the sanitizer’s existing validation behavior.
1 participant
Summary
low,medium,high,xhigh, andmaxin the Dashboard and management APIlowdefault for backward compatibilityWhy
The vision sidecar previously hard-coded
reasoning.effort: "low", even though the CLI already acceptedocx agent sidecar vision --reasoning. Users of vision models such asgpt-5.6-lunatherefore could not trade latency and cost for more detailed image analysis, and the CLI setting was silently ignored by the vision management path.Impact
OpenAI-backed vision descriptions now send the configured effort in the Responses request. Existing configurations behave exactly as before because the default remains
low. Anthropic vision retains the setting in shared configuration but ignores the OpenAI-specific wire field. Cache entries are separated by reasoning effort so changing the setting cannot reuse a description produced at a different effort.Validation
bun run typecheckbun run test— 8214 passed, 8 skipped, 0 failedcd gui && bun test tests— 583 passed, 0 failedcd gui && bun run lint:i18n && bun run lint && bun run buildcd docs-site && ASTRO_TELEMETRY_DISABLED=1 bun run build— 216 pages builtbun run privacy:scanSummary by CodeRabbit
New Features
Bug Fixes
Documentation