From 73964353a47d66637d5917de83277f918b3420c6 Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Thu, 3 Apr 2025 14:22:59 -0300 Subject: [PATCH] agent: Handle tool use without text (#28030) ### Context The Anthropic API fails if a request message contains a tool use and no `Text` segments or it only contains empty `Text` segments. These are cases that the model itself produces, but the API doesn't support sending them back. #27917 fixed this by appending "Using tool..." in the thread's message, but this causes the actual conversation to include it, so it would appear in the UI (we would actually display a gap because we never rendered its markdown, but "Using tool..." would show up when the thread was restored). ### Solution We'll now only append this placeholder when we build the request, so the API still sees it, but the UI/Thread doesn't. Another issue we found is that the model starts mimicking these placeholders in later tool uses which is undesirable. So unfortunately, we had to add logic to filter them out. Release Notes: - agent: Improved rendering of tool uses without text --------- Co-authored-by: Bennet --- crates/agent/src/active_thread.rs | 426 +++++++++++++++--------------- crates/agent/src/thread.rs | 49 ++-- crates/agent/src/tool_use.rs | 22 ++ 3 files changed, 265 insertions(+), 232 deletions(-) diff --git a/crates/agent/src/active_thread.rs b/crates/agent/src/active_thread.rs index 7920b2450176915417c7cf02607732397ee915f1..6765710f77b865786ffd60caae432b03d1980428 100644 --- a/crates/agent/src/active_thread.rs +++ b/crates/agent/src/active_thread.rs @@ -1099,36 +1099,47 @@ impl ActiveThread { .into_any_element(), }; - let message_content = v_flex() - .gap_1p5() - .child( - if let Some(edit_message_editor) = edit_message_editor.clone() { - div() - .key_context("EditMessageEditor") - .on_action(cx.listener(Self::cancel_editing_message)) - .on_action(cx.listener(Self::confirm_editing_message)) - .min_h_6() - .child(edit_message_editor) - } else { - div() - .min_h_6() - .text_ui(cx) - .child(self.render_message_content( - message_id, - rendered_message, - has_tool_uses, - cx, - )) - }, - ) - .when(!context.is_empty(), |parent| { - parent.child( - h_flex() - .flex_wrap() - .gap_1() - .children(context.into_iter().map(|context| { - let context_id = context.id(); - ContextPill::added(AddedContext::new(context, cx), false, false, None) + let message_is_empty = message.should_display_content(); + let has_content = !message_is_empty || !context.is_empty(); + + let message_content = + has_content.then(|| { + v_flex() + .gap_1p5() + .when(!message_is_empty, |parent| { + parent.child( + if let Some(edit_message_editor) = edit_message_editor.clone() { + div() + .key_context("EditMessageEditor") + .on_action(cx.listener(Self::cancel_editing_message)) + .on_action(cx.listener(Self::confirm_editing_message)) + .min_h_6() + .child(edit_message_editor) + .into_any() + } else { + div() + .min_h_6() + .text_ui(cx) + .child(self.render_message_content( + message_id, + rendered_message, + has_tool_uses, + cx, + )) + .into_any() + }, + ) + }) + .when(!context.is_empty(), |parent| { + parent.child(h_flex().flex_wrap().gap_1().children( + context.into_iter().map(|context| { + let context_id = context.id(); + ContextPill::added( + AddedContext::new(context, cx), + false, + false, + None, + ) .on_click(Rc::new(cx.listener({ let workspace = workspace.clone(); let context_store = context_store.clone(); @@ -1145,8 +1156,9 @@ impl ActiveThread { } } }))) - })), - ) + }), + )) + }) }); let styled_message = match message.role { @@ -1261,7 +1273,7 @@ impl ActiveThread { ), ), ) - .child(div().p_2().child(message_content)), + .child(div().p_2().children(message_content)), ), Role::Assistant => v_flex() .id(("message-container", ix)) @@ -1270,7 +1282,9 @@ impl ActiveThread { .pr_4() .border_l_1() .border_color(cx.theme().colors().border_variant) - .child(message_content) + .children(message_content) + .gap_2p5() + .pb_2p5() .when(!tool_uses.is_empty(), |parent| { parent.child( v_flex().children( @@ -1284,7 +1298,7 @@ impl ActiveThread { v_flex() .bg(colors.editor_background) .rounded_sm() - .child(div().p_4().child(message_content)), + .child(div().p_4().children(message_content)), ), }; @@ -1816,7 +1830,7 @@ impl ActiveThread { div().map(|element| { if !tool_use.needs_confirmation { - element.py_2p5().child( + element.child( v_flex() .child( h_flex() @@ -1888,145 +1902,164 @@ impl ActiveThread { }), ) } else { - element.py_2().child( - v_flex() - .rounded_lg() - .border_1() - .border_color(self.tool_card_border_color(cx)) - .overflow_hidden() - .child( - h_flex() - .group("disclosure-header") - .relative() - .justify_between() - .py_1() - .map(|element| { - if is_status_finished { - element.pl_2().pr_0p5() - } else { - element.px_2() - } - }) - .bg(self.tool_card_header_bg(cx)) + v_flex() + .rounded_lg() + .border_1() + .border_color(self.tool_card_border_color(cx)) + .overflow_hidden() + .child( + h_flex() + .group("disclosure-header") + .relative() + .justify_between() + .py_1() + .map(|element| { + if is_status_finished { + element.pl_2().pr_0p5() + } else { + element.px_2() + } + }) + .bg(self.tool_card_header_bg(cx)) + .map(|element| { + if is_open { + element.border_b_1().rounded_t_md() + } else if needs_confirmation { + element.rounded_t_md() + } else { + element.rounded_md() + } + }) + .border_color(self.tool_card_border_color(cx)) + .child( + h_flex() + .id("tool-label-container") + .gap_1p5() + .max_w_full() + .overflow_x_scroll() + .child( + Icon::new(tool_use.icon) + .size(IconSize::XSmall) + .color(Color::Muted), + ) + .child( + h_flex().pr_8().text_ui_sm(cx).children( + self.rendered_tool_use_labels + .get(&tool_use.id) + .cloned(), + ), + ), + ) + .child( + h_flex() + .gap_1() + .child( + div().visible_on_hover("disclosure-header").child( + Disclosure::new("tool-use-disclosure", is_open) + .opened_icon(IconName::ChevronUp) + .closed_icon(IconName::ChevronDown) + .on_click(cx.listener({ + let tool_use_id = tool_use.id.clone(); + move |this, _event, _window, _cx| { + let is_open = this + .expanded_tool_uses + .entry(tool_use_id.clone()) + .or_insert(false); + + *is_open = !*is_open; + } + })), + ), + ) + .child(status_icons), + ) + .child(gradient_overlay(self.tool_card_header_bg(cx))), + ) + .map(|parent| { + if !is_open { + return parent; + } + + parent.child( + v_flex() + .bg(cx.theme().colors().editor_background) .map(|element| { - if is_open { - element.border_b_1().rounded_t_md() - } else if needs_confirmation { - element.rounded_t_md() + if needs_confirmation { + element.rounded_none() } else { - element.rounded_md() + element.rounded_b_lg() } }) + .child(results_content), + ) + }) + .when(needs_confirmation, |this| { + this.child( + h_flex() + .py_1() + .pl_2() + .pr_1() + .gap_1() + .justify_between() + .bg(cx.theme().colors().editor_background) + .border_t_1() .border_color(self.tool_card_border_color(cx)) + .rounded_b_lg() + .child(Label::new("Action Confirmation").color(Color::Muted).size(LabelSize::Small)) .child( h_flex() - .id("tool-label-container") - .gap_1p5() - .max_w_full() - .overflow_x_scroll() - .child( - Icon::new(tool_use.icon) - .size(IconSize::XSmall) - .color(Color::Muted), - ) - .child( - h_flex().pr_8().text_ui_sm(cx).children( - self.rendered_tool_use_labels - .get(&tool_use.id) - .cloned(), - ), - ), - ) - .child( - h_flex() - .gap_1() - .child( - div().visible_on_hover("disclosure-header").child( - Disclosure::new("tool-use-disclosure", is_open) - .opened_icon(IconName::ChevronUp) - .closed_icon(IconName::ChevronDown) - .on_click(cx.listener({ - let tool_use_id = tool_use.id.clone(); - move |this, _event, _window, _cx| { - let is_open = this - .expanded_tool_uses - .entry(tool_use_id.clone()) - .or_insert(false); - - *is_open = !*is_open; - } - })), - ), - ) - .child(status_icons), - ) - .child(gradient_overlay(self.tool_card_header_bg(cx))), - ) - .map(|parent| { - if !is_open { - return parent; - } - - parent.child( - v_flex() - .bg(cx.theme().colors().editor_background) - .map(|element| { - if needs_confirmation { - element.rounded_none() - } else { - element.rounded_b_lg() - } - }) - .child(results_content), - ) - }) - .when(needs_confirmation, |this| { - this.child( - h_flex() - .py_1() - .pl_2() - .pr_1() - .gap_1() - .justify_between() - .bg(cx.theme().colors().editor_background) - .border_t_1() - .border_color(self.tool_card_border_color(cx)) - .rounded_b_lg() - .child(Label::new("Action Confirmation").color(Color::Muted).size(LabelSize::Small)) - .child( - h_flex() - .gap_0p5() - .child({ - let tool_id = tool_use.id.clone(); - Button::new( - "always-allow-tool-action", - "Always Allow", + .gap_0p5() + .child({ + let tool_id = tool_use.id.clone(); + Button::new( + "always-allow-tool-action", + "Always Allow", + ) + .label_size(LabelSize::Small) + .icon(IconName::CheckDouble) + .icon_position(IconPosition::Start) + .icon_size(IconSize::Small) + .icon_color(Color::Success) + .tooltip(move |window, cx| { + Tooltip::with_meta( + "Never ask for permission", + None, + "Restore the original behavior in your Agent Panel settings", + window, + cx, ) + }) + .on_click(cx.listener( + move |this, event, window, cx| { + if let Some(fs) = fs.clone() { + update_settings_file::( + fs.clone(), + cx, + |settings, _| { + settings.set_always_allow_tool_actions(true); + }, + ); + } + this.handle_allow_tool( + tool_id.clone(), + event, + window, + cx, + ) + }, + )) + }) + .child(ui::Divider::vertical()) + .child({ + let tool_id = tool_use.id.clone(); + Button::new("allow-tool-action", "Allow") .label_size(LabelSize::Small) - .icon(IconName::CheckDouble) + .icon(IconName::Check) .icon_position(IconPosition::Start) .icon_size(IconSize::Small) .icon_color(Color::Success) - .tooltip(move |window, cx| { - Tooltip::with_meta( - "Never ask for permission", - None, - "Restore the original behavior in your Agent Panel settings", - window, - cx, - ) - }) .on_click(cx.listener( move |this, event, window, cx| { - if let Some(fs) = fs.clone() { - update_settings_file::( - fs.clone(), - cx, - |settings, _| { - settings.set_always_allow_tool_actions(true); - }, - ); - } this.handle_allow_tool( tool_id.clone(), event, @@ -2035,52 +2068,31 @@ impl ActiveThread { ) }, )) - }) - .child(ui::Divider::vertical()) - .child({ - let tool_id = tool_use.id.clone(); - Button::new("allow-tool-action", "Allow") - .label_size(LabelSize::Small) - .icon(IconName::Check) - .icon_position(IconPosition::Start) - .icon_size(IconSize::Small) - .icon_color(Color::Success) - .on_click(cx.listener( - move |this, event, window, cx| { - this.handle_allow_tool( - tool_id.clone(), - event, - window, - cx, - ) - }, - )) - }) - .child({ - let tool_id = tool_use.id.clone(); - let tool_name: Arc = tool_use.name.into(); - Button::new("deny-tool", "Deny") - .label_size(LabelSize::Small) - .icon(IconName::Close) - .icon_position(IconPosition::Start) - .icon_size(IconSize::Small) - .icon_color(Color::Error) - .on_click(cx.listener( - move |this, event, window, cx| { - this.handle_deny_tool( - tool_id.clone(), - tool_name.clone(), - event, - window, - cx, - ) - }, - )) - }), - ), - ) - }), - ) + }) + .child({ + let tool_id = tool_use.id.clone(); + let tool_name: Arc = tool_use.name.into(); + Button::new("deny-tool", "Deny") + .label_size(LabelSize::Small) + .icon(IconName::Close) + .icon_position(IconPosition::Start) + .icon_size(IconSize::Small) + .icon_color(Color::Error) + .on_click(cx.listener( + move |this, event, window, cx| { + this.handle_deny_tool( + tool_id.clone(), + tool_name.clone(), + event, + window, + cx, + ) + }, + )) + }), + ), + ) + }) } }) } diff --git a/crates/agent/src/thread.rs b/crates/agent/src/thread.rs index 9eecd01bcc94154a564cb6f292c2488ac6c287c1..9619cdb492b6e3dd8c9951523bd5e0b9ff38d3ab 100644 --- a/crates/agent/src/thread.rs +++ b/crates/agent/src/thread.rs @@ -35,7 +35,7 @@ use crate::thread_store::{ SerializedMessage, SerializedMessageSegment, SerializedThread, SerializedToolResult, SerializedToolUse, }; -use crate::tool_use::{PendingToolUse, ToolUse, ToolUseState}; +use crate::tool_use::{PendingToolUse, ToolUse, ToolUseState, USING_TOOL_MARKER}; #[derive(Debug, Clone, Copy)] pub enum RequestKind { @@ -85,6 +85,12 @@ pub struct Message { } impl Message { + /// Returns whether the message contains any meaningful text that should be displayed + /// The model sometimes runs tool without producing any text or just a marker ([`USING_TOOL_MARKER`]) + pub fn should_display_content(&self) -> bool { + self.segments.iter().all(|segment| segment.should_display()) + } + pub fn push_thinking(&mut self, text: &str) { if let Some(MessageSegment::Thinking(segment)) = self.segments.last_mut() { segment.push_str(text); @@ -131,6 +137,16 @@ impl MessageSegment { Self::Thinking(text) => text, } } + + pub fn should_display(&self) -> bool { + // We add USING_TOOL_MARKER when making a request that includes tool uses + // without non-whitespace text around them, and this can cause the model + // to mimic the pattern, so we consider those segments not displayable. + match self { + Self::Text(text) => text.is_empty() || text.trim() == USING_TOOL_MARKER, + Self::Thinking(text) => text.is_empty() || text.trim() == USING_TOOL_MARKER, + } + } } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -1083,32 +1099,15 @@ impl Thread { } } LanguageModelCompletionEvent::ToolUse(tool_use) => { - let last_assistant_message = thread + let last_assistant_message_id = thread .messages .iter_mut() - .rfind(|message| message.role == Role::Assistant); - - let last_assistant_message_id = - if let Some(message) = last_assistant_message { - if let Some(segment) = message.segments.first_mut() { - let text = segment.text_mut(); - if text.is_empty() { - text.push_str("Using tool..."); - } - } else { - message.segments.push(MessageSegment::Text( - "Using tool...".to_string(), - )); - } - - message.id - } else { - thread.insert_message( - Role::Assistant, - vec![MessageSegment::Text("Using tool...".to_string())], - cx, - ) - }; + .rfind(|message| message.role == Role::Assistant) + .map(|message| message.id) + .unwrap_or_else(|| { + thread.insert_message(Role::Assistant, vec![], cx) + }); + thread.tool_use.request_tool_use( last_assistant_message_id, tool_use, diff --git a/crates/agent/src/tool_use.rs b/crates/agent/src/tool_use.rs index 602a8ab5aef77e732b765ea49b40965295f488a1..03cabb0870cf3693fd5d2598722c88c9c62f5552 100644 --- a/crates/agent/src/tool_use.rs +++ b/crates/agent/src/tool_use.rs @@ -43,6 +43,8 @@ pub struct ToolUseState { pending_tool_uses_by_id: HashMap, } +pub const USING_TOOL_MARKER: &str = ""; + impl ToolUseState { pub fn new(tools: Arc) -> Self { Self { @@ -357,8 +359,28 @@ impl ToolUseState { request_message: &mut LanguageModelRequestMessage, ) { if let Some(tool_uses) = self.tool_uses_by_assistant_message.get(&message_id) { + let mut found_tool_use = false; + for tool_use in tool_uses { if self.tool_results.contains_key(&tool_use.id) { + if !found_tool_use { + // The API fails if a message contains a tool use without any (non-whitespace) text around it + match request_message.content.last_mut() { + Some(MessageContent::Text(txt)) => { + if txt.is_empty() { + txt.push_str(USING_TOOL_MARKER); + } + } + None | Some(_) => { + request_message + .content + .push(MessageContent::Text(USING_TOOL_MARKER.into())); + } + }; + } + + found_tool_use = true; + // Do not send tool uses until they are completed request_message .content