Internal MCP Crate Cleanup by alexhancock · Pull Request #4800 · block/goose

alexhancock · 2025-09-24T22:05:03Z

Fully gets rid of mcp-client and mcp-core making all our mcp tooling just use the rust-sdk
Removes concept of ToolCall and replaces it with the similar CallToolRequestParam from rmcp
Associated required code changes, compilation fixes etc

alexhancock · 2025-09-26T01:47:52Z

@jamadeo how do you feel about the fact that this removes mcp-client but still impl McpClientTrait for McpClients

Do you think we should unwind that in a change where we remove those crates, or ok to do in a followup?

Also I realize this is a big PR so can sync on it when I've verified it's ready for review otherwise.

jamadeo

really nice. I left some nitpick comments, mostly places where I think there are unnecessary clones (.to_string().clone()) and some places where there's another level of if-nesting that could be replaced with a .and_then(..)

But feel free to merge before addressing all of them, wouldn't want to be constantly fixing conflicts.

Maybe the one thing to give an extra look to is the 1 that became None in acp.rs

jamadeo · 2025-09-26T19:36:54Z

crates/goose-cli/src/commands/acp.rs

-                            meta: None,
-                        });
+                    if let Some(args_map) = args {
+                        if let Some(path_str) = args_map.get("path").and_then(|p| p.as_str()) {


nit: could be a single if let Some(.. with a .and_then

jamadeo · 2025-09-26T19:37:55Z

crates/goose-cli/src/commands/acp.rs

+                            if path.exists() && path.is_file() {
+                                locs.push(acp::ToolCallLocation {
+                                    path: path_str.into(),
+                                    line: None,


this is line: Some(1) on the left but None here, is that deliberate?

jamadeo · 2025-09-26T19:44:12Z

crates/goose-cli/src/commands/web.rs

+                                                        tool_name: tool_call
+                                                            .name
+                                                            .to_string()
+                                                            .clone(),


I don't think you need to .clone() here since .to_string() should return an owned value

jamadeo · 2025-09-26T19:50:08Z

crates/goose-cli/src/session/export.rs

            md.push_str("**Arguments:**\n");

-            match call.name.as_str() {
+            match call.name.to_string().as_str() {


match call.name.as_ref()

jamadeo · 2025-09-26T19:52:47Z

crates/goose-cli/src/session/mod.rs

-
-                                        let success = tool_response.tool_result.is_ok();
-                                        let result_status = if success { "success" } else { "error" };
+                                            .unwrap_or_else(|| "tool".to_string().into());


did you mean to change this from "unknown" to "tool"?

good catch - thank you

jamadeo · 2025-09-26T19:54:02Z

crates/goose-cli/src/session/output.rs

-                                        style(format!("{}", item)).green()
-                                    );
+    if let Some(args) = &call.arguments {
+        if let Some(Value::Array(task_parameters)) = args.get("task_parameters") {


could chain these two as well

jamadeo · 2025-09-26T19:58:56Z

crates/goose/src/providers/formats/snowflake.rs

-            let tool_call = ToolCall::new(name, Value::Object(serde_json::Map::new()));
-            message = message.with_tool_request(id, Ok(tool_call));
+    if let Some(id) = tool_use_id {
+        if let Some(name) = tool_name {


could chain these to remove a nesting level

jamadeo · 2025-09-26T20:07:09Z

@jamadeo how do you feel about the fact that this removes mcp-client but still impl McpClientTrait for McpClients

Do you think we should unwind that in a change where we remove those crates, or ok to do in a followup?

Also I realize this is a big PR so can sync on it when I've verified it's ready for review otherwise.

I think that's fine, if there's some point in the future where it makes sense to remove a layer of abstraction, we can always do that.

This reverts commit 146cf31.

This reverts commit b9ba8dc.

…-unification * 'main' of github.com:block/goose: (24 commits) feat(cli): add `path` & `limit` to `session list` command (#4878) Allow better concurrent access (#4896) fix: Windows prompt cursor positioning issue with ANSI escape sequences (#4464) Fix: LiteLLM API key field not showing in UI configuration (#4105) fix: path is duplicated on tool calls causing them to fail (#4658) (#4859) add new prompt to get all available tutorials (#4802) Add filtering for agentVisible: false messages on streaming providers (#4847) alexhancock/mcp-crate-cleanup (#4885) docs: rename sub-recipe to subrecipe (#4886) docs: new multi-model section with autopilot topic (#4864) make agent manager singleton (#4880) Cli web auth token (#4456) fix(token_counter): fix panic with GitHub Copilot (#4632) Revert "Internal MCP Crate Cleanup (#4800)" (#4883) remove 2 redundant comments and one that lies (#4866) Internal MCP Crate Cleanup (#4800) Fix #4612: Return non-zero exit code when CLI session ends with error (#4621) Dead code cleanup (#4873) fix: restoring test data and correcting name (#4875) Add .goosehints file to enforce lowercase branding in documentation (#4870) ...

* main: (206 commits) Tiny: fix github casing (block#4903) remove anyOf from create_task tool (block#4897) chore(deps): bump tracing-subscriber from 0.3.19 to 0.3.20 (block#4442) fix optional recipe schema zod validation (block#4900) Added CMD+T keyboard shortcut that takes you to the Home tab (block#4541) feat(cli): add `path` & `limit` to `session list` command (block#4878) Allow better concurrent access (block#4896) fix: Windows prompt cursor positioning issue with ANSI escape sequences (block#4464) Fix: LiteLLM API key field not showing in UI configuration (block#4105) fix: path is duplicated on tool calls causing them to fail (block#4658) (block#4859) add new prompt to get all available tutorials (block#4802) Add filtering for agentVisible: false messages on streaming providers (block#4847) alexhancock/mcp-crate-cleanup (block#4885) docs: rename sub-recipe to subrecipe (block#4886) docs: new multi-model section with autopilot topic (block#4864) make agent manager singleton (block#4880) Cli web auth token (block#4456) fix(token_counter): fix panic with GitHub Copilot (block#4632) Revert "Internal MCP Crate Cleanup (block#4800)" (block#4883) remove 2 redundant comments and one that lies (block#4866) ...

Signed-off-by: HikaruEgashira <[email protected]>

alexhancock requested a review from jamadeo September 24, 2025 22:05

alexhancock marked this pull request as draft September 24, 2025 22:05

alexhancock force-pushed the alexhancock/mcp-cleanup branch 6 times, most recently from ffe77df to 1b4a865 Compare September 26, 2025 01:39

alexhancock force-pushed the alexhancock/mcp-cleanup branch 3 times, most recently from ea04549 to 2fa1ab7 Compare September 26, 2025 15:52

alexhancock changed the title ~~Final Internal MCP Crate Cleanup~~ Internal MCP Crate Cleanup Sep 26, 2025

alexhancock force-pushed the alexhancock/mcp-cleanup branch 4 times, most recently from 33da81d to 982a579 Compare September 26, 2025 19:10

alexhancock marked this pull request as ready for review September 26, 2025 19:29

jamadeo approved these changes Sep 26, 2025

View reviewed changes

alexhancock force-pushed the alexhancock/mcp-cleanup branch 2 times, most recently from 105da2d to 0764041 Compare September 29, 2025 14:20

feat(mcp): migrate remaining internal mcp crates to rmcp

440de70

alexhancock force-pushed the alexhancock/mcp-cleanup branch from 0764041 to 440de70 Compare September 29, 2025 14:20

alexhancock merged commit 146cf31 into main Sep 29, 2025
11 checks passed

alexhancock deleted the alexhancock/mcp-cleanup branch September 29, 2025 14:39

alexhancock added a commit that referenced this pull request Sep 29, 2025

Revert "Internal MCP Crate Cleanup (#4800)"

0191b7c

This reverts commit 146cf31.

alexhancock added a commit that referenced this pull request Sep 29, 2025

Revert "Internal MCP Crate Cleanup (#4800)" (#4883)

b9ba8dc

alexhancock added a commit that referenced this pull request Sep 29, 2025

Reapply "Internal MCP Crate Cleanup (#4800)" (#4883)

72347de

This reverts commit b9ba8dc.

alexhancock mentioned this pull request Sep 29, 2025

alexhancock/mcp-crate-cleanup #4885

Merged

yingjiehe-xyz pushed a commit that referenced this pull request Sep 29, 2025

Revert "Internal MCP Crate Cleanup (#4800)" (#4883)

dfb93fe

yingjiehe-xyz pushed a commit that referenced this pull request Sep 29, 2025

Revert "Internal MCP Crate Cleanup (#4800)" (#4883)

5d5d01e

HikaruEgashira pushed a commit to HikaruEgashira/goose that referenced this pull request Oct 3, 2025

Internal MCP Crate Cleanup (block#4800)

e841df8

Signed-off-by: HikaruEgashira <[email protected]>

HikaruEgashira pushed a commit to HikaruEgashira/goose that referenced this pull request Oct 3, 2025

Revert "Internal MCP Crate Cleanup (block#4800)" (block#4883)

be279ec

Signed-off-by: HikaruEgashira <[email protected]>

This was referenced Oct 8, 2025

chore(release): release version 1.10.0 #5060

Closed

release/1.10.0 #5101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Internal MCP Crate Cleanup#4800

Internal MCP Crate Cleanup#4800
alexhancock merged 1 commit intomainfrom
alexhancock/mcp-cleanup

alexhancock commented Sep 24, 2025 •

edited

Loading

Uh oh!

alexhancock commented Sep 26, 2025

Uh oh!

jamadeo left a comment

Uh oh!

jamadeo Sep 26, 2025

Uh oh!

jamadeo Sep 26, 2025

Uh oh!

jamadeo Sep 26, 2025

Uh oh!

jamadeo Sep 26, 2025

Uh oh!

jamadeo Sep 26, 2025

Uh oh!

alexhancock Sep 29, 2025

Uh oh!

jamadeo Sep 26, 2025

Uh oh!

jamadeo Sep 26, 2025

Uh oh!

jamadeo commented Sep 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

alexhancock commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexhancock commented Sep 26, 2025

Uh oh!

jamadeo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamadeo commented Sep 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexhancock commented Sep 24, 2025 •

edited

Loading