-
Notifications
You must be signed in to change notification settings - Fork 2.8k
fix(agent): recover from unsupported image input instead of poisoning the turn #4896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems too fragile?
Maybe break it into a few different errors
contains("not support image") // for open ai models
|| contains("no endpoints found that support image input") // for deepseek models
|| contains("image inputs are not supported") // for claude models
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ollama outputs: "not support multi-modal inputs"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good instinct — the matcher is deliberately narrow, but we probed the suggestion before taking it, and the phrase list turns out not to be the binding constraint: the status gate is.
is_unsupported_image_input_erroronly runs inside thestatus == 404arms (both here and inopenrouter_post). We measured all four cases against the real post path:AgentError::Llm— never reaches the matcherAgentError::Llm— never reaches the matcherLlmModelNotFound— matcher ran, phrase absentUnsupportedImageInput— works todaySo adding the OpenAI/Claude phrases as written would close the comment without changing behavior for either provider — their rejections arrive as 400s and are swallowed upstream of the matcher. Real coverage means hoisting the check above the status dispatch, which is a structurally different patch.
Two more reasons we're holding to the narrow matcher in this PR:
contains("not support image")is broad, and misclassification strips images out of history on a turn where images were fine. The neighboring testopenrouter_post_404_unknown_model_stays_model_not_foundexists precisely because 404-classification mistakes send users to the wrong fix.Filed as a follow-up: broaden coverage (hoist above the status dispatch + per-provider phrases) once we have real captured rejection bodies to match against. The PR body now states the scope guarantee explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on the measurement above — you were right about Ollama, and I have the captured body now.
Ollama is reachable through our openai-compat path with no credential, so I pulled a text-only model and drove the real
buzz-agentbinary against it at this PR's head. The rejection is real, close to your guess but not identical:You guessed
"not support multi-modal inputs"; the actual wording isdoes not support multimodal requests— no hyphen, "requests" not "inputs". That gap is exactly why I wanted a captured body rather than a guessed phrase: a matcher built on the guess would have silently missed this. Also worth flagging for whoever implements it — the body is doubly encoded, so the real sentence sits in a JSON string nested insideerror.message.At this PR's head the failure is unchanged for Ollama — three consecutive turns die on the same 400, history stays poisoned. I also ran a proxy that rewrote only that 400 into the OpenRouter 404 + phrase: recovery fires and all three turns end
end_turn. Same binary, same rig, sole variable is the status and phrase. So the recovery machinery here is right; it just isn't reached.Two more measurements that constrain the fix, and they argue against the simple version:
model not found(text-only request, no image).model not foundeven when the request does carry an image.So we can't just add the phrase to the existing 404 arm or loosen that arm — on this provider 404 already means something else. Real coverage needs the check hoisted above the status dispatch, plus a test pinning that Ollama's model-not-found 404 keeps
LlmModelNotFound.Full evidence, controls, and a reproduction recipe are on #4899. OpenAI and Anthropic bodies are still uncaptured, so I'd keep those phrases out until someone has them verbatim.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: your instinct on the phrase list was better than my first reply gave it credit for. We stood up a credential-free live rig against Ollama and captured a real image rejection — and one of your guessed phrases is essentially real:
Two things the capture pins down:
model 'x' not found) means model-not-found even when the request carries an image, so the 404 classifications must stay as-is.error.message), which any structured matcher needs to know about.The live rig also confirmed the recovery machinery in this PR works end-to-end against a real provider (a proxy rewriting that 400 to the OpenRouter-shaped 404 produces clean
end_turnrecovery on the same binary).Sequencing: this PR lands as-is (correct for OpenRouter/DeepSeek, live-verified recovery), and the hoist + captured Ollama phrase + ordering tests come as a stacked PR tracked in #4899. OpenAI/Anthropic phrases stay out until someone captures real bodies from them.