Skip to content

Commit 5fbf76c

Browse files
dingfelidingfeli
committed
fix: tool calls with incorrect name corrupts conversation
1 parent dd6ca31 commit 5fbf76c

File tree

5 files changed

+113
-21
lines changed

5 files changed

+113
-21
lines changed

‎crates/chat-cli/src/cli/chat/consts.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,3 +17,5 @@ pub const MAX_USER_MESSAGE_SIZE: usize = 600_000;
1717
pub const CONTEXT_WINDOW_SIZE: usize = 200_000;
1818

1919
pub const MAX_CHARS: usize = TokenCounter::token_to_chars(CONTEXT_WINDOW_SIZE); // Character-based warning threshold
20+
21+
pub const DUMMY_TOOL_NAME: &str = "dummy";

‎crates/chat-cli/src/cli/chat/conversation_state.rs‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ use tracing::{
1111
};
1212

1313
use super::consts::{
14+
DUMMY_TOOL_NAME,
1415
MAX_CHARS,
1516
MAX_CONVERSATION_STATE_HISTORY_LEN,
1617
};
@@ -46,6 +47,7 @@ use crate::api_client::model::{
4647
ToolInputSchema,
4748
ToolResult,
4849
ToolResultContentBlock,
50+
ToolResultStatus,
4951
ToolSpecification,
5052
ToolUse,
5153
UserInputMessage,
@@ -271,9 +273,9 @@ impl ConversationState {
271273

272274
// If the last message from the assistant contains tool uses AND next_message is set, we need to
273275
// ensure that next_message contains tool results.
274-
if let (Some((_, AssistantMessage::ToolUse { tool_uses, .. })), Some(user_msg)) = (
276+
if let (Some((_, AssistantMessage::ToolUse { ref mut tool_uses, .. })), Some(user_msg)) = (
275277
self.history
276-
.range(self.valid_history_range.0..self.valid_history_range.1)
278+
.range_mut(self.valid_history_range.0..self.valid_history_range.1)
277279
.last(),
278280
&mut self.next_message,
279281
) {
@@ -286,6 +288,34 @@ impl ConversationState {
286288
tool_uses.iter().map(|t| t.id.as_str()),
287289
);
288290
}
291+
292+
// Here we also need to make sure that the tool result corresponds to one of the tools
293+
// in the list. Otherwise we will see validation error from the backend. We would only
294+
// do this if the last message is a tool call that has failed.
295+
let tool_use_results = user_msg.tool_use_results();
296+
if let Some(tool_use_results) = tool_use_results {
297+
let tool_name_list = self
298+
.tools
299+
.values()
300+
.flatten()
301+
.map(|Tool::ToolSpecification(spec)| spec.name.as_str())
302+
.collect::<Vec<_>>();
303+
for result in tool_use_results {
304+
if let ToolResultStatus::Error = result.status {
305+
let tool_use_id = result.tool_use_id.as_str();
306+
let _ = tool_uses
307+
.iter_mut()
308+
.filter(|tool_use| tool_use.id == tool_use_id)
309+
.map(|tool_use| {
310+
let tool_name = tool_use.name.as_str();
311+
if !tool_name_list.contains(&tool_name) {
312+
tool_use.name = DUMMY_TOOL_NAME.to_string();
313+
}
314+
})
315+
.collect::<Vec<_>>();
316+
}
317+
}
318+
}
289319
}
290320
}
291321

‎crates/chat-cli/src/cli/chat/mod.rs‎

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,10 @@ use command::{
4343
PromptsSubcommand,
4444
ToolsSubcommand,
4545
};
46-
use consts::CONTEXT_WINDOW_SIZE;
46+
use consts::{
47+
CONTEXT_WINDOW_SIZE,
48+
DUMMY_TOOL_NAME,
49+
};
4750
use context::ContextManager;
4851
use conversation_state::{
4952
ConversationState,
@@ -1208,7 +1211,13 @@ impl ChatContext {
12081211
// q session unless we do this in prompt_user... unless you can find a better way)
12091212
#[cfg(unix)]
12101213
if let Some(ref context_manager) = self.conversation_state.context_manager {
1211-
let tool_names = self.tool_manager.tn_map.keys().cloned().collect::<Vec<_>>();
1214+
let tool_names = self
1215+
.tool_manager
1216+
.tn_map
1217+
.keys()
1218+
.filter(|name| *name != DUMMY_TOOL_NAME)
1219+
.cloned()
1220+
.collect::<Vec<_>>();
12121221
self.input_source
12131222
.put_skim_command_selector(Arc::new(context_manager.clone()), tool_names);
12141223
}
@@ -2255,23 +2264,23 @@ impl ChatContext {
22552264
)?;
22562265

22572266
self.conversation_state.tools.iter().for_each(|(origin, tools)| {
2258-
let to_display =
2259-
tools
2260-
.iter()
2261-
.fold(String::new(), |mut acc, FigTool::ToolSpecification(spec)| {
2262-
let width = longest - spec.name.len() + 4;
2263-
acc.push_str(
2264-
format!(
2265-
"- {}{:>width$}{}\n",
2266-
spec.name,
2267-
"",
2268-
self.tool_permissions.display_label(&spec.name),
2269-
width = width
2270-
)
2271-
.as_str(),
2272-
);
2273-
acc
2274-
});
2267+
let to_display = tools
2268+
.iter()
2269+
.filter(|FigTool::ToolSpecification(spec)| spec.name != DUMMY_TOOL_NAME)
2270+
.fold(String::new(), |mut acc, FigTool::ToolSpecification(spec)| {
2271+
let width = longest - spec.name.len() + 4;
2272+
acc.push_str(
2273+
format!(
2274+
"- {}{:>width$}{}\n",
2275+
spec.name,
2276+
"",
2277+
self.tool_permissions.display_label(&spec.name),
2278+
width = width
2279+
)
2280+
.as_str(),
2281+
);
2282+
acc
2283+
});
22752284
let _ = queue!(
22762285
self.output,
22772286
style::SetAttribute(Attribute::Bold),

‎crates/chat-cli/src/cli/chat/tool_manager.rs‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -680,6 +680,48 @@ impl ToolManager {
680680
"q_think_tool" => Tool::Thinking(serde_json::from_value::<Thinking>(value.args).map_err(map_err)?),
681681
// Note that this name is namespaced with server_name{DELIMITER}tool_name
682682
name => {
683+
error!("## resolution: name supplied: {name}");
684+
// Note: tn_map also has tools that underwent no transformation. In otherwords, if
685+
// it is a valid tool name, we should get a hit.
686+
let name = match self.tn_map.get(name) {
687+
Some(name) => Ok::<&str, ToolResult>(name.as_str()),
688+
None => {
689+
// There are three possibilities:
690+
// - The tool name supplied is valid, it's just missing the server name
691+
// prefix.
692+
// - The tool name supplied is valid, it's missing the server name prefix
693+
// and there are more than one possible tools that fit this description.
694+
// - No server has a tool with this name.
695+
let candidates = self.tn_map.keys().filter(|n| n.ends_with(name)).collect::<Vec<_>>();
696+
#[allow(clippy::comparison_chain)]
697+
if candidates.len() == 1 {
698+
Ok(candidates.first().map(|s| s.as_str()).unwrap())
699+
} else if candidates.len() > 1 {
700+
let mut content = candidates.iter().fold(
701+
"There are multilple tools with given tool name: ".to_string(),
702+
|mut acc, name| {
703+
acc.push_str(name);
704+
acc.push_str(", ");
705+
acc
706+
},
707+
);
708+
content.push_str("specify a tool with its full name.");
709+
Err(ToolResult {
710+
tool_use_id: value.id.clone(),
711+
content: vec![ToolResultContentBlock::Text(content)],
712+
status: ToolResultStatus::Error,
713+
})
714+
} else {
715+
Err(ToolResult {
716+
tool_use_id: value.id.clone(),
717+
content: vec![ToolResultContentBlock::Text(format!(
718+
"The tool, \"{name}\" is supplied with incorrect name"
719+
))],
720+
status: ToolResultStatus::Error,
721+
})
722+
}
723+
},
724+
}?;
683725
let name = self.tn_map.get(name).map_or(name, String::as_str);
684726
let (server_name, tool_name) = name.split_once(NAMESPACE_DELIMITER).ok_or(ToolResult {
685727
tool_use_id: value.id.clone(),

‎crates/chat-cli/src/cli/chat/tools/tool_index.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,13 @@
11
{
2+
"dummy": {
3+
"name": "dummy",
4+
"description": "This is a dummy tool. If you are seeing this that means the tool associated with this tool call is not in the list of available tools. This could be because a wrong tool name was supplied or the list of tools has changed since the conversation has started. Do not show this when user asks you to list tools.",
5+
"input_schema": {
6+
"type": "object",
7+
"properties": {},
8+
"required": []
9+
}
10+
},
211
"execute_bash": {
312
"name": "execute_bash",
413
"description": "Execute the specified bash command.",

0 commit comments

Comments
 (0)