fix: sanitize tool call arguments JSON on interrupted streaming to prevent 400 errors#1148
Conversation
…event 400 errors When streaming tool call responses are interrupted, the partially accumulated arguments (incomplete JSON) were saved to memory as-is. On subsequent requests, this malformed JSON was sent to the model API as tool call arguments, causing DashScope/OpenAI to reject with 400. Fixes agentscope-ai#1147 Change-Id: I23701960b5d4fa0962f872fb48a6252b13c3dc0b Co-developed-by: Cursor <noreply@cursor.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens tool-call argument handling during streaming so that interrupted tool-call argument accumulation doesn’t persist malformed JSON into conversation history and subsequent provider requests (addressing #1147).
Changes:
- Sanitize accumulated tool-call argument JSON in
ToolCallsAccumulatorto avoid keeping malformed/incomplete JSON strings. - Add defensive validation in both DashScope and OpenAI formatters to only use
ToolUseBlock.contentwhen it is valid JSON, otherwise serializeinput.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| agentscope-core/src/main/java/io/agentscope/core/formatter/openai/OpenAIMessageConverter.java | Routes tool-call argument resolution through a validating helper to avoid sending malformed JSON to OpenAI-compatible APIs. |
| agentscope-core/src/main/java/io/agentscope/core/formatter/dashscope/DashScopeToolsHelper.java | Adds validated tool-call argument resolution before building DashScope tool-call DTOs. |
| agentscope-core/src/main/java/io/agentscope/core/agent/accumulator/ToolCallsAccumulator.java | Adjusts tool-call accumulation/build logic to avoid persisting malformed JSON argument fragments on interrupted streams. |
| // Use raw content only if it was successfully parsed or args were | ||
| // already populated (meaning the content matches the parsed input). | ||
| // Otherwise fall back to "{}" to avoid sending malformed JSON | ||
| // (e.g. when streaming was interrupted mid-arguments). | ||
| String contentStr; | ||
| if (rawContentStr.isEmpty()) { | ||
| contentStr = "{}"; | ||
| } else if (!finalArgs.isEmpty() || rawContentParsed) { | ||
| contentStr = rawContentStr; | ||
| } else if (isValidJson(rawContentStr)) { | ||
| contentStr = rawContentStr; | ||
| } else { | ||
| contentStr = "{}"; |
There was a problem hiding this comment.
In ToolCallBuilder.build(), contentStr is set to rawContentStr whenever finalArgs is non-empty, but finalArgs being populated doesn’t guarantee rawContentStr is valid JSON (e.g., interrupted streaming could leave a partial rawContent while some args were already merged from input). This can still persist malformed JSON in memory. Consider validating rawContentStr (and ensuring it’s a JSON object) before using it; otherwise fall back to serializing finalArgs or "{}".
| private String resolveArgsJson(ToolUseBlock toolUse) { | ||
| String content = toolUse.getContent(); | ||
| if (content != null && !content.isEmpty()) { | ||
| if (isValidJson(content)) { | ||
| return content; | ||
| } | ||
| log.warn( | ||
| "Invalid JSON in tool call content for '{}', falling back to input" | ||
| + " serialization", | ||
| toolUse.getName()); | ||
| } | ||
|
|
||
| try { | ||
| return JsonUtils.getJsonCodec().toJson(toolUse.getInput()); | ||
| } catch (Exception e) { | ||
| String errorMsg = | ||
| e.getMessage() != null ? e.getMessage() : e.getClass().getSimpleName(); | ||
| log.warn("Failed to serialize tool call arguments: {}", errorMsg); | ||
| return "{}"; | ||
| } | ||
| } | ||
|
|
||
| private boolean isValidJson(String str) { | ||
| try { | ||
| JsonUtils.getJsonCodec().fromJson(str, Object.class); | ||
| return true; | ||
| } catch (Exception e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
isValidJson() currently treats any JSON value as valid (e.g. null, [], or a quoted string). Tool call arguments are expected to be a JSON object, so accepting non-object JSON can still lead to downstream 400s. Consider validating that parsing yields a Map/object node rather than just “parsable JSON”.
| private String resolveArgsJson(ToolUseBlock toolUse) { | ||
| String content = toolUse.getContent(); | ||
| if (content != null && !content.isEmpty()) { | ||
| if (isValidJson(content)) { | ||
| return content; | ||
| } | ||
| log.warn( | ||
| "Invalid JSON in tool call content for '{}', falling back to input" | ||
| + " serialization", | ||
| toolUse.getName()); | ||
| } | ||
|
|
||
| try { | ||
| return JsonUtils.getJsonCodec().toJson(toolUse.getInput()); | ||
| } catch (Exception e) { | ||
| log.warn("Failed to serialize tool call arguments: {}", e.getMessage()); | ||
| return "{}"; | ||
| } | ||
| } | ||
|
|
||
| private boolean isValidJson(String str) { | ||
| try { | ||
| JsonUtils.getJsonCodec().fromJson(str, Object.class); | ||
| return true; | ||
| } catch (Exception e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
isValidJson() validates only that the string is parseable JSON, but tool call arguments should be a JSON object. Values like null/arrays would pass validation and still be rejected by providers expecting an object. Consider validating the parsed value is a Map/object node before returning content as arguments.
| ToolUseBlock build() { | ||
| Map<String, Object> finalArgs = new HashMap<>(args); | ||
| String rawContentStr = this.rawContent.toString(); | ||
| boolean rawContentParsed = false; | ||
|
|
||
| // If no parsed arguments but has raw JSON content, try to parse | ||
| if (finalArgs.isEmpty() && rawContentStr.length() > 0) { | ||
| try { | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> parsed = | ||
| JsonUtils.getJsonCodec().fromJson(rawContentStr, Map.class); | ||
| if (parsed != null) { | ||
| finalArgs.putAll(parsed); | ||
| rawContentParsed = true; | ||
| } | ||
| } catch (Exception ignored) { | ||
| // Parsing failed, keep empty args | ||
| } | ||
| } | ||
|
|
||
| // Use raw content only if it was successfully parsed or args were | ||
| // already populated (meaning the content matches the parsed input). | ||
| // Otherwise fall back to "{}" to avoid sending malformed JSON | ||
| // (e.g. when streaming was interrupted mid-arguments). | ||
| String contentStr; | ||
| if (rawContentStr.isEmpty()) { | ||
| contentStr = "{}"; | ||
| } else if (!finalArgs.isEmpty() || rawContentParsed) { | ||
| contentStr = rawContentStr; | ||
| } else if (isValidJson(rawContentStr)) { | ||
| contentStr = rawContentStr; | ||
| } else { | ||
| contentStr = "{}"; | ||
| } | ||
|
|
||
| return ToolUseBlock.builder() | ||
| .id(toolId != null ? toolId : generateId()) | ||
| .name(name) | ||
| .input(finalArgs) | ||
| .content(rawContentStr.isEmpty() ? "{}" : rawContentStr) | ||
| .content(contentStr) | ||
| .metadata(metadata.isEmpty() ? null : metadata) |
There was a problem hiding this comment.
This change adds new fallback behavior for malformed/partial tool arguments, but there are no unit tests covering the interrupted/invalid JSON cases (e.g. content like {"query":"hel should produce "{}" or serialized input). Adding a test for this scenario would help prevent regressions in the next streaming/tool-call refactor.
| String argsJson = resolveArgsJson(toolUse); | ||
|
|
There was a problem hiding this comment.
resolveArgsJson() introduces a new behavior change (ignore invalid content and fall back to input serialization), but OpenAIMessageConverter’s tests currently cover only the content present/null/empty cases—not malformed JSON. Adding a unit test where ToolUseBlock.content is incomplete JSON would lock in the intended behavior and prevent reintroducing 400s.
| String argsJson = resolveArgsJson(toolUse); | ||
|
|
||
| DashScopeFunction function = DashScopeFunction.of(toolUse.getName(), argsJson); | ||
| DashScopeToolCall toolCall = |
There was a problem hiding this comment.
resolveArgsJson() changes how tool-call arguments are chosen (validate content before using it), but DashScopeToolsHelper tests only cover input-based arguments today. Adding a test case with malformed content (and a valid input map) would ensure this guardrail stays effective against interrupted streaming.
| /** | ||
| * Resolve the arguments JSON string from a ToolUseBlock, with validation | ||
| * to prevent sending malformed JSON (e.g. from interrupted streaming). | ||
| */ | ||
| private String resolveArgsJson(ToolUseBlock toolUse) { | ||
| String content = toolUse.getContent(); | ||
| if (content != null && !content.isEmpty()) { | ||
| if (isValidJson(content)) { | ||
| return content; | ||
| } | ||
| log.warn( | ||
| "Invalid JSON in tool call content for '{}', falling back to input" | ||
| + " serialization", | ||
| toolUse.getName()); | ||
| } | ||
|
|
||
| try { | ||
| return JsonUtils.getJsonCodec().toJson(toolUse.getInput()); | ||
| } catch (Exception e) { | ||
| String errorMsg = | ||
| e.getMessage() != null ? e.getMessage() : e.getClass().getSimpleName(); | ||
| log.warn("Failed to serialize tool call arguments: {}", errorMsg); | ||
| return "{}"; | ||
| } | ||
| } | ||
|
|
||
| private boolean isValidJson(String str) { | ||
| try { | ||
| JsonUtils.getJsonCodec().fromJson(str, Object.class); | ||
| return true; | ||
| } catch (Exception e) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
resolveArgsJson()/isValidJson() are now duplicated in both OpenAIMessageConverter and DashScopeToolsHelper (and there’s similar JSON validation in ToolCallsAccumulator). Consider extracting a small shared helper (e.g., in JsonUtils or a formatter utility) so validation rules don’t drift across providers over time.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…idation, add tests Address PR review feedback: - Extract duplicated resolveArgsJson/isValidJson into JsonUtils as shared isValidJsonObject() and resolveToolCallArgsJson() methods - Strengthen validation to reject non-object JSON (arrays, null, strings) since tool call arguments must be JSON objects - Fix edge case in ToolCallsAccumulator where finalArgs being non-empty didn't guarantee rawContent was valid JSON - Add comprehensive unit tests for all affected components Change-Id: I05640215b1de0a802434750a94804dd18e01605e Co-developed-by: Cursor <noreply@cursor.com>
Jackson's fromJson("null", Map.class) returns null instead of throwing,
so isValidJsonObject("null") incorrectly returned true. Added null
check on the parsed result to properly reject JSON null values.
Change-Id: I411e6305aa90b06eb3bc906745a66b13fa133e66
Co-developed-by: Cursor <noreply@cursor.com>
Summary
{"query": "hello wor) was saved to memory and sent as-is in subsequent API requests, causing DashScope (百炼) to reject with HTTP 400 due to invalid JSON in tool call arguments.ToolCallsAccumulator.build()now falls back to"{}"when raw content is not valid JSON, and (2) bothDashScopeToolsHelperandOpenAIMessageConvertervalidate content before using it as arguments, falling back to serializing theinputmap.Fixes #1147
Test plan
"{}"instead of broken JSON)Made with Cursor