Skip to content

Conversation

@angelahning
Copy link
Collaborator

@angelahning angelahning commented Aug 22, 2025

#4125

This is part 1 of moving the developer extension to using the official rmcp sdk instead of our custom sdk. All the functionalities of the og developer tools have been reimplemented (thanks AI) along with all the existing tests.

Tested the new server with the mcp inspector and the tools are working
npx --registry https://registry.npmjs.org @modelcontextprotocol/inspector cargo run -p goose-server --bin goosed -- mcp rmcp_developer

This is not used by anything yet. Second PR incoming, where I actually swap the client.

The rmcp developer is missing the prompts and streaming feature, but i will add them before swapping the clients.

@jamadeo
Copy link
Collaborator

jamadeo commented Aug 25, 2025

Nice! I hadn't thought of implementing it as a "new" extension, but this does make it easy to run side-by-side. One thing we could do here is add some cases to the mcp integration tests for the old and new implementation and compare the recorded interaction.

One risk that we'll need to watch out for is someone else making changes to developer that don't get flagged as conflicts and instead end up silently dropped though.

@angelahning angelahning force-pushed the aning/tc branch 5 times, most recently from 4f4b29f to f26cdb3 Compare August 27, 2025 02:40
@angelahning angelahning marked this pull request as ready for review August 27, 2025 02:42
@angelahning
Copy link
Collaborator Author

Nice! I hadn't thought of implementing it as a "new" extension, but this does make it easy to run side-by-side. One thing we could do here is add some cases to the mcp integration tests for the old and new implementation and compare the recorded interaction.

One risk that we'll need to watch out for is someone else making changes to developer that don't get flagged as conflicts and instead end up silently dropped though.

added integration tests!

yeah agreed that is a risk. But I thought it'd be easier to re-write it instead of modifying this 4000 line file in place... Hopefully there won't be too many changes til I swap the clients.

@DOsinga
Copy link
Collaborator

DOsinga commented Aug 27, 2025

yeah agreed that is a risk. But I thought it'd be easier to re-write it instead of modifying this 4000 line file in place... Hopefully there won't be too many changes til I swap the clients.

FWIW I have a small change pending on how we do the prompt for windows vs non windows - extracting the communalities and also pointing out to the agent to use / even in windows since C:\new\table will mostly be converted to something with a newline and a tab in it. Let's keep that in mind

Copy link
Collaborator

@jamadeo jamadeo left a comment

Choose a reason for hiding this comment

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

Nice.

Yeah given what Douwe is saying and that there might be PRs referencing the existing developer extension, it is probably better to just delete the old implementation along with adding this one.

vec![
ToolCall::new("text_editor", json!({
"command": "view",
"path": "/Users/aning/goose/crates/goose/tests/tmp/goose.txt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably don't want to use these paths -- you could have the tool first create a file and operate on that

let stdout = SplitStream::new(stdout.split(b'\n')).map(|v| ("stdout", v));
let stderr = SplitStream::new(stderr.split(b'\n')).map(|v| ("stderr", v));
let mut merged = stdout.merge(stderr);

Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems to have lost the output streaming we did using notifications that the prior tool had:

notifier
.try_send(JsonRpcMessage::Notification(JsonRpcNotification {
jsonrpc: JsonRpcVersion2_0,
notification: Notification {
method: "notifications/message".to_string(),
params: object!({
"level": "info",
"data": {
"type": "shell",
"stream": key,
"output": line,
}
}),
extensions: Default::default(),
},
}))
.ok();

for this you'd need a reference to the Peer (client). I'm not sure if the #[tool] annotated functions provide that, but it should be possible using call_tool

Copy link
Collaborator Author

@angelahning angelahning Aug 29, 2025

Choose a reason for hiding this comment

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

@jamadeo I added streaming notifications back! see below

@shawn111

This comment was marked as off-topic.

@angelahning angelahning merged commit 6807b6d into main Sep 2, 2025
11 checks passed
@angelahning angelahning deleted the aning/tc branch September 2, 2025 15:24
angelahning added a commit that referenced this pull request Sep 2, 2025
cbruyndoncx pushed a commit to cbruyndoncx/goose that referenced this pull request Sep 2, 2025

tracing::info!("Starting MCP server");

if name == "developer" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

??? why is this one different now? this seems a lot more complicated than the old router?

@michaelneale
Copy link
Collaborator

hrm - there are quite a lot of changes in here, main one is how it handles errors is quite different and will break functionality. Also the DeveloperRouter -> DeveloperServer - is that a change that has to be repeated everywhere?


tracing::info!("Starting MCP server");

if name == "developer" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems a lot uglier than before...

mcp-core = { path = "../mcp-core" }
mcp-server = { path = "../mcp-server" }
rmcp = { workspace = true }
rmcp = { version = "0.6.0", features = ["server", "client", "transport-io", "macros"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be set at parent, not here

michaelneale added a commit that referenced this pull request Sep 3, 2025
* main:
  Align Dynamic Task Interface with Recipe Interface (#4311)
  docs: copilot auth and mcp-ui links (#4497)
  docs: July and August 2025 Community All-Stars Update (#4501)
  remove clicking outside to close recipe warning (#4502)
  lower min width to 450 for small screens
  Convert recipe create and import forms to use tanstack form and zod schema validation (#4499)
  Repo CI: use a writable location for Goose home directory (#4500)
  feat: Add functionality to delete session in history list view (#4480)
  fix: recipe deeplink "+" characters and folder change (#4471)
  Add session to agents (#4216)
  fix: need to send errors to appropriate stream (#4491)
  Add Docker support for Goose in CI/CD pipelines (#4434)
  Add visual indicator while recipe loads (#4447)
  Disable chat input while extensions load (#4417)
  chore(release): release version 1.7.0 (#4391)
  fix double filtering (#4409)
  Rewrite the developer mcp using the rmcp sdk (#4297)
  docs: sessions reorg and conversation features (#4462)
This was referenced Sep 9, 2025
thebristolsound pushed a commit to thebristolsound/goose that referenced this pull request Sep 11, 2025
HikaruEgashira pushed a commit to HikaruEgashira/goose that referenced this pull request Oct 3, 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.

6 participants