fix: handle multimodal content in interim text and avoid retrying local processing errors - #66341
Conversation
…trying local processing errors (NousResearch#66267)
Related: #66218, #66235, and #66275 repair the same multimodal-content crash at shared-helper and call-site boundaries. This PR additionally changes retry classification; please consolidate the overlapping scope and remove the unrelated documentation tag-text edits. |
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean, well-scoped change with good test coverage. No security concerns.
Reviewed by Hermes Agent
…usResearch#66267) Second call site (non-streaming / gateway path) now flattens list-type content with flatten_message_text before the inline <think> regex and the surrogate sanitizer, matching the interim-text fix from the prior commit. Adds regression tests (tests/run_agent/test_66267_multimodal_interim.py) covering: - build_assistant_message with list content does not raise TypeError - inline <think> inside list content is extracted + stripped correctly - _interim_assistant_visible_text is safe for tool messages (list content) - duplicate_previous_interim dedup guards against tool messages Verified the tests fail without the fix (TypeError: expected string... got 'list') and pass with it.
|
Merged via #66945 — your commits cherry-picked with authorship preserved (rebase-merge). Your fix addressed the same root cause (#66267) with a broader approach: Thanks for the thorough fix and the error classifier concept! |
Fixes #66267
Problem
After a vision turn or context compaction, the next assistant message could trigger a nearly-infinite retry loop that ends with:
The API call itself succeeded; the crash happened during local post-processing of the previous assistant message when
assistant_msg["content"]was a multimodal parts list instead of a string. Because the exception fired beforemessages.append(assistant_msg), history was unchanged and every retry re-entered the same crash.Changes
run_agent.py: In_interim_assistant_visible_text, flattencontentwithflatten_message_textbefore passing it to_strip_think_blocks. This handles both string and structured parts list content.agent/agent_runtime_helpers.py: Hardenstrip_think_blocksat the entry point so it coerces list/dict/Nonecontent to a string, dropping non-text multimodal parts before running regexes. This protects all call sites of the helper.agent/conversation_loop.py: Classify errors in the outer loop based on traceback module provenance. Deterministic local processing errors (traceback passes through local post-processing modules and never enterschat_completion_helpers) now stop immediately instead of retrying until the iteration budget is exhausted. Error messages distinguish between API-call failures and local processing failures.agent/chat_completion_helpers.py(second call site): Inbuild_assistant_message, the reasoning-extraction fallback runsflatten_message_text()oncontentbefore the inline<think>regex and the surrogate sanitizer. This is the non-streaming-z/ gateway path (build_assistant_message) that hit the sameTypeErrorwith list content at "API call fix(cli): show correct config file path in /config command #89" (reported by @bemany). Usingflatten_message_text(rather than skipping the regex for non-str content) preserves the visible text from list parts instead of producing an empty string.Why no early
role == "assistant"guard?A reviewer suggested moving the existing
role == "assistant"guard before the text computation. It is intentionally not added: onceflatten_message_textnormalizes at the helper entry (strip_think_blocks), the pre-role call can no longer raise, and tool messages now return a safe string and are excluded by the existing dedup condition (role == "assistant" and finish_reason == "incomplete"). An earlier guard would be redundant defensive duplication. Open to adding it if maintainers prefer belt-and-suspenders.Related work
_interim_assistant_visible_text.strip_think_blocks.This PR addresses the same root cause but takes a slightly broader approach: it reuses the existing
flatten_message_texthelper at the call site, hardensstrip_think_blocksagainst non-string input, prevents deterministic local post-processing errors from being retried until the iteration budget is exhausted, and additionally normalizes thebuild_assistant_message(non-streaming) call site flagged in the issue thread.Testing
tests/agent/test_think_scrubber.py— passedtests/run_agent/test_run_agent.py— passedtests/run_agent/test_66267_multimodal_interim.py— new, pins the regression:build_assistant_messagewith list content does not raiseTypeError<think>inside list content is extracted + stripped correctly_interim_assistant_visible_textis safe for tool messages (list content)duplicate_previous_interimdedup guards against tool messagesTypeError: expected string... got 'list') and pass with it.