Remove dependency on goose-mcp from goose crate by jamadeo · Pull Request #6637 · block/goose

jamadeo · 2026-01-22T16:31:41Z

goose-mcp contains all of our "built-in" mcp servers. At present we do compile these into the main goose binary (goose-server and goose-cli), but better to keep the goose crate itself independent, as other binaries are built on it that don't need to drag in all of the various developer/analysis tool dependencies.

Copilot

Pull request overview

This PR successfully removes the circular dependency between the goose and goose-mcp crates by inverting the dependency relationship. The goose crate now defines the BuiltinDef and SpawnServerFn types, while goose-mcp depends on goose and re-exports these types.

Changes:

Introduced builtin_extension.rs in the goose crate to define extension types
Updated AgentManager, AgentConfig, Agent, and ExtensionManager to accept builtin extensions as constructor parameters
Modified all call sites to pass builtin extensions (either HashMap::new() for tests or goose_mcp::BUILTIN_EXTENSIONS.clone() for production code)

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
`crates/goose/src/builtin_extension.rs`	New module defining `BuiltinDef` struct and `SpawnServerFn` type for extension definitions
`crates/goose/src/lib.rs`	Exports the new `builtin_extension` module
`crates/goose/src/agents/agent.rs`	Updated `AgentConfig` and `Agent` to accept and store builtin extensions
`crates/goose/src/agents/extension_manager.rs`	Modified to accept builtin extensions and use them instead of `goose_mcp::BUILTIN_EXTENSIONS`
`crates/goose/src/execution/manager.rs`	Added `builtin_extensions` parameter to `AgentManager` with new initialization methods
`crates/goose/Cargo.toml`	Removed dependency on `goose-mcp`
`crates/goose-mcp/src/lib.rs`	Re-exports types from `goose::builtin_extension` and continues to define `BUILTIN_EXTENSIONS`
`crates/goose-mcp/Cargo.toml`	Added dependency on `goose`
`crates/goose-server/src/state.rs`	Uses `initialize_with_builtin_extensions()` to pass builtin extensions
`crates/goose-cli/src/scenario_tests/scenario_runner.rs`	Passes `goose_mcp::BUILTIN_EXTENSIONS.clone()` to `AgentConfig`
`crates/goose-acp/src/server.rs`	Passes `goose_mcp::BUILTIN_EXTENSIONS.clone()` to `AgentConfig`
`crates/goose-acp/Cargo.toml`	Added explicit dependency on `goose-mcp`
`crates/goose/tests/agent.rs`	Updated all test call sites to pass `HashMap::new()`
`crates/goose/tests/mcp_integration_test.rs`	Updated test to pass `HashMap::new()` to `ExtensionManager`
`Cargo.lock`	Reflects dependency changes across crates

Copilot

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

crates/goose/src/execution/manager.rs:85

The initialize() and instance() methods are identical. Both use the same AGENT_MANAGER OnceCell and have the same implementation. This duplication is unnecessary - you should either remove initialize() and use instance() everywhere, or clarify the semantic difference between them if one is intended.

    pub async fn initialize() -> Result<Arc<Self>> {
        AGENT_MANAGER
            .get_or_try_init(|| async {
                let max_sessions = Config::global()
                    .get_goose_max_active_agents()
                    .unwrap_or(DEFAULT_MAX_SESSION);
                let schedule_file_path = Paths::data_dir().join("schedule.json");
                let session_manager = Arc::new(SessionManager::instance());
                let manager = Self::new(
                    session_manager,
                    schedule_file_path,
                    Some(max_sessions),
                )
                .await?;
                Ok(Arc::new(manager))
            })
            .await
            .cloned()
    }

    pub async fn instance() -> Result<Arc<Self>> {
        AGENT_MANAGER
            .get_or_try_init(|| async {
                let max_sessions = Config::global()
                    .get_goose_max_active_agents()
                    .unwrap_or(DEFAULT_MAX_SESSION);
                let schedule_file_path = Paths::data_dir().join("schedule.json");
                let session_manager = Arc::new(SessionManager::instance());
                let manager = Self::new(
                    session_manager,
                    schedule_file_path,
                    Some(max_sessions),
                )
                .await?;
                Ok(Arc::new(manager))
            })
            .await
            .cloned()
    }

Copilot

Pull request overview

Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

crates/goose/src/execution/manager.rs:77

initialize() duplicates the implementation of instance() byte-for-byte, which increases maintenance risk if one gets updated without the other; consider removing initialize() and using instance(), or make one call the other so there’s a single initialization path.

    pub async fn initialize() -> Result<Arc<Self>> {
        AGENT_MANAGER
            .get_or_try_init(|| async {
                let max_sessions = Config::global()
                    .get_goose_max_active_agents()
                    .unwrap_or(DEFAULT_MAX_SESSION);
                let schedule_file_path = Paths::data_dir().join("schedule.json");
                let session_manager = Arc::new(SessionManager::instance());
                let manager =
                    Self::new(session_manager, schedule_file_path, Some(max_sessions)).await?;
                Ok(Arc::new(manager))
            })
            .await
            .cloned()
    }

    pub async fn instance() -> Result<Arc<Self>> {
        AGENT_MANAGER
            .get_or_try_init(|| async {
                let max_sessions = Config::global()
                    .get_goose_max_active_agents()
                    .unwrap_or(DEFAULT_MAX_SESSION);
                let schedule_file_path = Paths::data_dir().join("schedule.json");
                let session_manager = Arc::new(SessionManager::instance());
                let manager =
                    Self::new(session_manager, schedule_file_path, Some(max_sessions)).await?;
                Ok(Arc::new(manager))
            })
            .await
            .cloned()
    }

crates/goose/src/agents/extension_manager.rs:587

With the new global registry, this "Unknown builtin extension" error can also happen when builtins were never registered in the process; consider amending the message to hint at calling register_builtin_extensions(...) during startup to make misconfiguration easier to diagnose.

                if !builtin_extensions.contains_key(name.as_str()) {
                    return Err(ExtensionError::ConfigError(format!(
                        "Unknown builtin extension: {}",
                        name
                    )));

Copilot · 2026-01-28T17:39:22Z

crates/goose/src/builtin_extension.rs

+    BUILTIN_REGISTRY.write().unwrap().insert(name, spawn_fn);
+}
+
+/// Register multiple builtin extensions from a HashMap
+pub fn register_builtin_extensions(extensions: HashMap<&'static str, SpawnServerFn>) {
+    let mut registry = BUILTIN_REGISTRY.write().unwrap();
+    registry.extend(extensions);
+}
+
+/// Get a copy of all registered builtin extensions
+pub fn get_builtin_extensions() -> HashMap<&'static str, SpawnServerFn> {
+    BUILTIN_REGISTRY.read().unwrap().clone()


The unwrap() calls on the global RwLock will panic if the lock is ever poisoned; handle the PoisonError (e.g., by recovering with into_inner() or returning a Result) so a previous panic doesn’t take down the whole process when registering/reading builtins.

Suggested change

BUILTIN_REGISTRY.write().unwrap().insert(name, spawn_fn);

}

/// Register multiple builtin extensions from a HashMap

pub fn register_builtin_extensions(extensions: HashMap<&'static str, SpawnServerFn>) {

let mut registry = BUILTIN_REGISTRY.write().unwrap();

registry.extend(extensions);

}

/// Get a copy of all registered builtin extensions

pub fn get_builtin_extensions() -> HashMap<&'static str, SpawnServerFn> {

BUILTIN_REGISTRY.read().unwrap().clone()

BUILTIN_REGISTRY

.write()

.unwrap_or_else(|e| e.into_inner())

.insert(name, spawn_fn);

}

/// Register multiple builtin extensions from a HashMap

pub fn register_builtin_extensions(extensions: HashMap<&'static str, SpawnServerFn>) {

let mut registry = BUILTIN_REGISTRY

.write()

.unwrap_or_else(|e| e.into_inner());

registry.extend(extensions);

}

/// Get a copy of all registered builtin extensions

pub fn get_builtin_extensions() -> HashMap<&'static str, SpawnServerFn> {

BUILTIN_REGISTRY

.read()

.unwrap_or_else(|e| e.into_inner())

.clone()

Copilot · 2026-01-28T17:39:22Z

crates/goose/src/builtin_extension.rs

+/// Get a copy of all registered builtin extensions
+pub fn get_builtin_extensions() -> HashMap<&'static str, SpawnServerFn> {
+    BUILTIN_REGISTRY.read().unwrap().clone()
+}


get_builtin_extensions() clones the entire registry, which forces an allocation even when callers only need a lookup; consider adding has_builtin_extension(name: &str) / get_builtin_extension(name: &str) helpers that do the lookup under the read lock to avoid cloning the full HashMap.

Suggested change

}

}

/// Check if a builtin extension with the given name is registered

pub fn has_builtin_extension(name: &str) -> bool {

BUILTIN_REGISTRY.read().unwrap().contains_key(name)

}

/// Get the builtin extension spawn function for the given name, if registered

pub fn get_builtin_extension(name: &str) -> Option<SpawnServerFn> {

BUILTIN_REGISTRY.read().unwrap().get(name).copied()

}

jh-block

nice improvement! two minor comments

jh-block · 2026-01-29T08:47:57Z

crates/goose/src/execution/manager.rs

+            .await
+            .cloned()
+    }
+


This new initialize() method seems to be identical to the instance() method below. Could probably just use instance() wherever we're calling this?

jh-block · 2026-01-29T08:50:34Z

crates/goose/src/builtin_extension.rs

+}
+
+/// Get a copy of all registered builtin extensions
+pub fn get_builtin_extensions() -> HashMap<&'static str, SpawnServerFn> {


How often is this called? Could potentially wrap the HashMap in an Arc (inside the RwLock) to make cloning it ~free, or provide a get_builtin_extension(name: &'static str) -> SpawnServerFn so that we don't have to return the whole registry.

Copilot

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Copilot · 2026-02-02T14:35:44Z

crates/goose/src/agents/extension_manager.rs

                let timeout_duration = Duration::from_secs(timeout.unwrap_or(300));
-                let normalized_name = name_to_key(name);
-
-                if !goose_mcp::BUILTIN_EXTENSIONS.contains_key(normalized_name.as_str()) {
-                    return Err(ExtensionError::ConfigError(format!(
-                        "Unknown builtin extension: {}",
-                        name
-                    )));
-                }
+                let extension_fn = get_builtin_extension(name.as_str()).ok_or_else(|| {
+                    ExtensionError::ConfigError(format!("Unknown builtin extension: {}", name))
+                })?;


ExtensionConfig::Builtin lookup now uses the raw name, but builtin keys are normalized elsewhere (and previously used name_to_key), so configs like "Developer"/"developer" may regress; lookup should use the normalized key (or ensure registration uses the same normalization).

Copilot · 2026-02-02T14:35:44Z

crates/goose/src/builtin_extension.rs

+    registry.extend(extensions);
+}
+
+/// Get a copy of all registered builtin extensions


The doc comment says this returns “a copy of all registered builtin extensions”, but the function returns a single extension by name; update the comment to match the behavior.

Suggested change

/// Get a copy of all registered builtin extensions

/// Get a registered builtin extension by name

Copilot

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 2 comments.

Copilot · 2026-02-02T15:51:11Z

crates/goose-acp/tests/common_tests/mod.rs

    let _ = session.prompt(prompt, PermissionDecision::Cancel).await;

    let result = fs::read_to_string("/tmp/result.txt").unwrap_or_default();
+    eprintln!("{}", result);


This eprintln! will spam test output on success (and can leak fixture data in CI logs); remove it or only print the file contents when the assertion fails.

Suggested change

eprintln!("{}", result);

if !result.contains(FAKE_CODE) {

eprintln!("{}", result);

}

Copilot · 2026-02-02T15:51:12Z

crates/goose/src/agents/extension_manager.rs

            ExtensionConfig::Builtin { name, timeout, .. } => {
                let timeout_duration = Duration::from_secs(timeout.unwrap_or(300));
-                let normalized_name = name_to_key(name);
-
-                if !goose_mcp::BUILTIN_EXTENSIONS.contains_key(normalized_name.as_str()) {
-                    return Err(ExtensionError::ConfigError(format!(
-                        "Unknown builtin extension: {}",
-                        name
-                    )));
-                }
+                let extension_fn = get_builtin_extension(name.as_str()).ok_or_else(|| {
+                    ExtensionError::ConfigError(format!("Unknown builtin extension: {}", name))
+                })?;


The builtin extension lookup no longer normalizes the configured name (unlike the previous name_to_key lookup), so configs like "Developer" or names containing spaces/hyphens that previously worked may now fail with "Unknown builtin extension"; compute normalized_name = name_to_key(name) once and use it for both registry lookup and the Docker goose mcp invocation.

This reverts commit 95dea77.

Copilot

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Copilot · 2026-02-02T17:39:06Z

crates/goose/src/builtin_extension.rs

+static BUILTIN_REGISTRY: Lazy<RwLock<HashMap<&'static str, SpawnServerFn>>> =
+    Lazy::new(|| RwLock::new(HashMap::new()));


This module uses std::sync::RwLock but it’s accessed from async code paths (e.g., extension loading on Tokio); consider switching to tokio::sync::RwLock (or another async-friendly container) to avoid blocking executor threads.

…ntext * 'main' of github.com:block/goose: refactor(providers): extract ProviderDef trait and OpenAiCompatibleProvider (#6832) feat: ask ai discord bot (#6842) chore(maintenance): make GitHub repo configurable for auto-updater and publisher (#6828) fix: make apps work in built copies of goose (#6901) Remove dependency on goose-mcp from goose crate (#6637) Clean up build canonical warnings (#6880) Sync desktop_prompt with UI (#6898)

Signed-off-by: Harrison <hcstebbins@gmail.com>

jamadeo added 12 commits January 21, 2026 14:08

Inject into extension manager

8cb23a4

Agent

ff112d5

Move the dependency

e0ac05d

-acp crate

dcda387

extraneous

8de46fb

fmt

b7695c7

Merge remote-tracking branch 'origin/main' into jackamadeo/rm-dep

b0d88db

Owned map

d65721e

use over qualify

67632ff

Simplify

a68b89a

one more use

005a565

Merge remote-tracking branch 'origin/main' into jackamadeo/rm-dep

08ad92f

Copilot AI review requested due to automatic review settings January 22, 2026 16:31

Copilot started reviewing on behalf of jamadeo January 22, 2026 16:32 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

zanesq approved these changes Jan 22, 2026

View reviewed changes

jamadeo added 3 commits January 22, 2026 13:31

Set them

de0b113

Do it with a registry

ccee0ed

One less dep

0f1dfeb

Copilot AI review requested due to automatic review settings January 22, 2026 19:02

Copilot started reviewing on behalf of jamadeo January 22, 2026 19:02 View session

Copilot AI reviewed Jan 22, 2026

View reviewed changes

re-format

097a6e0

DOsinga requested a review from jh-block January 28, 2026 17:19

Merge remote-tracking branch 'origin/main' into jackamadeo/rm-dep

ca9b2db

Copilot AI review requested due to automatic review settings January 28, 2026 17:35

Copilot started reviewing on behalf of jamadeo January 28, 2026 17:36 View session

Copilot AI reviewed Jan 28, 2026

View reviewed changes

jh-block approved these changes Jan 29, 2026

View reviewed changes

Merge remote-tracking branch 'origin/main' into jackamadeo/rm-dep

10278d3

Copilot AI review requested due to automatic review settings February 2, 2026 14:32

Copilot started reviewing on behalf of jamadeo February 2, 2026 14:33 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

jamadeo added 2 commits February 2, 2026 09:52

register the builtins in serve()

b4ad24c

Merge remote-tracking branch 'origin/main' into jackamadeo/rm-dep

0047f29

Copilot AI review requested due to automatic review settings February 2, 2026 15:22

Copilot started reviewing on behalf of jamadeo February 2, 2026 15:23 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

revert me: print the result

95dea77

Copilot AI review requested due to automatic review settings February 2, 2026 15:46

jamadeo force-pushed the jackamadeo/rm-dep branch from 9e45210 to 95dea77 Compare February 2, 2026 15:46

Copilot started reviewing on behalf of jamadeo February 2, 2026 15:47 View session

Copilot AI reviewed Feb 2, 2026

View reviewed changes

jamadeo added 2 commits February 2, 2026 12:30

Move, again

c44dafb

Revert "revert me: print the result"

db3e5e5

This reverts commit 95dea77.

Copilot AI review requested due to automatic review settings February 2, 2026 17:35

Copilot started reviewing on behalf of jamadeo February 2, 2026 17:36 View session

Extraneous from merge

d6e4069

Copilot AI reviewed Feb 2, 2026

View reviewed changes

jamadeo added this pull request to the merge queue Feb 2, 2026

jamadeo removed this pull request from the merge queue due to a manual request Feb 2, 2026

Add back normalize

a4e5f16

jamadeo enabled auto-merge February 2, 2026 18:20

jamadeo added this pull request to the merge queue Feb 2, 2026

Merged via the queue into main with commit a9f4b2d Feb 2, 2026
16 checks passed

jamadeo deleted the jackamadeo/rm-dep branch February 2, 2026 18:26

github-actions bot mentioned this pull request Feb 3, 2026

chore(release): release version 1.23.0 (minor) #6906

Merged

stebbins pushed a commit to stebbins/goose that referenced this pull request Feb 4, 2026

Remove dependency on goose-mcp from goose crate (block#6637)

9984c28

Signed-off-by: Harrison <hcstebbins@gmail.com>

-}
+}
+/// Check if a builtin extension with the given name is registered
+pub fn has_builtin_extension(name: &str) -> bool {
+    BUILTIN_REGISTRY.read().unwrap().contains_key(name)
+}
+/// Get the builtin extension spawn function for the given name, if registered
+pub fn get_builtin_extension(name: &str) -> Option<SpawnServerFn> {
+    BUILTIN_REGISTRY.read().unwrap().get(name).copied()
+}

	/// Get a copy of all registered builtin extensions
	/// Get a registered builtin extension by name

		static BUILTIN_REGISTRY: Lazy<RwLock<HashMap<&'static str, SpawnServerFn>>> =
		Lazy::new(\|\| RwLock::new(HashMap::new()));

Conversation

jamadeo commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

jh-block left a comment

Choose a reason for hiding this comment

Uh oh!

jh-block Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

jh-block Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jamadeo commented Jan 22, 2026 •

edited

Loading