ACP editor tools by jamadeo · Pull Request #7388 · block/goose

jamadeo · 2026-02-20T15:38:55Z

This adds tools to use ACP's file system and terminals methods to call back to the ACP client to perform file edits and shell commands. It's enabled when the client advertises this capability.

This makes for a nicer experience when using goose over ACP in an editor like Zed.

Before:
https://github.com/user-attachments/assets/c3ee6a05-a4ba-411b-a881-02ae909b6274

After:
https://github.com/user-attachments/assets/247730ee-c83e-454f-b71c-b4b07630305a

Copilot

Pull request overview

This PR adds an --acp-editor-tools flag to goose-acp that replaces goose's standard developer tools with ACP-based editor tools. These tools use the Agent Client Protocol's file system and terminal methods to delegate file operations and shell commands back to the ACP client (like Zed editor), providing a better integrated experience.

Changes:

Introduces ToolCallContext struct to pass session ID, working directory, and tool call request ID through the tool dispatch chain
Refactors all call_tool implementations to accept context instead of individual parameters
Adds new ACP tools module implementing read, write, str_replace, insert, and shell tools using ACP protocol
Refactors text editor functions to separate in-memory logic from file I/O for reusability
Updates extension manager to expose tools unprefixed when only one client is active

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
crates/goose/src/agents/tool_execution.rs	Adds ToolCallContext struct to encapsulate tool call parameters
crates/goose/src/agents/extension_manager.rs	Updates dispatch_tool_call to use ToolCallContext; fixes tool prefixing logic
crates/goose/src/agents/agent.rs	Creates ToolCallContext when dispatching tool calls
crates/goose/src/agents/mcp_client.rs	Updates McpClientTrait call_tool signature to use ToolCallContext
crates/goose/src/agents/platform_extensions/*.rs	Updates all platform extension call_tool implementations
crates/goose-acp/src/tools.rs	New module implementing ACP-based file and shell tools
crates/goose-acp/src/server.rs	Adds acp_editor_tools flag and conditionally adds ACP tools
crates/goose-acp/src/server_factory.rs	Passes acp_editor_tools flag through configuration
crates/goose-mcp/src/developer/text_editor.rs	Refactors replace/insert to extract in-memory logic
crates/goose-cli/src/cli.rs	Adds CLI flag for acp_editor_tools
crates/goose-server/src/routes/agent.rs	Updates tool call dispatch to use ToolCallContext
All test files	Updates test code to construct ToolCallContext

Copilot · 2026-02-20T15:44:05Z

crates/goose-acp/src/server.rs

+        let is_acp_tool = tool_request
+            .and_then(|req| req.tool_call.as_ref().ok())
+            // TODO: something more robust here
+            .is_some_and(|req| {
+                req.name == "read"
+                    || req.name == "write"
+                    || req.name == "str_replace"
+                    || req.name == "insert"
+                    || req.name == "shell"
+            });


Hard-coding tool names for detection is fragile and creates a maintenance burden. Consider adding a method to AcpTools or a marker to identify these tools programmatically, or store the extension name in the tool metadata and check against "acp-tools" instead.

Copilot · 2026-02-20T15:44:06Z

crates/goose-acp/src/tools.rs

+    ($t:ty) => {
+        serde_json::to_value(schema_for!($t))
+            .unwrap()
+            .as_object()
+            .unwrap()
+            .clone()
+    };


The schema macro uses unwrap() which will panic at initialization if the schema conversion fails. While this is unlikely to fail for these simple parameter types, consider handling the error more gracefully or using a static assertion approach to catch schema issues at compile time rather than at runtime initialization.

Suggested change

($t:ty) => {

serde_json::to_value(schema_for!($t))

.unwrap()

.as_object()

.unwrap()

.clone()

};

($t:ty) => {{

let value = match serde_json::to_value(schema_for!($t)) {

Ok(v) => v,

Err(_) => serde_json::Value::Object(JsonObject::new()),

};

match value.as_object() {

Some(map) => map.clone(),

None => JsonObject::new(),

}

}};

Copilot · 2026-02-20T15:44:06Z

crates/goose-acp/src/tools.rs

@@ -0,0 +1,400 @@
+use std::{cell::LazyCell, fmt::Display, future::Future, path::PathBuf};


Replace std::cell::LazyCell with once_cell::sync::Lazy to match the codebase convention. The codebase consistently uses once_cell for lazy static initialization throughout (see usage in goose/src/agents/extension_manager.rs, goose-mcp/src/developer/rmcp_developer.rs, and many other files).

Suggested change

use std::{cell::LazyCell, fmt::Display, future::Future, path::PathBuf};

use std::{fmt::Display, future::Future, path::PathBuf};

use once_cell::sync::Lazy;

Copilot · 2026-02-20T15:44:06Z

crates/goose-acp/src/tools.rs

+        .map_err(|_| {
+            ServiceError::McpError(ErrorData::internal_error("failed to replace", None))


Error information is being discarded when mapping to internal error. The ErrorData from text_editor_replace_inmem contains useful details (like "'old_str' must appear exactly once") that would help users understand what went wrong. Consider propagating the original error message instead of using a generic "failed to replace" message.

Suggested change

.map_err(|_| {

ServiceError::McpError(ErrorData::internal_error("failed to replace", None))

.map_err(|e| {

ServiceError::McpError(ErrorData::internal_error(

&format!("failed to replace: {e}"),

None,

))

Copilot · 2026-02-20T15:44:07Z

crates/goose-acp/src/tools.rs

+            params.position,
+            &params.new_str.as_str(),
+        )
+        .map_err(|_| ServiceError::McpError(ErrorData::internal_error("failed to insert", None)))?;


Error information is being discarded when mapping to internal error. The ErrorData from text_editor_insert_inmem contains useful details (like "Insert line X is beyond the end of the file") that would help users understand what went wrong. Consider propagating the original error message instead of using a generic "failed to insert" message.

Suggested change

.map_err(|_| ServiceError::McpError(ErrorData::internal_error("failed to insert", None)))?;

.map_err(ServiceError::McpError)?;

Copilot · 2026-02-20T15:44:07Z

crates/goose-acp/src/tools.rs

+        params: WriteParams,
+    ) -> Result<CallToolResult, ServiceError> {
+        let path = self.working_dir.join(params.path);
+        let content = read_file(&path, &self.cx, self.session_id.clone()).await?;


The write operation reads the old file content at line 182, but this is unnecessary unless the file already exists. For new files, this will fail. Consider only reading the old content if the file exists, or handle the error case when the file doesn't exist (which is valid for a write operation).

This seems like a valid concern, no?

Copilot · 2026-02-20T15:44:07Z

crates/goose-acp/src/tools.rs

+    }
+
+    fn get_info(&self) -> Option<&InitializeResult> {
+        todo!()


This unimplemented method will panic if called. Since this implements the McpClientTrait, get_info might be called by the framework. Either implement it with a proper value or return None if this information isn't available for ACP tools.

Suggested change

todo!()

None

codefromthecrypt

This is a clever and elegant way to approach the challenge. While most agents do a tool dispatch solution, this replaces the entire tools, allowing a more direct implementation.

I have a branch which has tests etc as well the read path and I will port over this.

Great job!

rabi

If we all feel this is the way to go, then it's ok. My major reason for not approaching it this way as to not maintaining two different implementations of developer tools. If the plan is to move CLI/GUI to ACP clients, then this duplication is temporary and fine. Otherwise, a DeveloperBackend trait with capability-gated backend wiring would atleast keep the tool definitions in sync.

rabi · 2026-03-04T04:07:45Z

crates/goose-acp/src/tools.rs

+use serde::{de::DeserializeOwned, Deserialize, Serialize};
+use tokio_util::sync::CancellationToken;
+
+struct Capabilities {


Should we not use ClientCapabilities rather than having a separate struct?

rabi · 2026-03-04T04:08:04Z

crates/goose-acp/src/tools.rs

+        params: WriteParams,
+    ) -> Result<CallToolResult, ServiceError> {
+        let path = self.working_dir.join(params.path);
+        let content = read_file(&path, &self.cx, self.session_id.clone()).await?;


This seems like a valid concern, no?

rabi · 2026-03-04T04:08:59Z

crates/goose-acp/src/server.rs

        let config_path = self.config_dir.join(CONFIG_YAML_NAME);
        if let Ok(config_file) = Config::new(&config_path, "goose") {
-            let extensions = get_enabled_extensions_with_config(&config_file);
+            let mut extensions = get_enabled_extensions_with_config(&config_file);


The exclude_developer check seems to filter only config-loaded extensions. If passed via --with-builtin developer, it would create duplicate/conflicting tools? Should we remove the --with-builtin flag completely now that user is expected to enable extensions via config and then we filter based on that?

rabi · 2026-03-04T04:10:12Z

crates/goose-acp/src/server.rs

    goose_mode: goose::config::GooseMode,
    disable_session_naming: bool,
    builtins: Vec<String>,
+    client_capabilities: Mutex<ClientCapabilities>,


Maybe we use tokio::sync::OnceCell? As it's only set during initialize and then can be read freely?

jamadeo · 2026-03-05T16:10:46Z

If we all feel this is the way to go, then it's ok. My major reason for not approaching it this way as to not maintaining two different implementations of developer tools. If the plan is to move CLI/GUI to ACP clients, then this duplication is temporary and fine. Otherwise, a DeveloperBackend trait with capability-gated backend wiring would atleast keep the tool definitions in sync.

I agree with the reasoning, but one problem which we have now is that the ACP editor methods are a bit more constrained than how the new developer tools work in goose today. For example, if we want all files reads to go through ACP when you are connected to an editor, we would not want goose to read using the shell (cat/head/etc). But goose's standard developer tools now lack a readFile method entirely, meaning all reads happen with the shell.

This does mean that the tools seen by the model are different when you are using goose with an ACP-enabled editor vs as a standalone agent. And maybe that's okay? Arguably, as a user, I'd expect goose to be more autonomous when run on its own, and more interactive through an editor. But I'm open to arguments in the other direction.

codefromthecrypt · 2026-03-05T23:03:09Z

@jamadeo #7668 I raised last night swaps out the tool impl but re-uses exactly the same tool defs, so I think this balances the debate, correct me if I'm wrong. There's a whole lot more ACP work to complete, so hoping we can close this topic out. There are a lot more complicated things left and time budget is a thing or we'll never get through the spec.

rabi · 2026-03-06T06:31:46Z

But goose's standard developer tools now lack a readFile method entirely, meaning all reads happen with the shell.

@jamadeo Thanks. Yeah that's why I had implemented the read tool in #6940, so that tools remain the same but we decide how they behave based on the delegate registered.

jamadeo · 2026-03-11T17:23:17Z

most of this is in #7668

jamadeo added 5 commits February 19, 2026 20:10

ACP tools

722a925

Pass tool call ID into tool context

4a33bc4

Hack around the shell

ba8b7a9

Merge remote-tracking branch 'origin/main' into jackamadeo/acp-tools

53b8115

More ACP messaging

cdf900e

Copilot AI review requested due to automatic review settings February 20, 2026 15:38

Copilot started reviewing on behalf of jamadeo February 20, 2026 15:39 View session

Copilot AI reviewed Feb 20, 2026

View reviewed changes

jamadeo and others added 9 commits February 20, 2026 11:41

lints

d57ad91

Fix out of order messages

581065e

Check the extension name

2554e58

Use capabilities

23dbed7

Merge branch 'origin/main' into jackamadeo/acp-tools

c3ad2e2

Merge remote-tracking branch 'origin/main' into jackamadeo/acp-tools

e00e510

Update for new developer tools

7c5d164

lint

a60c6c7

remove un-prefixing

6f5d42a

jamadeo mentioned this pull request Mar 2, 2026

feat(acp): delegate developer file I/O through ACP fs methods #6940

Closed

10 tasks

codefromthecrypt enabled auto-merge March 4, 2026 03:02

codefromthecrypt approved these changes Mar 4, 2026

View reviewed changes

rabi reviewed Mar 4, 2026

View reviewed changes

codefromthecrypt mentioned this pull request Mar 5, 2026

feat(acp): add read tool and delegate filesystem I/O to ACP clients #7668

Merged

3 tasks

DOsinga assigned jamadeo Mar 9, 2026

jamadeo closed this Mar 11, 2026

auto-merge was automatically disabled March 11, 2026 17:23
Pull request was closed

jamadeo mentioned this pull request Mar 12, 2026

More acp tools #7843

Merged

3 tasks

-    ($t:ty) => {
-        serde_json::to_value(schema_for!($t))
-            .unwrap()
-            .as_object()
-            .unwrap()
-            .clone()
-    };
+    ($t:ty) => {{
+        let value = match serde_json::to_value(schema_for!($t)) {
+            Ok(v) => v,
+            Err(_) => serde_json::Value::Object(JsonObject::new()),
+        };
+        match value.as_object() {
+            Some(map) => map.clone(),
+            None => JsonObject::new(),
+        }
+    }};

		@@ -0,0 +1,400 @@
		use std::{cell::LazyCell, fmt::Display, future::Future, path::PathBuf};

	use std::{cell::LazyCell, fmt::Display, future::Future, path::PathBuf};
	use std::{fmt::Display, future::Future, path::PathBuf};
	use once_cell::sync::Lazy;

		.map_err(\|_\| {
		ServiceError::McpError(ErrorData::internal_error("failed to replace", None))

	.map_err(\|_\| ServiceError::McpError(ErrorData::internal_error("failed to insert", None)))?;
	.map_err(ServiceError::McpError)?;

Conversation

jamadeo commented Feb 20, 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 Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

rabi Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt left a comment

Choose a reason for hiding this comment

Uh oh!

rabi left a comment

Choose a reason for hiding this comment

Uh oh!

rabi Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

rabi Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

rabi Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

rabi Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

jamadeo commented Mar 5, 2026

Uh oh!

codefromthecrypt commented Mar 5, 2026

Uh oh!

rabi commented Mar 6, 2026

Uh oh!

jamadeo commented Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jamadeo commented Feb 20, 2026 •

edited

Loading