-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Platform extensions sketch #4868
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
Conversation
| version: "1.0.0".to_string(), | ||
| }, | ||
| instructions: Some( | ||
| "Manage TODO lists - read and write task planning content.".to_string(), |
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.
we should expand this of course
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.
so this replaces the prompt for existing todos? (or it is still in system prompt?). Existing todos seem to think they are a file which always looks odd, so probably good to play around with this
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.
yes, will be. currently todo read & write are tools without an extension, which means there is no extension prompt which means we include it in the system prompt which means that if you switch off todo, it still knows about it. so that's one of the things we're trying to fix here
| "Frontend tool execution required".to_string(), | ||
| None, | ||
| ))) | ||
| } else if tool_call.name == TODO_READ_TOOL_NAME { |
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.
nice
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.
yeah this is the best bit!
| Tool::new( | ||
| TODO_READ_TOOL_NAME.to_string(), | ||
| indoc! {r#" | ||
| Read the entire TODO file content. |
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.
Have these tool instructions been ported somewhere else?
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.
they have now
| .unwrap_or_else(|_| HashMap::new())) | ||
| .unwrap_or_else(|_| HashMap::new()); | ||
|
|
||
| if !extensions_map.is_empty() { |
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.
Why do we only enable the platform extensions if the user has existing extensions?
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.
good question; I saw this happening where it would get here and not have any extensions yet and then if we insert it here, something else is not going to set their defaults. obviously would be better to solve the issue in that something else, but that feels like it is inside some hairy client code. we should get a good opinion though on where in general the default extensions are set. I'm not sure !
|
|
||
| // If this is a Stdio extension that uses npx, check for Node.js installation | ||
| #[cfg(target_os = "windows")] | ||
| if let ExtensionConfigRequest::Stdio { cmd, .. } = &extension_request { |
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 this compile? here's referring to ExtensionConfigRequest::stdio which has been deleted
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.
ooh, good catch. probably no longer compiles on windows. it does beg the question what this windows specific code is doing here and why it is installing a specific version of node, but still.
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.
fixed
| vec![ | ||
| Tool { | ||
| name: "todo_read".into(), | ||
| description: Some("Read the current TODO list content".into()), |
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.
description needs to be expanded. Same for the write tool below.
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.
this is a sketch - the idea is to look at the architecture
| } else { | ||
| None | ||
| }; | ||
| let description = cliclack::input("Enter a description for this extension:") |
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.
so descriptions are required now - but not often used, how come mandatory now effectively?
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.
well, we were using them all over the place, just for when we didn't have them we would try to put something together ourselves. this is just an effort to make it all look a bit more like each other.
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.
that is ok - just wanted to make sure deliberate and won't cause problems loading older configurations
| let mut map = HashMap::new(); | ||
|
|
||
| map.insert( | ||
| todo_extension::EXTENSION_NAME, |
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.
much nicer looking
|
LGTM - when ready/builds any thing to manually test and watch out for @DOsinga ? |
| } | ||
|
|
||
| vec![ | ||
| 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.
Can do Tool::new for a bit nicer syntax if we want
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.
oh nice
| impl TodoClient { | ||
| pub fn new(context: PlatformExtensionContext) -> Result<Self> { | ||
| let info = InitializeResult { | ||
| protocol_version: ProtocolVersion::V_2025_03_26, |
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.
if there is a constant for the june version we can start using it. i was going to take a pass and make that the default in rmcp. we more closely resemble the june version than march.
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.
sounds good. of course this is internal only so for now I doubt we'll check it, bit still
jamadeo
left a comment
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.
Nice. I think passing the context to a per-session client is great, and opens these up to be really quite powerful.
I'm fine with description being non-optional in general, but I don't think users should have to create a description when adding an MCP server. (e.g. I go find the github mcp, add it, I should not have to also type "github" in a description field.)
|
|
||
| #[async_trait] | ||
| impl McpClientTrait for TodoClient { | ||
| async fn list_resources( |
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.
if these are going to act like mcp servers, it would be really nice to be able to define them using rmcp's server sdk, e.g.
impl Todo {
#[tool(name = "todo_add")]
pub async fn todo_add(...)
}
they could still be built with the platform extension context and have all the same access, but they'd be easier to write and easily port-able to out-of-process MCP servers
michaelneale
left a comment
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.
isn't picking up built in extensions or configured extensions (see comment)
|
@DOsinga still the same - not showing any extensions or finding them, and have pulled latest
no idea why - but any other branch is fine, it is just this one |
|
ok testing with production build |
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.
ok think have hammered on this enough - cli does work now - so I am happy (weird how it loaded different - I guess longer term that code path will be replaced with goosed so all good).
I also note that todo now shows up an extension you can turn off, but it is not in the config.yaml - which means you can't "add it" later on - is this an issue? (maybe it will always just show up in any list which is fine and ideal). Again - cli only.
(openapi schema isn't happy though - but when build is ok I think this makes sense)
yeah, good analysis. todo always shows up, as it is a platform tool, so all you can do is switch it off. if you delete it from the config, we add it again. I think this is how all bundled tools should work? not sure. also not sure on how it works now and when they are re-added (i.e. like with developer tool, the desktop will adde them if deleted?). might need to do a follow up to clean this up |
* main: Removed unused libs (block#4932) Platform extensions sketch (block#4868) Add reply to the routes (block#4963) feat(cli): add GOOSE_DEBUG environment variable support (block#4825)
* main: (22 commits) fix: Issue #4540: `goose configure` -> Cursor Agent succeeds (#4942) feat: Add advanced data analysis pipeline recipe (#4990) (#5005) Create / edit recipe form unification and improvements (#4693) feat: add Code Review Mentor recipe with Developer and Memory extensions (#4992) (#5014) feat: set custom models for lead/worker (#4598) feat: add grok-code-fast-1 support for xAI provider (#4472) Persist dynamic extension config so we can resume recipe sessions w/ extensions (#4331) fix: show PowerShell PATH instructions for Windows users (#4989) feat: add Smart Task Organizer recipe for Hacktoberfest (#4936) Fix extension headers (#5000) feat: add advanced software project generator initializer recipe (#4767) (#4949) Removed unused libs (#4932) Platform extensions sketch (#4868) Add reply to the routes (#4963) feat(cli): add GOOSE_DEBUG environment variable support (#4825) docs: Change community page sections (#4984) docs: remove temporary Hacktoberfest issue templates (#4982) Create multi-channel researcher prompt (#4947) docs: Add Community Content section to Community Page (#4964) Allow empty API Key when registering custom provider (#4977) ...
Co-authored-by: Douwe Osinga <[email protected]> Co-authored-by: Michael Neale <[email protected]> Signed-off-by: Itz-Agasta <[email protected]>
The todo extension added in block#4868 causes errors with bedrock because bedrock expects an input schema. Without this fix, bedrock tool calls fail with Error: Server error: Failed to call Bedrock: ValidationException(ValidationException { message: Some("The value at toolConfig.tools.3.toolSpec.inputSchema.json.type must be one of the following: object."), meta: ErrorMetadata { code: Some("ValidationException"), message: Some("The value at toolConfig.tools.3.toolSpec.inputSchema.json.type must be one of the following: object."), extras: Some({"aws_request_id": "9813e4c5-607f-47fc-8a6a-4387ef28acb7"}) } })
The todo extension added in block#4868 causes errors with bedrock because bedrock expects an input schema. Without this fix, bedrock tool calls fail with Error: Server error: Failed to call Bedrock: ValidationException(ValidationException { message: Some("The value at toolConfig.tools.3.toolSpec.inputSchema.json.type must be one of the following: object."), meta: ErrorMetadata { code: Some("ValidationException"), message: Some("The value at toolConfig.tools.3.toolSpec.inputSchema.json.type must be one of the following: object."), extras: Some({"aws_request_id": "9813e4c5-607f-47fc-8a6a-4387ef28acb7"}) } }) Signed-off-by: alexyao2015 <[email protected]>




Platform Extensions
This introduces a new type of extension, platform extensions. these are extensions that are tied tightly to the platform and can (currently) read and write to the current session.
It also makes description a required field on the extensions for consistency