feat(management): publish vision describers and refuse the blind ones - #1327
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ 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:
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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Hygiene✅ 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: b640fe42aa
ℹ️ 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 row = candidates.find(candidate => candidate.id === requested); | ||
| if (row) return modelAcceptsImageInput(config, row) === false; |
There was a problem hiding this comment.
When any catalog or custom-provider row shares the requested bare ID, this early return skips the OpenAI/Anthropic probes below. For example, a custom provider row for o3-mini advertising inputModalities: ["text", "image"] makes the management routes accept and persist o3-mini, even though the OpenAI metadata proves that the model sent by the OpenAI sidecar is text-only. Check all matching rows and the actual execution-backend metadata, rejecting when any authoritative verdict proves the requested describer blind.
Useful? React with 👍 / 👎.
| return rows.filter(row => row.disabled !== true).map(row => ({ | ||
| provider: row.provider, | ||
| id: row.id, | ||
| ...(row.inputModalities ? { inputModalities: row.inputModalities } : {}), | ||
| ...(row.native ? { native: true } : {}), | ||
| })); |
There was a problem hiding this comment.
In a configuration with an Anthropic OAuth provider plus another Anthropic-compatible provider, these candidates include models from both providers, and visionEligibleModelOptions treats every adapter: "anthropic" row as reachable. However, planVisionSidecar executes only through the provider returned by findAnthropicVisionProvider, so selecting an image-capable private model exposed by the other provider sends that bare ID to the OAuth provider and fails upstream. Build the option list from the actual executable provider's rows rather than every management catalog row.
Useful? React with 👍 / 👎.
| if (!visionModels.some(option => option.value === visionModel)) { | ||
| visionModels.unshift({ value: visionModel, label: visionModel, backend: vs.backend ?? "openai" }); |
There was a problem hiding this comment.
When visionSidecar.backend is anthropic and no model override is stored, the runtime uses claude-sonnet-5, but this route computes visionModel as gpt-5.4-mini and now grandfathers that value as an Anthropic option. The API therefore publishes a selectable model that is neither configured nor the effective runtime default, and a client that submits the selected option will persist an OpenAI model for the Anthropic executor. Resolve the same backend-specific default as planVisionSidecar, or grandfather only an explicitly stored model; the identical post-PUT block needs the same correction.
Useful? React with 👍 / 👎.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes because the management API still validates and publishes options against catalog-wide bare IDs instead of the backend/provider that will actually execute the sidecar.\n\nA colliding row can make the validator return early before authoritative OpenAI/Anthropic metadata is checked. Likewise, models from any Anthropic-compatible provider are suggested even though runtime dispatch uses only the provider selected by findAnthropicVisionProvider. Finally, the grandfathered default is currently gpt-5.4-mini even when the selected backend is Anthropic, whose runtime default is claude-sonnet-5.\n\nPlease carry the exact execution provider/backend into option construction and validation, evaluate all relevant authoritative matches rather than returning on an unrelated bare-ID row, and resolve the same backend-specific default as planVisionSidecar. Add focused tests for bare-ID collisions, multiple Anthropic-compatible providers, and an Anthropic backend with no explicit model. This stacked head is not merge-ready until those cases are green.
GET and PUT /api/sidecar-settings now carry a visionModels list, and both routes that can set a vision describer refuse a model we can prove cannot see. The GUI filter alone would have been cosmetic: anything holding the admin token could still write a blind model into visionSidecar.model. The gate rejects only a PROVEN-blind model. An id no source knows stays allowed, because the runtime never required catalog membership and an operator may be ahead of our tables; tests/vision-reasoning-contract.ts pins custom-vision at 200 and this change keeps it there. The caller's backend is a hint, never the authority. Trusting it opened a laundering path: o3-mini is text-only in the OpenAI table but absent from the Anthropic one, so a client could claim backend anthropic and have a blind model read as merely unknown. Both families are now consulted and any positive text-only verdict wins, which is unambiguous because the two vendor tables share no bare model id. One policy module serves both routes. A gate on /api/sidecar-settings with a private copy in the Claude Code override is the same as no gate once the two drift. Plan: devlog/_plan/260809_vision_sidecar_model_filter/020
… them Three corrections, one theme: the API answered from a projection of the config rather than from what the runtime would actually do. - The write gate no longer stops at the first catalog row matching the id. An operator-authored row declaring image input could hide the canonical table that proves the same id blind, so o3-mini could be persisted as a describer. Every matching row and both backend probes are consulted; any positive blind verdict rejects, and an id no source knows is still accepted. - GET and the post-PUT response shared a copied block that defaulted to gpt-5.4-mini even under an anthropic backend, whose runtime default is claude-sonnet-5. Both now call one helper built on resolveEffectiveVisionModel, which planVisionSidecar also uses, so the report cannot drift from the run. - Anthropic catalog options are scoped to the OAuth provider the runtime would dispatch through, resolved once per request and threaded into the filter.
…rgument Audit follow-up. Defaulting the parameter to findAnthropicVisionProvider(config) read like sugar, but an explicit undefined argument — precisely the no-executor case — re-triggered the default in each helper, so one response could read the OAuth account store four times. The threaded parameter is now required, and only the top-level callers resolve it.
b640fe4 to
6196fc5
Compare
2 participants
Summary
Builds on #1326.
GETandPUT /api/sidecar-settingsnow carry avisionModelslist, and both routes that can set a vision describer refuse a model that can be proven blind.Without this layer the picker filter is cosmetic: anything holding the admin token could still write a text-only model into
visionSidecar.model, and the sidecar would spend real tokens producing nothing.Three decisions worth review attention:
tests/vision-reasoning-contract.test.tsalready pinscustom-visionat HTTP 200 and this change keeps it there. The suggestion list and the write gate are deliberately different sets.backendis a hint, never the authority. Trusting it opened a laundering path:o3-miniis text-only in the OpenAI metadata table but absent from the Anthropic one, so a client could claimbackend: "anthropic"and have a known-blind model read as merely unknown. Measured before the fix:{model: "o3-mini"}→ 400,{model: "o3-mini", backend: "openai"}→ 400,{model: "o3-mini", backend: "anthropic"}→ 200 and persisted. Both families are now consulted and any positive text-only verdict wins, which is unambiguous because the two vendor tables share zero bare model ids./api/sidecar-settingswith a private copy inside the Claude Code override is the same as no gate once the two drift, sovision-sidecar-options.tsowns the rule and both callers import it. The Claude Code dashboard control stays freeform by design — it is a text input with suggestions, not a constrained picker.The catalog is read once per request and reused for the rejection body, so a 400 does not cost two provider fetches.
Verification
bun run typecheck— exit 0bun test tests/sidecar-settings-vision-filter.test.ts— 11 pass, 0 failbun run test(full suite) — 10146 pass, 0 fail, 632 filesif (false)→ 1 fail, so the 400 path is genuinely coveredhandleManagementAPI, before vs after viagit stash: 200/200/200 with the model persisted → 400/400/400 with nothing persistedsaveConfigPreservingClaudeCodespy shows 0 calls on the reject path, so rejection precedes persistencewebSearchSidecarstill acceptso3-miniwith 200, so the gate did not leak into the wrong fielddocs-site/src/content/docs/guides/sidecars.mdupdated (SOT sync); translated locales deliberately left alone.Checklist
Stack (merge bottom-up):
Depends on #1326. Review this PR's diff only.