fix(vision): treat a model without image support as an answer, not a crash - #89186
fix(vision): treat a model without image support as an answer, not a crash#89186jackulau wants to merge 1 commit into
Conversation
…crash Fixes NousResearch#89114 A provider rejecting a request because the model has no vision is a well-formed request the model cannot answer. Three call sites reach the same auxiliary vision model by the same route and each mishandled that rejection in a different way. vision_analyze_tool already classified it and returned success: false with a readable sentence -- but logged it first at ERROR with exc_info=True. The traceback quoted in NousResearch#89114 as an "unhandled openai.BadRequestError" is that log line: a handled, classified, actionable outcome printed exactly like a crash. Log classified failures at WARNING without a stack; keep ERROR + traceback for the unclassified branch, where a stack earns its place. browser_vision did not classify at all. The same rejection that produced a sentence through vision_analyze handed the agent a raw `Error code: 400 - {...}` blob it could neither relay nor act on. It now recognises the capability and billing cases and names the config key that fixes the first (auxiliary.vision), falling back to the raw error text -- with the traceback -- only for failures nothing recognises. computer_use is the tool the issue is filed against, and its defect is the one the reporter felt. _route_capture_through_aux_vision read `analysis` out of vision_analyze_tool's envelope without checking `success`. That field is populated on the failure path too, so the rejection was merged into the response as `vision_analysis` -- the main model was told the screenshot "shows" a 400 blob. The caller already had the graceful degradation the issue asks for (screenshot omitted, vision_unavailable flagged, element index still drivable); it never fired because the helper never reported failure. It does now. The capability predicate is shared rather than duplicated: is_vision_capability_error is public because browser_tool imports it. content_policy deliberately stays out of it -- it is not a capability problem -- so vision_analyze_tool tests it separately and its branch behaviour is unchanged. 20 regression tests. Each of the four behaviours fails independently when its production change is reverted.
|
Disclosure on a self-overlap, since it is the kind of thing that is annoying to discover at merge time: my own open #87867 ( They are also independent on the merits: #87867 is a feature on the success path, this is a correctness fix on the failure path. Neither needs the other, and landing this one first makes #87867 strictly better — a directed question that the aux model cannot answer because it has no vision would otherwise be returned to the main model as if it had been answered. |
What does this PR do?
A provider rejecting a request because the model has no vision is a well-formed request the model cannot answer. Three call sites reach the same auxiliary vision model by the same route, and each mishandled that rejection differently. This makes all three treat it as an answer.
The reported traceback is not an unhandled exception.
vision_analyze_toolalready caught, classified and answered this; it just logged the caught exception atERRORwithexc_info=Truefirst. The stack quoted in #89114 as an "unhandledopenai.BadRequestError" is that log line. A handled, classified, user-actionable outcome printed exactly like a crash cost the reporter a triage cycle, and would cost the next person one.The defect they actually felt is in
computer_use._route_capture_through_aux_visionreadsanalysisout ofvision_analyze_tool's envelope without checkingsuccess. That field is populated on the failure path too — with an explanation of why it could not look, not a description of what it saw — so the rejection was merged into the response asvision_analysis. The main model was told the screenshot showsError code: 400 - {...}.The fix is small because the graceful degradation the issue asks for already exists in the caller and simply never fired.
_capture_responseomits the screenshot, flagsvision_unavailable: true, and tells the model "element-index actions still work — drive via the element list above." It is guarded on_route_capture_through_aux_visionreturningNone, which its own docstring says it does "on failure". It never reported this failure. Now it does — items 2 and 3 of the issue's Expected Behavior, using machinery already in the tree.Why not item 1 (pre-flight capability detection). Deliberately not implemented. There is no reliable capability signal for an arbitrary OpenAI-compatible endpoint —
/modelsdoes not carry modality for most providers, and the reporter's is a Go proxy in front of another vendor, so a local capability table would be wrong for exactly the setups that hit this. A static allowlist would also fail closed on new multimodal models. The provider's 400 is the capability check; it only has to be read as an answer. That costs one wasted call the first time, which is the better trade than silently refusing a model that would have worked. A per-model+provider negative cache is a reasonable separable follow-up and shouldn't gate this.Related Issue
Fixes #89114
Type of Change
Changes Made
tools/vision_tools.pyis_vision_capability_error(error)— the shared predicate. Public (no leading underscore) becausetools/browser_tool.pyimports it; the two tools reach the same aux model by the same route and must recognise the same rejection.vision_analyze_tool's handler setsclassifiedacross the existing if/elif chain and logs after it:WARNINGwithout a stack for a classified failure,ERROR+exc_info=Truekept for the unclassified branch, where a traceback earns its place.is_vision_capability_error(e) or "content_policy" in err_str.content_policyis deliberately excluded from the shared predicate — it is not a capability problem, andbrowser_visionshould not tell a user to changeauxiliary.visionbecause of it — but it still routes to the same branch here, so the truth table is unchanged. There is a test pinning exactly that.tools/browser_tool.pybrowser_vision's handler classifies before responding. Capability rejections become a sentence that names the config key that fixes it (auxiliary.vision); billing rejections get their own remedy; anything unrecognised keeps the raw error text and the traceback. Previously it classified nothing, so the same rejection that produced a sentence throughvision_analyzehanded the agent a rawError code: 400 - {...}blob.tools/computer_use/tool.py_route_capture_through_aux_visionreturnsNonewhen the envelope reportssuccess: false, logging what the aux model said. This routes the failure into the caller's existingvision_unavailabledegradation instead of presenting a failed analysis asvision_analysis.vision_unavailablework and the docstring did not follow).tests/tools/test_vision_capability_error_surface.py— new, 20 tests.How to Test
Reproduce the reported setup — point
auxiliary.visionat any text-only model and analyse a PNG:hermes --toolsets vision -q "Use vision_analyze on ./shot.png and describe it"Before: an
ERRORline with a fullopenai.BadRequestErrorstack (the traceback in the issue). After: aWARNINGwith the same message, no stack, and the samesuccess: falseJSON the tool has always returned.The
computer_usehalf — the one that changes behaviour rather than logging:hermes --toolsets computer_use -q "Capture the screen and tell me what app is in front"Before: the model receives
"vision_analysis": "<model> does not support vision ... Error code: 400 - {...}"under a key promising a description of the screen. After:"vision_unavailable": true, the screenshot omitted, and the AX/SOM element index intact so index-driven actions still work.Automated:
pytest tests/tools/test_vision_capability_error_surface.py -q # 20 passedMutation proof — each production change is independently load-bearing. Reverting one fails only its own test(s):
is_vision_capability_error→return Falsevision_analyzemessage +browser_visionmessage)browser_vision's classification blocktest_capability_rejection_becomes_a_sentence_not_a_400_blobclassifiedlog split → unconditionallogger.error(..., exc_info=True)test_classified_failure_logs_warning_without_a_tracebackcomputer_use'ssuccess is Falseguardtest_capability_failure_degrades_to_the_ax_payloadBaseline over the affected slice (
tests/tools/test_vision_*.py,test_browser_content_none_guard.py,test_computer_use*.py,tests/agent/test_vision_routing_31179.py,tests/gateway/test_vision_preprocess.py), run serially with and without the change: 6 failures both ways, the same 6 — all pre-existing and unrelated to this diff. 200 → 220 passed, the delta being exactly the new file.For the record, since I had to characterise them: two of the six (
TestBrowserSourceLinesAreGuardedintest_browser_content_none_guard.py) are a genuine Windows-only test bug —open(path)with noencoding=decodestools/browser_tool.pyas cp1252 and dies on the emoji in the tool registration. Three are Linux/env-specifictest_computer_use.pycases, and one istest_vision_routing_31179.py::test_vision_capable_main_usedfailing to resolve a client in this environment. I left all six alone rather than widen this PR; happy to send the one-lineencoding="utf-8"fix separately if that is wanted.Overlap with open PRs
Searched before building. Five open PRs touch
tools/vision_tools.py+tools/browser_tool.pytogether, and none of them touches theexcepthandlers this PR changes:browser_visionnative embedsvision_tools.pybut not its handlerimage_urlwhen a provider rejectsvideo_urlNone classifies a provider rejection in a handler, and none touches
tools/computer_use/tool.py. #72516 is the closest in spirit — it also reacts to a provider rejection — but it retries a different request shape rather than deciding how to report an unrecoverable one, and it is video-only.Worth a maintainer's attention: #51551 (
route opencode-go / opencode-zen captures to native fast path) is adjacent and this PR makes it safer rather than conflicting with it. #51551 marks the reporter's transport (opencode-go)supports_vision=Trueso captures take the native path. That is right for the vision-capable models behind that proxy and wrong formimo-v2.5, which is the one in this issue — per-model overrides still apply, but a user who has not set one would move the same 400 from the aux call onto the main-model turn. With this PR, whichever path is taken, a capability rejection degrades instead of narrating itself. No file conflict: #51551 editstests/tools/test_computer_use_capture_routing.py, which this PR does not touch.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qwholesale: on Windowstests/hermes_cli/cannot be collected (test_doctor_journal_modes.pycallsos.geteuid), so the full-suite result would be meaningless from here. CI runs it.Documentation & Housekeeping
docs/, docstrings) — docstrings only;_route_capture_through_aux_vision's return contract was stale and is correctedtmp_pathwith no shell-outs or path assumptionscomputer_use's response shape on this path was already documented by the existingvision_unavailablebranch; this PR only makes that branch reachable.Screenshots / Logs
Before (
computer_useon a text-only model — what the main model was handed):{ "mode": "som", "vision_analysis": "mimo-v2.5 does not support vision or our request was not accepted by the server. Error: Error code: 400 - {'error': {'message': 'This model does not support image inputs', ...}}", "vision_analysis_routed_via": "auxiliary.vision" }After:
{ "mode": "som", "elements": [{"index": 0, "role": "AXButton", "label": "Sign in", ...}], "summary": "... (vision unavailable: the auxiliary vision model could not be reached; screenshot omitted. Element-index actions still work — drive via the element list above.)", "vision_unavailable": true }And the log line the issue was filed on,
vision_analyzewith the reporter's provider string: