Skip to content

Conversation

@alexhancock
Copy link
Collaborator

@alexhancock alexhancock commented Sep 2, 2025

Remove the mcp-server crate, and update all internally implemented mcp servers to use https://github.com/modelcontextprotocol/rust-sdk's way of doing things

Following up on @angelahning's excellent change for the developer server here #4297. It's just more of the same!

Also removes crates/mcp-server

@michaelneale
Copy link
Collaborator

I think want to chase down regressions that the other crate seems to have brought first

@alexhancock alexhancock force-pushed the alexhancock/rmcp-server-conversions branch 4 times, most recently from c5105b7 to d739415 Compare September 5, 2025 16:39
@alexhancock alexhancock marked this pull request as draft September 5, 2025 16:39
@alexhancock alexhancock force-pushed the alexhancock/rmcp-server-conversions branch from 171fbfa to a448c1a Compare September 5, 2025 17:10
@alexhancock
Copy link
Collaborator Author

Need to bring back the input schemas in Autovisualizer, and look for any other issues before merge

@alexhancock alexhancock force-pushed the alexhancock/rmcp-server-conversions branch 3 times, most recently from 885666d to 0cf43d1 Compare September 8, 2025 17:59
@@ -1,23 +0,0 @@
[package]
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete delete delete 💯

@alexhancock alexhancock marked this pull request as ready for review September 8, 2025 18:50
@@ -1,426 +1,132 @@
use base64::Engine;
use etcetera::{choose_app_strategy, AppStrategy};
Copy link
Collaborator

Choose a reason for hiding this comment

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

computercontroller looks good

Copy link
Collaborator

@angelahning angelahning Sep 8, 2025

Choose a reason for hiding this comment

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

Tested on UI:

  • play classical music in safari, worked
  • max screen brightness, didnt work on the released branch nor this

@@ -1,129 +1,122 @@
use async_trait::async_trait;
use etcetera::{choose_app_strategy, AppStrategy};
Copy link
Collaborator

Choose a reason for hiding this comment

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

memory looks good

Copy link
Collaborator

Choose a reason for hiding this comment

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

tesed on ui

@@ -1,57 +1,59 @@
use anyhow::Result;
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

use base64::{engine::general_purpose::STANDARD, Engine as _};
use etcetera::{choose_app_strategy, AppStrategy};
use indoc::{formatdoc, indoc};
use indoc::formatdoc;
Copy link
Collaborator

@angelahning angelahning Sep 8, 2025

Choose a reason for hiding this comment

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

Not sure if it is an regression.
Given this prompt

1. The hierarchical breakdown of revenue across our nested product categories
2. How our performance metrics compare across all four quarters  
3. The customer flow through our sales funnel process

Here's the data:
- Electronics: Q1: $150k, Q2: $180k, Q3: $220k, Q4: $195k
- Clothing: Q1: $120k, Q2: $140k, Q3: $160k, Q4: $175k  
- Home & Garden: Q1: $80k, Q2: $95k, Q3: $110k, Q4: $125k```

Copy link
Collaborator

@angelahning angelahning Sep 8, 2025

Choose a reason for hiding this comment

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

Goose created 3 visualizations - a treemap, radar, and sankey diagram. But only one diagram is rendered on the UI.
Screenshot 2025-09-08 at 4 35 10 PM

Copy link
Collaborator

Choose a reason for hiding this comment

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

same behaviour on main. Good to go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

unrelated to this PR - but worth looking for any console errors in electron client, as the html can likely be tweaked

Copy link
Collaborator

Choose a reason for hiding this comment

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

(and the prompts have changed enough that there could be some effect there)

Copy link
Collaborator

@angelahning angelahning left a comment

Choose a reason for hiding this comment

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

Looks good. Works well on the UI (I didn't test on CLI).

@michaelneale michaelneale self-assigned this Sep 8, 2025
Copy link
Collaborator

@michaelneale michaelneale left a comment

Choose a reason for hiding this comment

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

Looks like a lot of tool descriptions are truncated, which will hurt performance (I wonder if we saw similar thing with developer one, but seems ok).

did some inline comments.

I think the tool macro like:

    #[tool(
        name = "web_scrape",
        description = "Fetch and save content from a web page. The content can be saved as text, json, or binary files."
    )]

Can be more like:

    #[tool(
        name = "web_scrape",
        description = r#"Fetch and save content from a web page. 
The content can be saved as text, json, or binary files.

and the rest here...

including examples
"#
    )]

this would make it perform like original, and also probably make the diffs managable, at the moment github hides it away as so much has changed (likely these)

Example:
{
"type": "line",
"title": "Monthly Sales",
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like a lot of this doc/prompt was lost, it is hard to tell as everything is on one line now, but I can't find this anywhere, which will likely stop it working.

- nodes: Array of objects with 'name' and optional 'category' properties
- links: Array of objects with 'source', 'target', and 'value' properties
Example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is missing from the new tool description

"#},
object!({
"type": "object",
"required": ["data"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this is now done by the macro?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes. I tried to bake this into the schema of the input args itself. will validate in the inspector it makes it into the inputSchema before merge

- labels: Array of strings representing the dimensions/axes
- datasets: Array of dataset objects with 'label' and 'data' properties
Example:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is also missing

let web_scrape_tool = Tool::new(
"web_scrape",
indoc! {r#"
Fetch and save content from a web page. The content can be saved as:
Copy link
Collaborator

Choose a reason for hiding this comment

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

missing.

@michaelneale
Copy link
Collaborator

can we ensure we have all the tool descriptions intact like before, with examples in the tool macros (which I think can be multiline?)

@alexhancock alexhancock force-pushed the alexhancock/rmcp-server-conversions branch 3 times, most recently from 7ae90d9 to fa686b8 Compare September 9, 2025 18:24
@michaelneale
Copy link
Collaborator

I think once the prompts are back in place, probably good to go. I saw some changes to the developer mcp - which seemed more rust style related - deliberate?

@alexhancock alexhancock force-pushed the alexhancock/rmcp-server-conversions branch 2 times, most recently from 6b782cb to 009db68 Compare September 10, 2025 17:54
@alexhancock
Copy link
Collaborator Author

Need to find a way to bring back all the os specific tool descriptions while also using rmcp instead of the old mcp-server construct which modeled things as a vec of Tools

The macros in rmcp, while nice, make it hard to provide os specific descriptions.

Will find a good pattern for this.

@michaelneale
Copy link
Collaborator

@alexhancock yeah fair point - as sometimes we want to programmatically form the set of tools available, either at load time or even dynamically

@alexhancock alexhancock force-pushed the alexhancock/rmcp-server-conversions branch from 009db68 to fab29c8 Compare September 11, 2025 18:00
@alexhancock
Copy link
Collaborator Author

Breaking this apart into separate changes that are easier to verify are totally correct

Here is the first one #4664

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.

4 participants