fix(agent): coerce tool-result content to string for OpenAI wire format - #31770
fix(agent): coerce tool-result content to string for OpenAI wire format#31770hclsys wants to merge 2 commits into
Conversation
Tranquil-Flow
left a comment
There was a problem hiding this comment.
Thanks for this fix — the approach of normalizing at make_tool_result_message() is exactly the right place. Clean and minimal.
One edge case worth adding: None tool content → empty string. Tool handlers can return None (no output, silent success), and strict OpenAI-compatible providers reject null content the same way they reject dicts. A two-line addition to _coerce_tool_result_content:
if content is None:
return ""(before the isinstance(str) check, since None isn't a str)
Also worth a quick test: test_none_content_becomes_empty_string.
Otherwise LGTM.
|
Good catch — you're right, |
Plugin tool handlers returning Dict[str, Any] were persisted into the chat history with a raw dict as the role:"tool" message content field. The OpenAI Chat Completions spec requires tool message content to be a string; strict upstreams reject the dict with HTTP 400 (Z.ai 1210, Manifest fallback_exhausted), while permissive providers silently coerce and mask the bug. make_tool_result_message now normalizes content via a single defensive helper: strings and multimodal content-part envelopes pass through unchanged; any other value is JSON-encoded with the json.dumps(default=str) idiom already used in this module. Fixes NousResearch#31435
Per review feedback on NousResearch#31770: a handler returning None (silent success) hit the json.dumps path and produced the literal string "null", which strict providers reject the same way as a dict. Map None -> "" before the other checks, with a regression test.
ca40aed to
cb837cf
Compare
|
Rebased onto current The two changes are complementary, not redundant — #31435 (non-string tool content) is still open and While rebasing I caught and fixed an interaction: All of |
… sessions Constraint: current main no longer has make_tool_result_message, so the fix had to land on the shared outbound sanitizer and tool-content boundary that still exist. Rejected: Reapply NousResearch#31770 verbatim | target helper is absent on current main Rejected: Reapply NousResearch#29920 verbatim | would stringify valid multimodal content-part lists Confidence: high Scope-risk: narrow Directive: Keep role=tool content coercion centralized and preserve wire-valid content-part lists when tightening future sanitizers. Tested: scripts/run_tests.sh tests/run_agent/test_agent_guardrails.py tests/tools/test_computer_use.py -q Not-tested: Full suite; live provider round-trip against strict upstreams
|
Re-verified on current Runtime proof on main: This PR is the right place to fix it: Cross-ref: the competing #31963 coerces in |
|
Thanks for the focused reproduction and multimodal analysis. This is now covered on
No release tag containing the fix was available locally. |
Fixes #31435.
Summary
Plugin tool handlers that return
Dict[str, Any]were persisted into the chat history with a raw dict in therole: "tool"messagecontentfield. The OpenAI Chat Completions spec requirestoolmessagecontentto be a string. Strict upstreams reject the dict with HTTP 400 (Z.ai1210, Manifestfallback_exhausted); permissive providers silently coerce, masking the bug until a provider switch.The fix normalizes
contentin one place —make_tool_result_message()— through a small defensive helper:str→ passes through unchanged_multimodal=True+content: list) → preserved as-is, so multipart-capable providers still get the listjson.dumps(content, default=str), falling back tostr()if unserializableThis reuses the exact
json.dumps(default=str)idiom already present in_multimodal_text_summaryrather than inventing a new serializer.Pre-implement audit
json.dumps(..., default=str)idiom and the existing_is_multimodal_tool_resultpredicate; no new serializer introduced.make_tool_result_messageis imported byagent_runtime_helpers.py,tool_executor.py,transports/chat_completions.py,mini_swe_runner.py. The signature is unchanged; the only behavior change is that a non-string, non-multimodalcontentis now stringified — which is what every OpenAI-spec caller already required. Multimodal dicts (the one legitimate non-string case) are explicitly preserved, so no caller contract is broken.Real-behavior proof
Before:
contentwas the raw dict → strict upstream returns 400. After: valid JSON string. Multimodal results verified still passed through as a list.Test plan
New
tests/test_tool_result_content_coercion.pycovers: dict→JSON string, string passthrough, multimodal preserved, bare-list stringified, non-serializable fallback, and that the other message fields (role/name/tool_name/tool_call_id) are unaffected.