Skip to content

Conversation

@jamadeo
Copy link
Collaborator

@jamadeo jamadeo commented Nov 19, 2025

This is a good change to make but as implemented it calls .list_tools() on every token chunk response from a provider. .list_tools() is potentially very expensive, as it queries all extensions for their tools. This is especially noticeable if you have remote extensions, as they tend to have a higher latency.

reverts #5478

This reverts commit 78fd31a.

Copy link
Collaborator

@tlongwell-block tlongwell-block left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work instead?

diff --git a/crates/goose/src/agents/agent.rs b/crates/goose/src/agents/agent.rs
index 088fba588a7..d8a12c0ea34 100644
--- a/crates/goose/src/agents/agent.rs
+++ b/crates/goose/src/agents/agent.rs
@@ -283,11 +283,11 @@ impl Agent {
     async fn categorize_tools(
         &self,
         response: &Message,
-        _tools: &[rmcp::model::Tool],
+        tools: &[rmcp::model::Tool],
     ) -> ToolCategorizeResult {
         // Categorize tool requests
         let (frontend_requests, remaining_requests, filtered_response) =
-            self.categorize_tool_requests(response).await;
+            self.categorize_tool_requests(response, tools).await;
 
         ToolCategorizeResult {
             frontend_requests,
diff --git a/crates/goose/src/agents/reply_parts.rs b/crates/goose/src/agents/reply_parts.rs
index 40b1a99d6a8..9132364cb25 100644
--- a/crates/goose/src/agents/reply_parts.rs
+++ b/crates/goose/src/agents/reply_parts.rs
@@ -269,9 +269,8 @@ impl Agent {
     pub(crate) async fn categorize_tool_requests(
         &self,
         response: &Message,
+        tools: &[Tool],
     ) -> (Vec<ToolRequest>, Vec<ToolRequest>, Message) {
-        let tools = self.list_tools(None).await;
-
         // First collect all tool requests with coercion applied
         let tool_requests: Vec<ToolRequest> = response
             .content

@jamadeo
Copy link
Collaborator Author

jamadeo commented Dec 5, 2025

@tlongwell-block yes that would do it. The important thing to avoid is calls to list_tools() that query all MCP servers.

@jamadeo
Copy link
Collaborator Author

jamadeo commented Dec 16, 2025

@tlongwell-block did you end up making the above change? if so we can close this PR, if not let's merge this if it's still an active issue because it could be quite bad for anyone connected to remote MCPs

@tlongwell-block
Copy link
Collaborator

@tlongwell-block did you end up making the above change? if so we can close this PR, if not let's merge this if it's still an active issue because it could be quite bad for anyone connected to remote MCPs

I can open that PR today. I'll link it here when its ready

@alexhancock
Copy link
Collaborator

@jamadeo @tlongwell-block Is this one still relevant, or replaced with another PR?

@tlongwell-block
Copy link
Collaborator

@alexhancock @jamadeo #6138 should fix this issue

@jamadeo jamadeo closed this Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants