fix: recover malformed tool calls from GLM/Minimax models instead of rejecting them (#10230)

Co-authored-by: Douwe M Osinga <douwe@sidewalklabs.com>
This commit is contained in:
Herley 2026-07-06 09:37:36 +07:00 committed by GitHub
parent f452ea9b3e
commit 1b2f77f719
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 563 additions and 97 deletions

View file

@ -709,13 +709,13 @@ pub fn response_to_message(response: &Value) -> anyhow::Result<Message> {
})
.filter(|m: &serde_json::Map<String, Value>| !m.is_empty());
if !is_valid_function_name(&function_name) {
if function_name.is_empty() {
let error = ErrorData {
code: ErrorCode::INVALID_REQUEST,
message: Cow::from(format!(
"The provided function name '{}' had invalid characters, it must match this regex [a-zA-Z0-9_-]+",
function_name
)),
message: Cow::from(
"The provided function name was empty; a tool call must name a tool"
.to_string(),
),
data: None,
};
content.push(MessageContent::tool_request_with_metadata(
@ -723,54 +723,50 @@ pub fn response_to_message(response: &Value) -> anyhow::Result<Message> {
Err(error),
metadata.as_ref(),
));
} else {
match parse_tool_arguments(&arguments_str) {
Some(params) if params.is_object() => {
content.push(MessageContent::tool_request_with_metadata(
continue;
}
match parse_tool_arguments(&arguments_str) {
Some(params) if params.is_object() => {
content.push(MessageContent::tool_request_with_metadata(
id,
Ok(CallToolRequestParams::new(function_name)
.with_arguments(object(params))),
metadata.as_ref(),
));
}
Some(other) => {
let error = ErrorData {
code: ErrorCode::INVALID_PARAMS,
message: Cow::from(format!(
"Tool arguments for {} (id {}) must be a JSON object, got {}. Raw arguments: '{}'",
function_name,
id,
Ok(CallToolRequestParams::new(function_name)
.with_arguments(object(params))),
metadata.as_ref(),
));
}
// Valid JSON but NOT an object (a bare array/string/number).
// Weaker models emit this; surface a tool error so the model
// retries with a proper object instead of crashing the run
// (rmcp's `object()` debug-asserts on non-objects).
Some(other) => {
let error = ErrorData {
code: ErrorCode::INVALID_PARAMS,
message: Cow::from(format!(
"Tool arguments for {} (id {}) must be a JSON object, got {}. Raw arguments: '{}'",
function_name,
id,
describe_json_value(&other),
arguments_str
)),
data: None,
};
content.push(MessageContent::tool_request_with_metadata(
id,
Err(error),
metadata.as_ref(),
));
}
None => {
let message_text = truncation_error_message(&arguments_str)
.unwrap_or_else(|| {
format!("Could not interpret tool use parameters for id {id}")
});
let error = ErrorData {
code: ErrorCode::INVALID_PARAMS,
message: Cow::from(message_text),
data: None,
};
content.push(MessageContent::tool_request_with_metadata(
id,
Err(error),
metadata.as_ref(),
));
}
describe_json_value(&other),
arguments_str
)),
data: None,
};
content.push(MessageContent::tool_request_with_metadata(
id,
Err(error),
metadata.as_ref(),
));
}
None => {
let message_text =
truncation_error_message(&arguments_str).unwrap_or_else(|| {
format!("Could not interpret tool use parameters for id {id}")
});
let error = ErrorData {
code: ErrorCode::INVALID_PARAMS,
message: Cow::from(message_text),
data: None,
};
content.push(MessageContent::tool_request_with_metadata(
id,
Err(error),
metadata.as_ref(),
));
}
}
}
@ -2012,10 +2008,9 @@ mod tests {
}
#[test]
fn test_response_to_message_invalid_func_name() -> anyhow::Result<()> {
fn test_response_to_message_empty_func_name() -> anyhow::Result<()> {
let mut response: Value = serde_json::from_str(OPENAI_TOOL_USE_RESPONSE)?;
response["choices"][0]["message"]["tool_calls"][0]["function"]["name"] =
json!("invalid fn");
response["choices"][0]["message"]["tool_calls"][0]["function"]["name"] = json!("");
let message = response_to_message(&response)?;
@ -2028,7 +2023,7 @@ mod tests {
}) => {
assert!(msg.starts_with("The provided function name"));
}
_ => panic!("Expected ToolNotFound error"),
_ => panic!("Expected invalid-request error for empty name"),
}
} else {
panic!("Expected ToolRequest content");
@ -2037,6 +2032,48 @@ mod tests {
Ok(())
}
#[test]
fn test_response_to_message_passes_names_through_to_dispatch() -> anyhow::Result<()> {
for name in [
"developer.shell",
"functions.example_fn",
"example fn",
"???!",
] {
let mut response: Value = serde_json::from_str(OPENAI_TOOL_USE_RESPONSE)?;
response["choices"][0]["message"]["tool_calls"][0]["function"]["name"] = json!(name);
let message = response_to_message(&response)?;
if let MessageContent::ToolRequest(request) = &message.content[0] {
let tool_call = request.tool_call.as_ref().expect("tool call should parse");
assert_eq!(tool_call.name, name, "name must pass through verbatim");
} else {
panic!("Expected ToolRequest content");
}
}
Ok(())
}
#[test]
fn test_response_to_message_fenced_arguments() -> anyhow::Result<()> {
let mut response: Value = serde_json::from_str(OPENAI_TOOL_USE_RESPONSE)?;
response["choices"][0]["message"]["tool_calls"][0]["function"]["arguments"] =
json!("```json\n{\"param\": \"value\"}\n```");
let message = response_to_message(&response)?;
if let MessageContent::ToolRequest(request) = &message.content[0] {
let tool_call = request.tool_call.as_ref().expect("tool call should parse");
assert_eq!(tool_call.arguments, Some(object!({"param": "value"})));
} else {
panic!("Expected ToolRequest content");
}
Ok(())
}
#[test]
fn test_response_to_message_json_decode_error() -> anyhow::Result<()> {
let mut response: Value = serde_json::from_str(OPENAI_TOOL_USE_RESPONSE)?;
@ -3825,6 +3862,68 @@ data: [DONE]"#;
Ok(())
}
#[tokio::test]
async fn test_streaming_tool_call_dotted_name_passes_through() -> anyhow::Result<()> {
let response_lines = concat!(
"data: {\"id\":\"x\",\"object\":\"chat.completion.chunk\",\"model\":\"m\",\"choices\":[{\"index\":0,\"delta\":{\"role\":\"assistant\",\"tool_calls\":[{\"index\":0,\"id\":\"tc1\",\"type\":\"function\",\"function\":{\"name\":\"ext__db.query\",\"arguments\":\"{\\\"command\\\": \\\"ls\\\"}\"}}]},\"finish_reason\":\"tool_calls\"}]}\n",
"data: [DONE]"
);
let lines: Vec<String> = response_lines.lines().map(|s| s.to_string()).collect();
let response_stream = tokio_stream::iter(lines.into_iter().map(Ok));
let mut messages = std::pin::pin!(response_to_streaming_message(response_stream));
let mut tool_calls = Vec::new();
while let Some(result) = messages.next().await {
let (message, _usage) = result?;
if let Some(msg) = message {
for content in &msg.content {
if let MessageContent::ToolRequest(request) = content {
let tool_call = request.tool_call.as_ref().expect("tool call should parse");
tool_calls.push((tool_call.name.to_string(), tool_call.arguments.clone()));
}
}
}
}
assert_eq!(tool_calls.len(), 1);
assert_eq!(tool_calls[0].0, "ext__db.query");
assert_eq!(tool_calls[0].1, Some(object!({"command": "ls"})));
Ok(())
}
#[tokio::test]
async fn test_streaming_tool_call_degenerate_name_passes_through() -> anyhow::Result<()> {
let response_lines = concat!(
"data: {\"id\":\"x\",\"object\":\"chat.completion.chunk\",\"model\":\"m\",\"choices\":[{\"index\":0,\"delta\":{\"role\":\"assistant\",\"tool_calls\":[{\"index\":0,\"id\":\"tc1\",\"type\":\"function\",\"function\":{\"name\":\"???\",\"arguments\":\"{}\"}}]},\"finish_reason\":\"tool_calls\"}]}\n",
"data: [DONE]"
);
let lines: Vec<String> = response_lines.lines().map(|s| s.to_string()).collect();
let response_stream = tokio_stream::iter(lines.into_iter().map(Ok));
let mut messages = std::pin::pin!(response_to_streaming_message(response_stream));
let mut names = Vec::new();
while let Some(result) = messages.next().await {
let (message, _usage) = result?;
if let Some(msg) = message {
for content in &msg.content {
if let MessageContent::ToolRequest(request) = content {
names.push(
request
.tool_call
.as_ref()
.expect("passes through")
.name
.to_string(),
);
}
}
}
}
assert_eq!(names, vec!["???".to_string()]);
Ok(())
}
#[tokio::test]
async fn test_streaming_multiple_tool_calls_without_tool_call_index() -> anyhow::Result<()> {
let response_lines = concat!(

View file

@ -215,6 +215,56 @@ pub fn truncation_error_message(args: &str) -> Option<String> {
))
}
fn strip_json_wrapper(args: &str) -> Option<&str> {
let trimmed = args.trim();
if let Some(rest) = trimmed.strip_prefix("```") {
let body = rest.strip_suffix("```")?.trim_start();
let body = body
.strip_prefix("json")
.or_else(|| body.strip_prefix("JSON"))
.unwrap_or(body);
return Some(body.trim());
}
if trimmed.starts_with('<') && !trimmed.starts_with("</") {
let open_end = trimmed.find('>')?;
let tag_name = trimmed.get(1..open_end)?.split_whitespace().next()?;
if tag_name.is_empty()
|| !tag_name
.chars()
.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-')
{
return None;
}
let closing = format!("</{tag_name}>");
let body = trimmed
.get(open_end + 1..)?
.trim_end()
.strip_suffix(&closing)?;
return Some(body.trim());
}
None
}
fn unwrap_double_encoded_object(value: serde_json::Value) -> serde_json::Value {
let serde_json::Value::String(s) = &value else {
return value;
};
let mut current = s.trim().to_string();
for _ in 0..3 {
match serde_json::from_str::<serde_json::Value>(&current) {
Ok(inner @ serde_json::Value::Object(_)) => return inner,
Ok(serde_json::Value::String(inner)) => current = inner.trim().to_string(),
_ => break,
}
}
value
}
/// Parse tool-call arguments, returning `None` when the input looks truncated
/// so callers can surface an actionable error rather than invoking a tool with
/// incomplete arguments. Non-truncated malformation (e.g. unescaped control
@ -225,12 +275,23 @@ pub fn parse_tool_arguments(args: &str) -> Option<serde_json::Value> {
}
if let Ok(value) = serde_json::from_str::<serde_json::Value>(args) {
return Some(value);
return Some(unwrap_double_encoded_object(value));
}
if let Some(inner) = strip_json_wrapper(args) {
if let Ok(value) = serde_json::from_str::<serde_json::Value>(inner) {
return Some(unwrap_double_encoded_object(value));
}
if !looks_truncated(inner) {
if let Ok(value) = safely_parse_json(inner) {
return Some(unwrap_double_encoded_object(value));
}
}
}
if !looks_truncated(args) {
if let Ok(value) = safely_parse_json(args) {
return Some(value);
return Some(unwrap_double_encoded_object(value));
}
}
@ -416,6 +477,78 @@ mod tests {
assert!(msg.contains("cut off at:"), "msg: {msg}");
}
#[test]
fn test_parse_tool_arguments_markdown_fenced() {
let fenced = "```json\n{\"command\": \"ls\"}\n```";
let parsed = parse_tool_arguments(fenced).expect("fenced JSON should parse");
assert_eq!(parsed["command"], "ls");
let fenced_no_lang = "```\n{\"command\": \"ls\"}\n```";
let parsed = parse_tool_arguments(fenced_no_lang).expect("fenced JSON should parse");
assert_eq!(parsed["command"], "ls");
let fenced_inline = "```json{\"command\": \"ls\"}```";
let parsed = parse_tool_arguments(fenced_inline).expect("fenced JSON should parse");
assert_eq!(parsed["command"], "ls");
}
#[test]
fn test_parse_tool_arguments_double_encoded() {
let double_encoded = r#""{\"command\": \"ls\"}""#;
let parsed = parse_tool_arguments(double_encoded).expect("double-encoded should parse");
assert!(parsed.is_object(), "expected object, got {parsed:?}");
assert_eq!(parsed["command"], "ls");
let twice = serde_json::to_string(&serde_json::json!(r#"{"command": "ls"}"#)).unwrap();
let twice = serde_json::to_string(&serde_json::json!(twice)).unwrap();
let parsed = parse_tool_arguments(&twice).expect("twice-encoded should parse");
assert!(parsed.is_object(), "expected object, got {parsed:?}");
assert_eq!(parsed["command"], "ls");
}
#[test]
fn test_parse_tool_arguments_xml_wrapped() {
let wrapped = r#"<tool_call>{"command": "ls"}</tool_call>"#;
let parsed = parse_tool_arguments(wrapped).expect("xml-wrapped JSON should parse");
assert_eq!(parsed["command"], "ls");
let with_attrs = r#"<tool_call id="1">{"command": "ls"}</tool_call>"#;
let parsed = parse_tool_arguments(with_attrs).expect("xml-wrapped JSON should parse");
assert_eq!(parsed["command"], "ls");
}
#[test]
fn test_parse_tool_arguments_unrecoverable_wrappers_still_fail() {
assert!(parse_tool_arguments(r#"<a><b>{"x": 1}</b></a>"#).is_none());
assert!(parse_tool_arguments("```json\n{\"x\": 1}\n``` extra").is_none());
assert!(parse_tool_arguments(r#"<tool_call>{"x": 1}</other>"#).is_none());
assert!(parse_tool_arguments(r#"</tool_call>"#).is_none());
assert!(parse_tool_arguments("```").is_none());
}
#[test]
fn test_parse_tool_arguments_non_object_json_unchanged() {
assert_eq!(parse_tool_arguments("null"), Some(serde_json::Value::Null));
assert_eq!(parse_tool_arguments("42"), Some(serde_json::json!(42)));
assert_eq!(
parse_tool_arguments("[1, 2]"),
Some(serde_json::json!([1, 2]))
);
}
#[test]
fn test_parse_tool_arguments_plain_string_preserved() {
let plain = r#""hello""#;
let parsed = parse_tool_arguments(plain).expect("valid JSON string should parse");
assert_eq!(parsed, serde_json::json!("hello"));
}
#[test]
fn test_parse_tool_arguments_fenced_truncated_still_fails() {
let truncated = "```json\n{\"path\": \"/report.md\", \"content\": \"# cut";
assert!(parse_tool_arguments(truncated).is_none());
}
#[test]
fn test_truncation_error_message_malformed() {
// Malformed JSON that ends with } (not truncated, just broken).

View file

@ -275,6 +275,36 @@ pub fn get_tool_owner(tool: &Tool) -> Option<String> {
.map(|s| s.to_string())
}
fn recover_mangled_tool_name<'a>(
emitted: &str,
tool_names: impl Iterator<Item = &'a str>,
) -> Option<String> {
let trimmed = emitted.trim();
let stripped = trimmed
.strip_prefix("functions.")
.or_else(|| trimmed.strip_prefix("functions:"))
.unwrap_or(trimmed);
let mut matched: Option<&str> = None;
for name in tool_names {
let separator_mangled = name
.split_once("__")
.map(|(extension, tool)| format!("{extension}.{tool}"));
let matches = stripped == name || separator_mangled.as_deref() == Some(stripped);
if name == emitted || !matches {
continue;
}
match matched {
None => matched = Some(name),
Some(prev) if prev == name => {}
Some(_) => return None,
}
}
matched.map(|s| s.to_string())
}
fn get_tool_meta_value(tool: &Tool) -> Option<Value> {
tool.meta.as_ref().map(|meta| Value::Object(meta.0.clone()))
}
@ -1673,56 +1703,68 @@ impl ExtensionManager {
)
})?;
if let Some(tool) = tools.iter().find(|t| *t.name == *tool_name) {
let owner = get_tool_owner(tool)
.or_else(|| {
tool_name
.split_once("__")
.map(|(prefix, _)| name_to_key(prefix))
})
.ok_or_else(|| {
let mut name = tool_name.to_string();
let mut recovery_attempted = false;
loop {
if let Some(tool) = tools.iter().find(|t| *t.name == *name) {
let owner = get_tool_owner(tool)
.or_else(|| name.split_once("__").map(|(prefix, _)| name_to_key(prefix)))
.ok_or_else(|| {
ErrorData::new(
ErrorCode::RESOURCE_NOT_FOUND,
format!("Tool '{}' has no owner", name),
None,
)
})?;
let actual_tool_name = name
.strip_prefix(&format!("{owner}__"))
.unwrap_or(&name)
.to_string();
let client = self.get_server_client(&owner).await.ok_or_else(|| {
ErrorData::new(
ErrorCode::RESOURCE_NOT_FOUND,
format!("Tool '{}' has no owner", tool_name),
format!("Extension '{}' not found for tool '{}'", owner, name),
None,
)
})?;
let actual_tool_name = tool_name
.strip_prefix(&format!("{owner}__"))
.unwrap_or(tool_name)
.to_string();
let client = self.get_server_client(&owner).await.ok_or_else(|| {
ErrorData::new(
ErrorCode::RESOURCE_NOT_FOUND,
format!("Extension '{}' not found for tool '{}'", owner, tool_name),
None,
)
})?;
return Ok(ResolvedTool {
tool_name: tool.name.to_string(),
extension_name: owner,
actual_tool_name,
client,
tool_meta: get_tool_meta_value(tool),
resource_uri: get_tool_resource_uri(tool),
});
}
if let Some((prefix, actual)) = tool_name.split_once("__") {
let owner = name_to_key(prefix);
if let Some(client) = self.get_server_client(&owner).await {
return Ok(ResolvedTool {
tool_name: tool_name.to_string(),
tool_name: tool.name.to_string(),
extension_name: owner,
actual_tool_name: actual.to_string(),
actual_tool_name,
client,
tool_meta: None,
resource_uri: None,
tool_meta: get_tool_meta_value(tool),
resource_uri: get_tool_resource_uri(tool),
});
}
if let Some((prefix, actual)) = name.split_once("__") {
let owner = name_to_key(prefix);
if let Some(client) = self.get_server_client(&owner).await {
return Ok(ResolvedTool {
tool_name: name.to_string(),
extension_name: owner,
actual_tool_name: actual.to_string(),
client,
tool_meta: None,
resource_uri: None,
});
}
}
if !recovery_attempted {
recovery_attempted = true;
if let Some(recovered) =
recover_mangled_tool_name(&name, tools.iter().map(|t| t.name.as_ref()))
{
name = recovered;
continue;
}
}
break;
}
let available = tools
@ -2594,6 +2636,198 @@ mod tests {
);
}
struct MockDottedClient {}
#[async_trait::async_trait]
impl McpClientTrait for MockDottedClient {
fn get_info(&self) -> Option<&InitializeResult> {
None
}
async fn list_resources(
&self,
_session_id: &str,
_next_cursor: Option<String>,
_cancellation_token: CancellationToken,
) -> Result<ListResourcesResult, Error> {
Err(Error::TransportClosed)
}
async fn read_resource(
&self,
_session_id: &str,
_uri: &str,
_cancellation_token: CancellationToken,
) -> Result<ReadResourceResult, Error> {
Err(Error::TransportClosed)
}
async fn list_tools(
&self,
_session_id: &str,
_next_cursor: Option<String>,
_cancellation_token: CancellationToken,
) -> Result<ListToolsResult, Error> {
use serde_json::json;
use std::sync::Arc;
Ok(ListToolsResult {
tools: vec![
Tool::new(
"db.query".to_string(),
"A tool with a dotted name".to_string(),
Arc::new(json!({}).as_object().unwrap().clone()),
),
Tool::new(
"db__query".to_string(),
"A sibling with the separator name".to_string(),
Arc::new(json!({}).as_object().unwrap().clone()),
),
],
next_cursor: None,
meta: None,
})
}
async fn call_tool(
&self,
_ctx: &ToolCallContext,
name: &str,
_arguments: Option<JsonObject>,
_cancellation_token: CancellationToken,
) -> Result<CallToolResult, Error> {
match name {
"db.query" | "db__query" => Ok(CallToolResult::success(vec![])),
_ => Err(Error::TransportClosed),
}
}
async fn list_prompts(
&self,
_session_id: &str,
_next_cursor: Option<String>,
_cancellation_token: CancellationToken,
) -> Result<ListPromptsResult, Error> {
Err(Error::TransportClosed)
}
async fn get_prompt(
&self,
_session_id: &str,
_name: &str,
_arguments: Value,
_cancellation_token: CancellationToken,
) -> Result<GetPromptResult, Error> {
Err(Error::TransportClosed)
}
async fn subscribe(&self) -> mpsc::Receiver<ServerNotification> {
mpsc::channel(1).1
}
}
#[tokio::test]
async fn test_resolve_tool_recovers_dotted_mangled_name() {
let temp_dir = tempfile::tempdir().unwrap();
let extension_manager =
ExtensionManager::new_without_provider(temp_dir.path().to_path_buf());
extension_manager
.add_mock_extension("test_client".to_string(), Arc::new(MockClient {}))
.await;
let resolved = extension_manager
.resolve_tool("test-session-id", "test_client.tool")
.await
.expect("mangled dotted name should resolve to the real tool");
assert_eq!(resolved.tool_name, "test_client__tool");
}
#[tokio::test]
async fn test_resolve_tool_recovers_functions_prefixed_name() {
let temp_dir = tempfile::tempdir().unwrap();
let extension_manager =
ExtensionManager::new_without_provider(temp_dir.path().to_path_buf());
extension_manager
.add_mock_extension("test_client".to_string(), Arc::new(MockClient {}))
.await;
let resolved = extension_manager
.resolve_tool("test-session-id", "functions.test_client__tool")
.await
.expect("functions-prefixed name should resolve to the real tool");
assert_eq!(resolved.tool_name, "test_client__tool");
}
#[tokio::test]
async fn test_resolve_tool_exact_dotted_name_never_rewritten() {
let temp_dir = tempfile::tempdir().unwrap();
let extension_manager =
ExtensionManager::new_without_provider(temp_dir.path().to_path_buf());
extension_manager
.add_mock_extension("dotted".to_string(), Arc::new(MockDottedClient {}))
.await;
let resolved = extension_manager
.resolve_tool("test-session-id", "dotted__db.query")
.await
.expect("exact dotted tool name must resolve");
assert_eq!(resolved.tool_name, "dotted__db.query");
assert_eq!(resolved.actual_tool_name, "db.query");
}
#[tokio::test]
async fn test_resolve_tool_recovers_mangled_separator_with_dotted_tool_name() {
let temp_dir = tempfile::tempdir().unwrap();
let extension_manager =
ExtensionManager::new_without_provider(temp_dir.path().to_path_buf());
extension_manager
.add_mock_extension("dotted".to_string(), Arc::new(MockDottedClient {}))
.await;
let resolved = extension_manager
.resolve_tool("test-session-id", "dotted.db.query")
.await
.expect("mangled extension separator should resolve");
assert_eq!(resolved.tool_name, "dotted__db.query");
assert_eq!(resolved.actual_tool_name, "db.query");
}
#[test]
fn test_recover_mangled_tool_name() {
let tools = ["developer__shell", "platform__search"];
assert_eq!(
recover_mangled_tool_name("developer.shell", tools.iter().copied()).as_deref(),
Some("developer__shell")
);
assert_eq!(
recover_mangled_tool_name("functions.developer__shell", tools.iter().copied())
.as_deref(),
Some("developer__shell")
);
assert_eq!(
recover_mangled_tool_name("functions.developer.shell", tools.iter().copied())
.as_deref(),
Some("developer__shell")
);
assert_eq!(
recover_mangled_tool_name("developer shell", tools.iter().copied()),
None
);
assert_eq!(
recover_mangled_tool_name("developer__shell!", tools.iter().copied()),
None
);
assert_eq!(
recover_mangled_tool_name("nonexistent.tool", tools.iter().copied()),
None
);
let dotted_tool = ["dotted__db.query"];
assert_eq!(
recover_mangled_tool_name("dotted.db.query", dotted_tool.iter().copied()).as_deref(),
Some("dotted__db.query")
);
}
#[test]
fn test_remove_untrusted_mcp_app_meta_strips_spoofed_payload() {
let mut result = CallToolResult::success(vec![]);