feat(vision): filter sidecar describers by real image-input capability - #1326
Conversation
Roadmap unit for replacing the vision sidecar's provider-name model list (every openai + every anthropic row) with a real image-input capability filter, an allowed-model list on the wire, and a compact delegation-style card. The load-bearing finding is that catalog inputModalities is not a truthful capability signal: applyProviderConfigHints deliberately adds "image" to a model listed in provider.noVisionModels, because Codex gates attachments client-side and a text-only entry would block the image before the sidecar could run. A blind model therefore advertises image input, so noVisionModels membership is a hard disqualifier checked before the modality list it rewrote. Two audit rounds with an independent reviewer are recorded in 002; the write gate rejects only models that can be PROVEN blind, never merely unknown ones, so an operator can still point at a model the catalog has never heard of. No production code in this commit.
The vision sidecar picker offered every model whose provider was openai or anthropic, which is both too wide and too narrow: it listed models that cannot see, while hiding capability behind a provider name. This adds the predicate that answers the real question. Two rules make it non-obvious, and both are load-bearing: - provider.noVisionModels marks models the proxy describes images FOR, and applyProviderConfigHints deliberately adds "image" to their advertised modalities so Codex does not block the attachment client-side. A blind model therefore advertises image input, so list membership is a hard disqualifier checked BEFORE the modality list it rewrote. - Catalog rows routinely omit inputModalities (live /api/models carries none for openai or anthropic rows), so the predicate is a tri-state and unknown stays eligible. Collapsing undefined into false would empty the picker. visionEligibleModelOptions is the suggestion list and is deliberately NOT the write gate: absence from it must never imply rejection. gpt-5.6-luna and claude-haiku-4-5 are guaranteed per enabled backend so a cold catalog still offers a usable describer. No caller yet; the management API and dashboard consume this in later layers. Plan: devlog/_plan/260809_vision_sidecar_model_filter/010
|
✅ Deterministic PR hygiene checks passed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR documents a roadmap for vision sidecar model filtering and implements its eligibility foundation. It detects image capability, excludes configured sidecar consumers, routes supported backends, preserves baselines, generates stable options, re-exports the API, and adds focused tests. ChangesVision sidecar model filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ProviderConfig
participant ModelCatalog
participant Eligibility
participant VisionPicker
ProviderConfig->>ModelCatalog: provide provider and model metadata
ModelCatalog->>Eligibility: provide candidate models
Eligibility->>Eligibility: apply capability and executor checks
Eligibility->>VisionPicker: return stable eligible options
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 |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
`@devlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md`:
- Around line 62-64: In
devlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md:62-64,
update the phase 2 backend fallback to return backends unchanged when no vision
executor is enabled; do not add baseline models. In
devlog/_plan/260809_vision_sidecar_model_filter/030_dashboard_vision_card.md:57-59,
make the phase 3 legacy fallback apply only when serverOptions is undefined,
treating an empty server result as authoritative. Add coverage for visionModels:
[] and configurations with no enabled vision executor.
In `@src/vision/eligibility.ts`:
- Around line 127-131: Update the baseline insertion loop in the vision
eligibility function to retain the resolved provider identity for each enabled
backend, then pass the baseline model through isVisionEligibleModel before
adding it to byValue. Ensure models listed in the provider’s noVisionModels are
excluded, and add a regression test covering a baseline rejected by that
provider configuration.
🪄 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: 21b43281-081e-41bc-8b0d-34b870eb1750
📒 Files selected for processing (10)
devlog/_plan/260809_vision_sidecar_model_filter/000_plan.mddevlog/_plan/260809_vision_sidecar_model_filter/001_capability_signal_inventory.mddevlog/_plan/260809_vision_sidecar_model_filter/002_audit_synthesis.mddevlog/_plan/260809_vision_sidecar_model_filter/010_vision_eligibility_core.mddevlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.mddevlog/_plan/260809_vision_sidecar_model_filter/030_dashboard_vision_card.mddevlog/_plan/260809_vision_sidecar_model_filter/040_stack_publication.mdsrc/vision/eligibility.tssrc/vision/index.tstests/vision-eligibility.test.ts
| // Neither side resolvable (fresh install, no login): fall back to both so the | ||
| // picker is populated rather than empty, matching the permissive-unknown rule. | ||
| return backends.length > 0 ? backends : ["openai", "anthropic"]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not replace an unavailable backend set with selectable legacy models.
When no executor is available, phase 2 returns both baselines. This advertises models that no sidecar can dispatch. If phase 2 returns an authoritative empty list instead, phase 3 currently restores the unfiltered legacy list. This violates the documented requirement that picker options are reachable by an enabled executor.
devlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md#L62-L64: Returnbackendsunchanged when no backend is enabled. Do not add unavailable baseline options.devlog/_plan/260809_vision_sidecar_model_filter/030_dashboard_vision_card.md#L57-L59: Use the legacy list only whenserverOptions === undefined. Treat[]as an authoritative server result.
Add coverage for a server response with visionModels: [] and for a configuration with no enabled vision executor.
📍 Affects 2 files
devlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md#L62-L64(this comment)devlog/_plan/260809_vision_sidecar_model_filter/030_dashboard_vision_card.md#L57-L59
🤖 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
`@devlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md`
around lines 62 - 64, In
devlog/_plan/260809_vision_sidecar_model_filter/020_management_api_allowed_models.md:62-64,
update the phase 2 backend fallback to return backends unchanged when no vision
executor is enabled; do not add baseline models. In
devlog/_plan/260809_vision_sidecar_model_filter/030_dashboard_vision_card.md:57-59,
make the phase 3 legacy fallback apply only when serverOptions is undefined,
treating an empty server result as authoritative. Add coverage for visionModels:
[] and configurations with no enabled vision executor.
![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: 2f42ab3b45
ℹ️ 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 provider = config.providers?.[providerName]; | ||
| return provider ? modelInList(provider.noVisionModels, modelId) : false; |
There was a problem hiding this comment.
When a persisted registry-managed provider predates a newer noVisionModels entry, this reads only the raw config and misses the classification that gatherRoutedModels and routedProviderConfig backfill from the registry. The catalog then supplies the blind row with its deliberately augmented inputModalities: ["text", "image"], so this function accepts exactly the sidecar consumer it is meant to reject. Resolve the provider through the same registry-enrichment path used by routing/catalog generation before checking noVisionModels.
AGENTS.md reference: src/AGENTS.md:L18-L18
Useful? React with 👍 / 👎.
| const adapter = config.providers?.[candidate.provider]?.adapter; | ||
| if (adapter === "anthropic") return "anthropic"; |
There was a problem hiding this comment.
When Anthropic OAuth is enabled alongside another adapter: "anthropic" provider such as Umans, Xiaomi, or an API-key gateway, this classifies that other provider's unique model IDs as Anthropic sidecar options. The option carries only the bare model ID, while planVisionSidecar always sends it through the provider returned by findAnthropicVisionProvider, so selecting such an option sends an unsupported ID to the unrelated OAuth endpoint and image description fails. Restrict candidates to the actual selected OAuth provider, or retain provider identity through selection and dispatch.
Useful? React with 👍 / 👎.
| if (candidate.native || SUPPORTED_NATIVE_OPENAI_SLUGS.has(candidate.id)) { | ||
| return advertisesImageInput(nativeInputModalities(candidate.id)) ?? true; |
There was a problem hiding this comment.
When a non-native provider exposes a model whose bare ID happens to equal a supported native slug, such as a custom gpt-5.4-mini row explicitly declaring inputModalities: ["text"], the slug-set check treats it as native and returns OpenAI's pinned image capability before consulting the row. This allows a positively known text-only candidate through the filter; use native metadata only when candidate.native is true, or otherwise scope the slug fallback to the canonical native provider.
Useful? React with 👍 / 👎.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Requesting changes because the current eligibility core can still publish describers that the runtime cannot safely execute.\n\nThe blocking cases are:\n\n1. Resolve the effective provider through the same registry-enrichment path used by routing before applying noVisionModels. A stale persisted provider can otherwise miss a newer registry exclusion and admit a known-blind model.\n2. Keep provider identity attached to every candidate. Treating every adapter: anthropic row as an Anthropic sidecar option lets models from Umans/Xiaomi/custom gateways get dispatched through the unrelated OAuth provider selected by the runtime.\n3. Use native capability metadata only for a genuinely native/canonical candidate. A custom row whose bare ID collides with a native slug must still honor its own text-only modalities.\n4. Baseline insertion must pass the same eligibility check, and no enabled vision executor must produce an authoritative empty set rather than selectable legacy baselines.\n\nPlease add regressions for registry-enriched no-vision classification, multiple Anthropic-compatible providers, a non-native native-slug collision, a rejected baseline, and no enabled executor. The direction is useful, but this head should not merge until these provider-identity boundaries are fixed.
…ider Four review findings shared one root: the predicate judged a row without the provider identity that actually governs it. - Consumer membership now reads the registry-enriched provider, matching what the catalog does before it force-adds "image" to exactly those rows. A config persisted before the registry learned noVisionModels no longer offers a blind describer. - Anthropic options are restricted to the provider that would really execute them. Dispatch goes through one OAuth provider, so a key-auth row speaking the same wire was offered and then sent somewhere it is unsupported. - Baselines run through the same eligibility check instead of bypassing it, and are dropped only on an explicit consumer listing, never on a silent table. - Native OpenAI metadata applies to a native row or the canonical provider only, so a routed row declaring ["text"] keeps its own answer.
…s rows Audit follow-up. The previous fallback still offered a canonical anthropic row when no executor name was supplied, so a key-auth provider — which findAnthropicVisionProvider never returns — could be picked and then fail at describe time. An option that cannot be dispatched is worse than a missing one. The executor name is now required for an Anthropic catalog suggestion; the side's baseline keeps the picker populated on its own. Baseline comments now say what the code does: offered unless explicitly listed as a sidecar consumer.
2 participants
Summary
The Vision sidecar model picker offered every model whose provider was
openaioranthropic. That is simultaneously too wide — it lists models that cannot see — and too narrow, because it asks about a provider name instead of a capability. Users have been asking for more models; this is the foundation that lets the answer be "any model that can actually accept an image".This layer adds
src/vision/eligibility.tsand nothing else. It has no caller yet; the management API and the dashboard consume it in the PRs above.Two rules make the predicate non-obvious, and both are load-bearing:
provider.noVisionModelsinverts the modality signal. That list marks models the proxy describes images for, andapplyProviderConfigHintsdeliberately adds"image"to their advertisedinputModalitiesso the Codex app does not block the attachment client-side before the sidecar can run. A blind model therefore advertises image input. Membership in that list is a hard disqualifier, checked before the modality list it rewrote.inputModalitiesentirely — the live/api/modelsresponse carries none for eitheropenaioranthropicrows. SomodelAcceptsImageInputis a tri-state, andundefinedstays eligible. Collapsing it tofalsewould have emptied the picker completely.visionEligibleModelOptionsis the suggestion list and is deliberately not the same set as the write gate that lands in the next PR: absence from it must never imply rejection.gpt-5.6-lunaandclaude-haiku-4-5are guaranteed per enabled backend so a cold or failed catalog still offers a usable describer.Verification
bun run typecheck— exit 0bun test tests/vision-eligibility.test.ts— 11 pass, 0 failbun run test(full suite) — 10146 pass, 0 fail, 632 filesisVisionSidecarConsumerearly return → 1 fail (the inversion guard is genuinely exercised)undefined→ 1 fail (the fallback branch is genuinely exercised)Checklist
Stack (merge bottom-up):
Review this PR's diff only.
Summary by CodeRabbit
New Features
Documentation
Tests