-
Notifications
You must be signed in to change notification settings - Fork 604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: adds default tools that can use MCP resources #619
Conversation
salman1993
commented
Jan 16, 2025
•
edited
Loading
edited
- adds 2 default "platform" tools to list and read MCP resources
- dynamically added when any of the connected extensions have resource capabilities
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
f936f67
to
06388d2
Compare
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
Desktop App for this PRThe following build is available for testing: The app is signed and notarized for macOS. After downloading, unzip the file and drag the Goose.app to your Applications folder. This link is provided by nightly.link and will work even if you're not logged into GitHub. |
crates/goose/src/prompts/system.md
Outdated
The format of a tool is "{system_name}__{tool_name}", i.e. the system | ||
name followed by the tool name with '__' as the separator. | ||
|
||
By default, we add a tool called "platform__read_resource", which can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can briefly describe platform__list_resources
here
this looks good to me but i think we should test this out a bit more with MCP servers that have resources, after merging in #636 curious how you've found this so far for gdrive? |
only a tiny useful for gdrive imo, the list returns the last 10 most recently viewed files. depending on how other mcp servers implement resources, since we don't do pagination could be hit or miss |
c52b367
to
03fe456
Compare
lgtm - github won't let me approve it since i am a co-author. @baxen can you do a quick skim on this one? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I would do the conditional enable though if possible
crates/goose/src/agents/reference.rs
Outdated
}), | ||
); | ||
|
||
tools.push(read_resource_tool); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would conditionally append these tools only if one of the systems has a resource capability. I'd also update the tooltip to include which systems have resources
crates/goose/src/prompts/system.md
Outdated
@@ -10,6 +10,14 @@ to interactions with sytems that are not currently active. The currently | |||
active systems are below. Each of these systems provides tools that are | |||
in your tool specification. | |||
|
|||
The format of a tool is "{system_name}__{tool_name}", i.e. the system |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i think these additions are excludable? not sure if you saw any performance issues with this but i have not seen any problems or a need to warn it about tools beyond the tool descriptions
// If system name is not provided, we need to search for the resource across all systems | ||
// Loop through each system and try to read the resource, don't raise an error if the resource is not found | ||
for system_name in self.clients.keys() { | ||
let result = self.read_resource_from_system(uri, system_name).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: either make a note that we assume no risk of collision for generic names or ensure num_matches == 1 before proceeding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm good point, if two systems have the same resource uri, it would find the first match and exit the loop
i'll put a TODO comment, not sure if we'd want to find the uri across all systems or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one interesting follow up idea would be to append some metadata about the resources like token count or file size and make that transparent to the user
developer2 only returns an Vec::new() for list_resources anyway, we should just report we don't support resources
@baxen added the conditional adding of the tools, I removed the |
4b1425f
to
00b4203
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Let's try this out with some real mcp servers before merging i think
let mut futures = FuturesUnordered::new(); | ||
|
||
// Create futures for each system | ||
for (system_name, client) in &self.clients { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can skip some steps by iterating over self.resource_capable_systems
instead here now?
// Loop through each system and try to read the resource, don't raise an error if the resource is not found | ||
// TODO: do we want to find if a provided uri is in multiple systems? | ||
// currently it will reutrn the first match and skip any systems | ||
for system_name in self.clients.keys() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can skip some steps by searching only self.resource_capable_system (like below)
crates/goose/src/prompts/system.md
Outdated
@@ -10,12 +10,21 @@ to interactions with sytems that are not currently active. The currently | |||
active systems are below. Each of these systems provides tools that are | |||
in your tool specification. | |||
|
|||
When a system is added that supports Resources,, we add two tools, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i think we can just delete this entirely. the tool descriptions and the below hint per system should cover this i think!
crates/goose/src/agents/reference.rs
Outdated
resource URI in the provided system, and reads in the resource content. If no system | ||
is provided, the tool will search all systems for the resource. | ||
|
||
The read_resource tool is typically used with a search query (can be before or after). Here are two examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i'd remove these extra tips unless we find they are needed tips? not sure if it was tested. i've been noticing these specific examples don't necessarily make it better at using the tools but can confuse it into hallucinating usage that isn't relevant, with frontier models at least
crates/goose/src/agents/reference.rs
Outdated
in the provided system, and returns a list for the user to browse. If no system | ||
is provided, the tool will search all systems for the resource. | ||
|
||
The list_resources tool is typically used before a read_resource tool call. Here are two examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: i'd remove these extra tips unless we find they are needed tips? not sure if it was tested. i've been noticing these specific examples don't necessarily make it better at using the tools but can confuse it into hallucinating usage that isn't relevant, with frontier models at least
reuse list_resources_from_system instead of reimplmenting it for iterating over all systems
crates/goose/src/prompts/system.md
Outdated
@@ -15,7 +15,10 @@ in your tool specification. | |||
|
|||
## {{system.name}} | |||
{{system.description}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the description get populated anywhere? i think we can remove this
* adds 2 default "platform" tools to list and read MCP resources * dynamically added when any of the connected extensions have resource capabilities Co-authored-by: Kalvin Chau <[email protected]>