fix(transport): strip empty/null tool_calls on assistant messages - #72591
fix(transport): strip empty/null tool_calls on assistant messages#72591TurgutKural wants to merge 3 commits into
Conversation
Related to the open empty-tool_calls family (nearest #68942; origin #58755). This patches convert_messages for null and assistant-role handling; #68942 also covers the run_agent sanitizer. The competing open fixes need a maintainer consolidation decision rather than a duplicate call. |
e936c18 to
349b18a
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused copy-on-write transport hardening. Current main still leaves an assistant tool_calls: [] or null unchanged in agent/transports/chat_completions.py:197-211, so the transport-level guard is a valid addition.
Problems
- The claimed auxiliary/custom-provider coverage is not delivered by this transport-only change.
agent/auxiliary_client.py:7225-7228builds raw request kwargs frommessages, and its async call path directly invokes the SDK atagent/auxiliary_client.py:8801; neither usesChatCompletionsTransport.convert_messages. tests/agent/transports/test_chat_completions_empty_tool_calls.py:77describes an empty-array case, but the fixture is a non-empty list at line 83 and correctly asserts preservation.
Suggested changes
- Either scope this to transport defense-in-depth, or add the normalization at the actual auxiliary request boundary and test that wire path.
- Rename or correct the contradictory Codex-fields test.
The normal conversation and iteration-summary paths already use the shared sanitizer (agent/conversation_loop.py:1640, agent/chat_completion_helpers.py:2167), so the added coverage should identify a real additional boundary. This is an automated hermes-sweeper review.
349b18a to
101a314
Compare
|
Rebased onto current upstream/main (7965462). The CI failure was a pre-existing vercel sandbox test issue (ImportError: lazy installs disabled) now fixed on main — not related to this PR's changes. |
78c8f55 to
b94ea97
Compare
|
Good catch — renamed |
63ccab0 to
bd554f9
Compare
SummarySeventy PRs reference a broad issue complex spanning Feishu recipient routing, provider/model catalogs, message-sequence repair, strict-provider payload normalization, HOME handling, delegation limits, reasoning configuration, and unrelated vision work. The target #72591 adds transport-level empty/null tool_calls normalization; the central unresolved dedup defect is instead addressed most directly by #67933, while several other sub-clusters are already implemented on main. Related pull requests
DuplicatesFeishu: #7694/#7769/#10040/#10123/#10502; empty assistant content: #11924/#11945/#12470/#12737/#31175/#31582/#31615/#59427/#59714/#63452/#64087/#71787/#73135; consecutive assistants: #29168/#34510/#49162 superseded by #55603; duplicate IDs: #58350/#58362/#60270/#60398; pre-existing empty tool_calls: #58768/#58953/#59110/#68942/#72591/#72619; dedup-created empty tool_calls: #64345/#64843/#65211/#67933/#70176/#70181/#70182/#70781/#74906/#77377. Kimi catalogs overlap across #10675/#10751/#13148/#13152/#13156/#13380/#46309/#53532/#65913, while GLM reasoning overlaps #69917/#70079/#70182. Suggested consolidationKeep #72591 open with a salvage path limited to transport defense-in-depth: retain its copy-on-write []/null normalization and corrected non-empty-call test, but revise the auxiliary/custom-provider claim or add normalization and a wire-level test at the actual auxiliary request boundary, explicitly following the keep_open review on #72591. For the canonical post-dedup defect, author action on best-fix #67933: rebase its focused source hunk and regression onto current main; #64345, #64843, #65211, #70176, #70181, #70182, #70781, and the core hunk of #77377 can then close as duplicates, while #74906 should remain separate only if maintainers want its additional post-dedup empty-content repair. Split #77377's unrelated delegate-tool refactor before further review so the sanitizer fix and documentation work can be evaluated independently. Cross-PR triage: Reviewed 70 pull requests and 27 issues in this complex. Diffs were read for 63 of 70 PRs (rest unavailable); Assessment working set: 486 kB of PR diffs, 253 kB of issue/PR text, 99 kB of discussion (158 comments), 111 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
bd554f9 to
f39b454
Compare
f39b454 to
23557ff
Compare
|
Noted — this sweep's relevant line is the relationship to #67933. They are complementary, not competing:
Different files, different layers, no overlap. Both remain open; a maintainer consolidation decision can land them independently (either order works — each fixes a distinct production path). |
4a4e2b7 to
0af8b1f
Compare
78d565b to
738acf3
Compare
|
Response to the scope point from GottZ consolidation sweep: the "auxiliary / custom provider routes" claim in the review comment was narrowed (a3148fd2). Actual scope: this layer is only the last boundary for requests serialized through this transport; auxiliary clients that build fully separate payload paths never pass through this layer and are out of scope. Behavior is unchanged — only the claim text was corrected. |
42f0a32 to
cd1f4e5
Compare
Strict OpenAI-compatible providers (onerouter / Qwen, DeepSeek v4) reject an assistant message carrying tool_calls: [] (or null) with HTTP 400 'Empty tool_calls is not supported in message.' The pre-API sanitizer in agent_runtime_helpers.sanitize_api_messages already drops these on the conversation_loop path, but auxiliary / custom-provider routes that bypass that sanitizer can still reach the wire with an invalid empty array and abort the whole session (non-retryable 400). Normalize at the transport layer too: detect an empty-list / null tool_calls on assistant messages, strip the key on the per-call copy (never mutate the stored history), and keep real tool_calls untouched. Includes unit tests covering empty-list, null, real-call preservation, mixed batches, user-role non-mutation, copy-on-write, and cross-provider parity. Follow-up to #58755.
…ping, not empty array Reviewer noted the test name suggested an empty-array case but the fixture has one tool call; renamed to match actual behavior.
cd1f4e5 to
a2389e1
Compare
|
Merged via #86654 — your three commits were cherry-picked onto current main with your authorship preserved in git log. Thank you! The salvage combines your transport-boundary strip with the dedup-pass fix (#64345) and the repair-merge fix (#77944) so the whole empty- |
Summary
Strict OpenAI-compatible providers (notably
onerouter/ Qwen and DeepSeek v4) reject an assistant message that carriestool_calls: [](empty array) — ortool_calls: null— with a non-retryable HTTP 400:The pre-API sanitizer in
agent.agent_runtime_helpers.sanitize_api_messagesalready strips these, but only on theconversation_looppath. Auxiliary / custom-provider routes can reach the wire without passing through that sanitizer, so a stale emptytool_callsarray aborts the entire session with a 400 that the retry loop cannot recover from.This PR normalizes the same invariant at the transport layer (
ChatCompletionsTransport.convert_messages) so every route agrees.Root cause
convert_messagesalready walkstool_callsto strip Codex scaffolding fields (call_id,response_item_id,extra_content), but it never handled the case where the array itself is empty/invalid. An emptytool_callson an assistant message slipped through to the wire.Fix
In
agent/transports/chat_completions.py::convert_messages:needs_sanitize=Truewhen an assistant message carries"tool_calls"that is an empty list or explicitnull.tool_callskey when empty/null. Real calls (non-empty arrays) are preserved and still get their Codex scaffolding stripped as before.tool_callson other roles is left untouched, and copy-on-write semantics are preserved (input list is never mutated in place).Evidence
customprovider session againsthttps://llm.onerouter.pro/v1(modelqwen/qwen3.8-max-preview:free) failed with the exact error above, repeated under the same request id — i.e. the same invalid payload was resent every attempt, confirming the sanitizer was being bypassed on that route.agent.agent_runtime_helpers.sanitize_api_messages(commita7932d86c, [Bug]: repair_message_sequence creates empty tool_calls array, triggering DeepSeek v4 HTTP 400 (follow-up to #56980) #58755) already dropstool_calls: []on the conversation_loop path; this change extends the same guarantee to the transport layer.Tests
New file
tests/agent/transports/test_chat_completions_empty_tool_calls.py(10 cases):nulldroppedtool_callsleft untouchedqwen,deepseek-v4,gpt-4o)Full run:
tests/agent/transports/+tests/run_agent/test_message_sequence_repair.py+ sanitizer suites → 502 passed, no regressions.Backward compatibility
Pure additive normalization at the transport layer. No schema changes, no behavior change for well-formed payloads. Strict providers that previously 400 now receive a valid message; permissive providers already ignored the empty array, so their behavior is unchanged.