agent: Do not add `<using_tool>` placeholder (#29194)

Agus Zubiaga created 8 months ago

Our provider code in `language_models` filters out messages for which
`LanguageModelRequestMessage::contents_empty` returns `false`. This
doesn't seem wrong by itself, but `contents_empty` was returning `false`
for messages whose first segment didn't contain non-whitespace text even
if they contained other non-empty segments. This caused requests to fail
when a message with a tool call didn't contain any preceding text.

Release Notes:

- N/A

Change summary

crates/agent/src/thread.rs           |  9 +++------
crates/agent/src/tool_use.rs         | 22 ----------------------
crates/language_model/src/request.rs | 27 ++++++++++-----------------
3 files changed, 13 insertions(+), 45 deletions(-)

Detailed changes

crates/agent/src/thread.rs

@@ -38,7 +38,7 @@ use crate::thread_store::{
     SerializedMessage, SerializedMessageSegment, SerializedThread, SerializedToolResult,
     SerializedToolUse, SharedProjectContext,
 };
-use crate::tool_use::{PendingToolUse, ToolUse, ToolUseMetadata, ToolUseState, USING_TOOL_MARKER};
+use crate::tool_use::{PendingToolUse, ToolUse, ToolUseMetadata, ToolUseState};
 
 #[derive(
     Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Serialize, Deserialize, JsonSchema,
@@ -167,12 +167,9 @@ pub enum MessageSegment {
 
 impl MessageSegment {
     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,
+            Self::Text(text) => text.is_empty(),
+            Self::Thinking { text, .. } => text.is_empty(),
             Self::RedactedThinking(_) => false,
         }
     }

crates/agent/src/tool_use.rs

@@ -27,8 +27,6 @@ pub struct ToolUse {
     pub needs_confirmation: bool,
 }
 
-pub const USING_TOOL_MARKER: &str = "<using_tool>";
-
 pub struct ToolUseState {
     tools: Entity<ToolWorkingSet>,
     tool_uses_by_assistant_message: HashMap<MessageId, Vec<LanguageModelToolUse>>,
@@ -452,28 +450,8 @@ 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

crates/language_model/src/request.rs

@@ -221,23 +221,16 @@ impl LanguageModelRequestMessage {
     }
 
     pub fn contents_empty(&self) -> bool {
-        self.content.is_empty()
-            || self
-                .content
-                .first()
-                .map(|content| match content {
-                    MessageContent::Text(text) => text.chars().all(|c| c.is_whitespace()),
-                    MessageContent::Thinking { text, .. } => {
-                        text.chars().all(|c| c.is_whitespace())
-                    }
-                    MessageContent::ToolResult(tool_result) => {
-                        tool_result.content.chars().all(|c| c.is_whitespace())
-                    }
-                    MessageContent::RedactedThinking(_)
-                    | MessageContent::ToolUse(_)
-                    | MessageContent::Image(_) => true,
-                })
-                .unwrap_or(false)
+        self.content.iter().all(|content| match content {
+            MessageContent::Text(text) => text.chars().all(|c| c.is_whitespace()),
+            MessageContent::Thinking { text, .. } => text.chars().all(|c| c.is_whitespace()),
+            MessageContent::ToolResult(tool_result) => {
+                tool_result.content.chars().all(|c| c.is_whitespace())
+            }
+            MessageContent::RedactedThinking(_)
+            | MessageContent::ToolUse(_)
+            | MessageContent::Image(_) => false,
+        })
     }
 }