feat(vision): make sidecar reasoning model-aware - #1238
Merged
Conversation
Contributor
✅ READY
|
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds configurable, model-aware vision reasoning levels. It validates and normalizes settings across the CLI, management API, dashboard, and runtime. OpenAI cache identities include reasoning effort. Documentation and contract tests cover provider and model behavior. ChangesVision reasoning configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dashboard
participant ManagementAPI
participant VisionRuntime
participant OpenAI
Dashboard->>ManagementAPI: Submit model and reasoning
ManagementAPI->>ManagementAPI: Validate and normalize reasoning
ManagementAPI-->>Dashboard: Return effective settings
VisionRuntime->>OpenAI: Send image request with reasoning effort
VisionRuntime->>VisionRuntime: Build cache identity with reasoning effort
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
Collaborator
Author
|
not gui |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/zh-cn/guides/vision-sidecar-reasoning.md`:
- Line 10: Update the product name in the Chinese documentation sentence from
“opencodex” to the capitalized “OpenCodex”, leaving the surrounding wording
unchanged.
In `@gui/src/pages/dashboard-overview-sections.tsx`:
- Around line 300-306: Add a dedicated dash.visionReasoning translation key with
appropriate text to every locale file, then update the Select associated with
visionReasoning in the dashboard delegation controls to use
t("dash.visionReasoning") instead of the web-search label key. Keep the existing
searchReasoning translation for the web-search control.
In `@gui/src/pages/dashboard-shared.ts`:
- Around line 160-166: The dashboard clamp rule must match
normalizeVisionReasoningForModel by selecting the highest ladder rung at or
below the persisted rank, falling back to the lowest rung when none qualifies;
update the doc comment and implementation of clampVisionReasoningToLadder in
gui/src/pages/dashboard-shared.ts (lines 160-166). Add non-prefix ladder
fixtures and assertions to gui/tests/vision-reasoning-contract.test.ts (lines
9-20), including ["low", "high"] from "medium" and ["high", "max"] from "low",
to verify the server-matching results.
In `@gui/tests/vision-reasoning-contract.test.ts`:
- Around line 9-20: Extend the test around visionReasoningLadder and
clampVisionReasoningToLadder with non-contiguous ladders and a persisted value
below the ladder’s minimum. Assert that clamping selects the highest supported
rung at or below the requested value, and define the expected behavior when no
rung qualifies, matching normalizeVisionReasoningForModel rather than simply
returning the ladder’s last element.
- Around line 22-24: Update the PUT request setup in
vision-reasoning-contract.test.ts to provide an Origin accepted by
isAllowedManagementOrigin, or adjust the test host/principal configuration to
satisfy that policy, so handleManagementAPI admits the request and
handleConfigRoutes can return the expected 2xx response.
In `@src/server/management/config-routes.ts`:
- Around line 72-82: Normalize the vision reasoning value before delegating to
the base sidecar PUT handler, rather than mutating and saving after delegation.
Update the wrapper around the base handler so config.visionSidecar.reasoning is
normalized or removed before the handler’s single save, preserving one atomic
persisted write and preventing unsupported model/reasoning pairs from reaching
disk.
- Around line 48-54: Update the wrapper’s body-reading logic before
handleBaseConfigRoutes to use readManagementJsonBody(req.clone()) instead of
req.clone().json(), preserving the shared 4 MiB limit. Catch and rethrow
DecompressedBodyTooLargeError, while delegating other read failures to
handleBaseConfigRoutes(ctx) to retain its invalid-JSON behavior.
- Around line 12-26: Update sidecarResponseWithReasoning to preserve the
original response headers when rebuilding a valid JSON body, rather than relying
on jsonResponse with ctx.req and ctx.config, which introduces CORS headers and
drops upstream non-CORS headers. Keep the existing status and reasoning mutation
behavior, while retaining the headers from the original response; ensure
handleBaseConfigRoutes GET and PUT responses remain consistent.
In `@src/server/management/model-rows.ts`:
- Line 12: Filter the vision-sidecar reasoning options returned by the
model-rows response through isVisionReasoningEffort before exposing them to the
Dashboard, while preserving native GPT-5.6 ladders such as max and excluding
ultra. Add a regression assertion covering gpt-5.6-sol or gpt-5.6-terra.
In `@tests/vision-reasoning-contract.test.ts`:
- Around line 36-64: Update the test around handleManagementAPI to configure the
required management credential alongside the isolated OPENCODEX_HOME, send the
accepted authentication header on the PUT request, and restore the credential in
the finally block. Keep the existing response-status, reasoning normalization,
and persisted-config assertions unchanged.
🪄 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: 83341b8f-39de-4c31-977f-55b5bdfba214
📒 Files selected for processing (16)
docs-site/src/content/docs/guides/vision-sidecar-reasoning.mddocs-site/src/content/docs/ja/guides/vision-sidecar-reasoning.mddocs-site/src/content/docs/ko/guides/vision-sidecar-reasoning.mddocs-site/src/content/docs/ru/guides/vision-sidecar-reasoning.mddocs-site/src/content/docs/zh-cn/guides/vision-sidecar-reasoning.mdgui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/tests/vision-reasoning-contract.test.tssrc/reasoning-effort.tssrc/server/management/config-routes-base.tssrc/server/management/config-routes.tssrc/server/management/model-rows.tssrc/vision/describe.tssrc/vision/index.tssrc/vision/reasoning.tstests/vision-reasoning-contract.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/management/config-routes.ts (1)
344-399: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not expose OpenAI reasoning controls for Anthropic vision sidecars.
The dashboard always renders the reasoning selector and sends its value with the selected backend. The management route accepts
backend: "anthropic"and persists that reasoning value. This creates a setting that is unsupported for Anthropic and can either silently do nothing or leak OpenAI reasoning semantics into the Anthropic path.
src/server/management/config-routes.ts#L344-L399: Derive the effective backend before processing reasoning. Reject or omit a supplied reasoning value when the effective backend is Anthropic. Ensure returned effective settings omit unsupported reasoning.gui/src/pages/dashboard-overview-sections.tsx#L290-L315: Render the reasoning selector only for the canonical OpenAI backend. When switching to Anthropic, omitreasoningfrom the patch.- Add a provider-specific regression that verifies Anthropic patches do not persist or transmit OpenAI reasoning.
As per path instructions, “OpenAI reasoning configuration must not alter Anthropic requests or cache identity.”
🤖 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/management/config-routes.ts` around lines 344 - 399, In src/server/management/config-routes.ts:344-399, derive the effective vision backend before processing reasoning and reject or omit reasoning for Anthropic, ensuring persisted and returned settings exclude unsupported reasoning. In gui/src/pages/dashboard-overview-sections.tsx:290-315, render the reasoning selector only for the canonical OpenAI backend and omit reasoning when switching to Anthropic. Add a provider-specific regression verifying Anthropic patches neither persist nor transmit OpenAI reasoning or affect request/cache identity.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 83-84: Update the clamping descriptions consistently in
docs-site/src/content/docs/guides/sidecars.md (83-84),
docs-site/src/content/docs/ja/guides/sidecars.md (82-83),
docs-site/src/content/docs/ko/guides/sidecars.md (83-84),
docs-site/src/content/docs/ru/guides/sidecars.md (94-96), and
docs-site/src/content/docs/zh-cn/guides/sidecars.md (75-76). State that unknown
or custom models remain permissive when capability metadata is unavailable;
apply clamping only when capabilities are known, and describe the actual
fallback behavior when the native model ladder has gaps, keeping each
localization semantically aligned with CLI/API behavior.
In `@docs-site/src/content/docs/ja/reference/configuration/server.md`:
- Around line 147-151: Update the visionSidecar.model documentation to use
gpt-5.6-luna as the dashboard and management API default, while clearly
identifying gpt-5.4-mini as the legacy migration value or runtime fallback when
the field is absent. Apply this correction in
docs-site/src/content/docs/ja/reference/configuration/server.md (lines 147-151),
docs-site/src/content/docs/ko/reference/configuration/server.md (lines 147-151),
docs-site/src/content/docs/ru/reference/configuration/server.md (lines 182-192),
and docs-site/src/content/docs/zh-cn/reference/configuration/server.md (lines
162-166), preserving each language’s existing documentation structure.
In `@src/server/management/config-routes.ts`:
- Around line 320-322: Apply the default vision model before reasoning
normalization in src/server/management/config-routes.ts lines 320-322: resolve
an absent or empty model to gpt-5.4-mini and return
normalizeVisionReasoningForModel(model, vs.reasoning) ?? "low". In
src/cli/config-command.ts lines 84-90, normalize whenever vision.reasoning
exists and use vision.model || "gpt-5.4-mini" as the model input. Add
regressions covering model-less and empty-model configurations through CLI
persistence and GET /api/sidecar-settings.
---
Outside diff comments:
In `@src/server/management/config-routes.ts`:
- Around line 344-399: In src/server/management/config-routes.ts:344-399, derive
the effective vision backend before processing reasoning and reject or omit
reasoning for Anthropic, ensuring persisted and returned settings exclude
unsupported reasoning. In gui/src/pages/dashboard-overview-sections.tsx:290-315,
render the reasoning selector only for the canonical OpenAI backend and omit
reasoning when switching to Anthropic. Add a provider-specific regression
verifying Anthropic patches neither persist nor transmit OpenAI reasoning or
affect request/cache identity.
🪄 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: f94160fb-dc3e-4772-b6ea-8d2d3d782625
📒 Files selected for processing (16)
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/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/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/tests/vision-reasoning-contract.test.tssrc/cli/config-command.tssrc/server/management/config-routes.tssrc/server/management/model-rows.tstests/vision-reasoning-contract.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/cli/config-command.ts`:
- Around line 86-90: Update the vision handling before the reasoning guard in
the CLI config normalization flow: treat vision.model === "" as a reset by
removing or replacing the empty model with the bounded default gpt-5.4-mini,
then return the config when reasoning is absent. Ensure reasoning normalization
also uses the reset/default model rather than treating an empty string as a
custom model, and add regression coverage in vision-reasoning-contract.test.ts
while preserving alignment with the shared routing/config 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: 5407a98f-2e3e-49fa-bf55-8d4a3db2e13f
📒 Files selected for processing (2)
src/cli/config-command.tstests/vision-reasoning-contract.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/pages/dashboard-overview-sections.tsx`:
- Line 303: Update the vision reasoning options mapping in the dashboard
overview section so each option keeps its raw protocol value in value but uses
an i18n translation key for label. Add locale entries for every supported vision
reasoning level, including values such as xhigh and max, and resolve them
through the existing localization mechanism instead of displaying raw strings.
In `@gui/tests/vision-reasoning-contract.test.ts`:
- Around line 31-33: Extend the test for visionReasoningLadder in “unknown
vision model metadata stays permissive” with known-model fixtures whose
reasoningEfforts is omitted, empty, or contains no supported values, and assert
each returns VISION_REASONING_LEVELS. Reuse the existing model metadata fixture
structure and cover the fallback branches in visionReasoningLadder without
changing the unknown-model 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: 7efce70f-353e-4d6b-adda-152209363291
📒 Files selected for processing (3)
gui/src/pages/dashboard-overview-sections.tsxgui/src/pages/dashboard-shared.tsgui/tests/vision-reasoning-contract.test.ts
Collaborator
Author
|
Maintainer review note on CodeRabbit’s outside-diff Anthropic reasoning finding: not applying that suggestion. #1002’s contract intentionally keeps the shared |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Maintainer takeover of #1002
This draft ports the useful parts of #1002 onto current
devand hardens the capability boundary found during maintainer review.Original implementation and follow-up work: @hanjianjun in #1002. Thank you for building the initial configurable vision-reasoning path.
Takeover goals
lowdefault/api/modelsdevand rerun the current CI matrixTDD state
The first commit intentionally contains regression tests only. They should fail on current
devuntil the takeover implementation is applied.Supersedes #1002 once this replacement is complete and verified.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation