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

[rabi]

Summary

When an ACP client (e.g. Zed) advertises fs.writeTextFile and fs.readTextFile capabilities, the developer extension routes file reads/writes through the client instead of hitting disk directly. This lets the editor track modifications and return unsaved buffer contents.

Type of Change

• Feature
• Bug fix
• Refactor / Code quality
• Performance improvement
• Documentation
• Tests
• Security fix
• Build / Release
• Other (specify below)

AI Assistance

• This PR was created or reviewed with AI assistance

Testing

Tested locally with zed.

Related Issues

Relates to #6894

Screenshots/Demos (for UX changes)

After:

[michaelneale]

hrm - yeah this is not a small change, although pretty cool. Main thing is this changes the developer tool to have a new mode, that is main thing that concerns, but otherwise would make editor experience nicer, wonder if there is a way to package this to not be as cross cutting, as you will rarely want "write mode" etc?

[rabi]

hrm - yeah this is not a small change, although pretty cool. Main thing is this changes the developer tool to have a new mode, that is main thing that concerns, but otherwise would make editor experience nicer, wonder if there is a way to package this to not be as cross cutting, as you will rarely want "write mode" etc?

It's based on client capability to write. If you think there could be scenarios where clients won't need it irrespective of them advertising their capabilities, we can probably make it configurable, but we would still need the new developer mode.

        let client_can_write = self
            .client_capabilities
            .get()
            .is_some_and(|c| c.fs_write_text_file);
        let write_mode = if client_can_write && !has_code_execution {
            WriteMode::Deferred
        } else {
            WriteMode::Direct
        };

[michaelneale]

@rabi yeah - I think that we really want this only available when actually used from visual editors don't you think? otherwise it is adding more work for the LLM to do? if could polish that off I think is a great idea

[rabi]

@rabi yeah - I think that we really want this only available when actually used from visual editors don't you think? otherwise it is adding more work for the LLM to do? if could polish that off I think is a great idea

OK, Thanks @michaelneale, I'll expose the feature behind --acp-editor flag for clients to enable it . So it will be behind both the flag and client capability to write files. The same flag can be used for other features in the future too.

[codefromthecrypt]

Hey @rabi, I filed #7451 to capture what I think the minimal approach looks like here.

The core idea is the same (route file I/O through ACP), but the implementation can be a lot simpler if we lean on what the spec already defines:

• Reads too, not just writes. fs/read_text_file means the editor can return unsaved buffer contents. This PR only handles writes.
• No custom MIME type or _meta.old_text. The spec's fs/write_text_file just takes path + content. The client decides how to present diffs. We don't need application/x-goose-file-diff or FileDiff structs.
• No --acp-editor flag. Capability negotiation during initialize is the feature gate. If the client advertises readTextFile/writeTextFile, delegate. If not, fall back to disk. No new CLI flags needed.
• No double permission. The existing session/request_permission flow handles tool approval. A second managed-write permission prompt isn't in the spec.
• No in-process duplex pipe. The developer extension's disk I/O functions (File::open, fs::write, read_to_string) just need to become pluggable so they can go through ACP instead of disk. No need to rewire how the extension connects.

The view_range to line+limit translation and the checklist in #7451 should give a clearer starting point. Happy to discuss any of it.

[rabi]

@codefromthecrypt Thanks for checking this and creating the issue as well.

Reads too, not just writes. fs/read_text_file means the editor can return unsaved buffer contents. This PR only handles writes.

We can implement these incrementally right? I kept this limited to write ad read as a followup. As you can see I've added read_text_file in as TODO.

No custom MIME type or _meta.old_text. The spec's fs/write_text_file just takes path + content. The client decides how to present diffs. We don't need application/x-goose-file-diff or FileDiff structs.

Yeah, that's a good point. I'll change.

No --acp-editor flag. Capability negotiation during initialize is the feature gate. If the client advertises readTextFile/writeTextFile, delegate. If not, fall back to disk. No new CLI flags needed.

It was not in my original PR. Added after @michaelneale asked for it.

No double permission. The existing session/request_permission flow handles tool approval. A second managed-write permission prompt isn't in the spec.

I observed cases where approval didn’t trigger as expected in my testing, so I added a temporary guard. I’ll re-verify and remove if unnecessary.

No in-process duplex pipe. The developer extension's disk I/O functions (File::open, fs::write, read_to_string) just need to become pluggable so they can go through ACP instead of disk. No need to rewire how the extension connects.

Right, that would be larger refactor of developer extension with broader touch points I wanted to avoid, but I can include it if we want.

[codefromthecrypt]

thanks for accepting the feedback so far ;) ack that there are a couple design points going on.

Here's some unsolicited feedback with TL;DR; maybe chop off a targeted smaller PR from this, with eventually landing this as the goal. The first, is probably just tuning the developer extension (builtin) so that the impl of reads are possible to adapt in with an ACP client., This is like a wiring concern and shouldn't be huge. with that in, other things are simpler to progress maybe.

---

Something that can help is that when we look at this change, probably the easiest one is the read file not the write. One of the reasons it takes me so long to get changes together is that I make a PR (such as this one) then hack it to bits until all the pieces needed are in.

This is why #6605 has taken so long, for example. For example, the infra carve-out for MCP came in this, and in some ways it is similar to the design issue you are hinting at #6972

Basically, you have a chicken-egg, right? you are looking to have a way to control which impl is used for editing tools that Goose tells the LLM it has. So, how do you get ACP to be the "impl" of the developer plugin, especially considering the chicken-egg is you don't know if ACP will be supported by the client until after it connects.

So, one way to think about it is the "builtin" are already added to ACP via #6284 I mean that's how I was testing code mode, right? So, developer is a builtin.. so one way to chop this up is to choose the easiest thing, which may be read, not write, and do only that.. surgically, in a PR. It isn't required to do both read and write in the same one.. this would prove you've solved the bootstrapping concern in the right abstraction.

The other thing that might help, is while we have hints everywhere somethings are not very explicit. I force the agent to re-read this every few minutes to cut down on cruft..

CODING_STYLE.md

[rabi]

The first, is probably just tuning the developer extension (builtin) so that the impl of reads are possible to adapt in with an ACP client

I think I've addressed all the feedback in the last update, including developer extension changes. So it should be good now.

[michaelneale]

@rabi just today (I think?) there is a new developer tool, does this work with that? otherwise LGTM I think

[rabi]

@rabi just today (I think?) there is a new developer tool, does this work with that? otherwise LGTM I think

@michaelneale If you mean #7466, does not look like it's merged yet.

[codefromthecrypt]

I'm lightly available, so only had 2 hours to figure out the best advice to give on this PR, so here goes.

Thanks for addressing point comments like the _meta.old_text leak, the application/x-goose-file-diff MIME type, the --acp-editor flag, and the double permission prompt.

The biggest thing is that the archecture itself looks the same. I mentioned I wrote #7451 for you, but maybe it got lost in comments.

So, architecture: the ACP server shouldn't parse custom URI schemes or extract diffs from tool results. That whole goose-internal://file-diff/ flow exists because the old developer extension (developer__text_editor) couldn't call back to the server. But #7466 merged and rewrote the developer extension as a platform extension with flat tools (write, edit, shell, tree). You need to rebase anyway, and the new code makes the injection point obvious — edit.rs has three fs:: calls and that's it.

I updated #7451 with the new architecture, exact file paths on main, and what the spec requires.

Below is a prompt you can iterate on to help your coding agent implement this cleanly.

Details

---

Prompt: Implement ACP file I/O delegation for goose

You are implementing #7451 — ACP file system delegation for the goose agent. This lets editors (Zed, VS Code, Neovim) provide buffer contents on reads and track modifications on writes, instead of goose hitting disk directly.

Read the issue first: #7451 — it has the full spec, architecture diagram, implementation checklist, and test scenarios.

Study these PRs for style:

• stakpak/agent#389: ACP fs capability gating in another agent (6 files, +170/-38 — small, focused)
• block/goose#7115: per-session Agent refactor (7 files, balanced refactor)
• block/goose#7466: the developer extension rewrite your PR must rebase onto

Phase 1: Research (read before coding)

Read the ACP file system spec:
https://agentclientprotocol.com/protocol/file-system

Key points:

• fs/read_text_file: client-side method. Agent calls client, client returns buffer content. Params: sessionId, path, optional line (1-based) + limit.
• fs/write_text_file: client-side method. Agent calls client with sessionId, path, content. That's it — no oldText, no diff, no metadata.
• Capability negotiation: client sends clientCapabilities.fs.readTextFile / .writeTextFile in initialize. Agent checks before calling.
• The LLM never sees these methods. It sees write, edit. The agent translates internally.

Read the tracking issue: #7451 — it has the architecture diagram, implementation checklist, and test scenarios.

Understand how editors handle diffs:

The agent sends full file content via one JSON-RPC call. The editor already has the old content (from its buffer or disk). The editor computes and displays the diff on its side:

• Zed: receives fs/write_text_file(path, content), compares with its buffer, shows native diff in assistant panel (source)
• codecompanion.nvim: same call, opens split window showing before/after using Neovim's native diff
• acp.nvim: same call, shows inline permission text, :AcpViewDiff for review
• vscode-acp: same call, delegates to registered callback for diff rendering

No diff logic belongs in the agent or server. The agent sends content; the editor diffs it.

Read the new developer extension on main (post-#7466):

The old DeveloperServer (MCP builtin, spawned via duplex pipe) is gone. The new code is a platform extension:

• DeveloperClient (in-process):
  • DeveloperClient::new() receives PlatformExtensionContext (line 33)
  • Tools: write, edit, shell, tree (lines 96-147)
• EditTools does file I/O:
  • file_write_with_cwd: calls fs::write(path, content) at line 56
  • file_edit_with_cwd: calls fs::read_to_string(&path) at line 85, then fs::write(&path, &new_content) at line 112

These three fs:: calls are your injection points. No TextFileIOStrategy enum, no SpawnServerFn changes, no builtin_context threading needed.

Phase 2: Architecture — where each responsibility belongs

Three layers:

ACP server (server.rs):

• Parse clientCapabilities.fs from InitializeRequest (already done in your PR, keep this)
• Store booleans, expose supports_fs_read() / supports_fs_write() (already done, keep this)
• Provide ACP-backed read/write closures or trait impls to the developer extension via PlatformExtensionContext
• Send ReadTextFileRequest / WriteTextFileRequest to client when called
• Do NOT intercept tool results, parse URIs, or extract diffs

Developer platform extension (edit.rs):

• EditTools calls read/write through an injected abstraction instead of std::fs directly
• Default: std::fs (when no ACP, or capabilities absent)
• ACP-provided: closures/trait that send JSON-RPC to client
• No knowledge of ACP protocol, goose-internal:// URIs, or JSON-RPC
• No TextFileIOStrategy enum — just "I have a read function and a write function"

Client (editor) — not goose code:

• Receives fs/write_text_file, compares with buffer, shows diff
• Receives fs/read_text_file, returns buffer content

Phase 3: Implementation steps

Step 0: Rebase on main (prerequisite)

• developer__text_editor is gone. TextFileIOStrategy is gone. DeveloperServer is gone.
• Start fresh from edit.rs on main.

Step 1: Make EditTools pluggable (developer extension only, ~50 lines)

• Add read/write closures or a trait to EditTools
• Default to std::fs::read_to_string / std::fs::write
• Thread through DeveloperClient::new() via PlatformExtensionContext
• Files: edit.rs, mod.rs
• Test: existing developer tests still pass with default impls

Step 2: Wire ACP server to provide closures (server only, ~50 lines)

• Parse capabilities (already works from your PR)
• When readTextFile=true: provide a closure that sends ReadTextFileRequest
• When writeTextFile=true: provide a closure that sends WriteTextFileRequest
• Pass closures to developer extension via context
• Files: server.rs, maybe PlatformExtensionContext
• No changes to goose-mcp, extension_manager, builtin_extension, or SpawnServerFn

Step 3: Write in-process ACP tests

Goose already has an in-process test framework for ACP. Tests use OpenAiFixture (mocked LLM with pattern-matched SSE responses) and ClientToAgentConnection (in-process duplex transport). Read #6969 for context on how tests work and recording tips.

The existing test fixture at fixtures/server.rs already handles:

• RequestPermissionRequest via PermissionDecision enum (AllowOnce, RejectOnce, Cancel, etc.)
• SessionNotification collection for asserting tool_call_update status
• OpenAiFixture for canned LLM responses without hitting a real API

You need to extend the fixture to also handle ReadTextFileRequest and WriteTextFileRequest (add handlers in the on_receive_request builder, similar to how RequestPermissionRequest is handled). Then add these test scenarios:

Scenario 1: Read delegation without permission

• Initialize with clientCapabilities.fs.readTextFile = true
• Prompt agent to read a file
• Assert: ReadTextFileRequest received by client with correct path
• Assert: RequestPermissionRequest NOT sent (reads don't need permission)
• Assert: response contains file content

Scenario 2: Write delegation with permission approved

• Initialize with clientCapabilities.fs.writeTextFile = true
• Set PermissionDecision::AllowOnce
• Prompt agent to write content to a file
• Assert: RequestPermissionRequest sent first
• Assert: WriteTextFileRequest received with correct path and content
• Assert: tool_call_update status is completed
• Assert: file exists on disk (client-side handler wrote it)

Scenario 3: Write delegation with permission rejected

• Initialize with clientCapabilities.fs.writeTextFile = true
• Set PermissionDecision::RejectOnce
• Prompt agent to write content to a file
• Assert: RequestPermissionRequest sent
• Assert: WriteTextFileRequest NOT called
• Assert: tool_call_update status is failed
• Assert: file does NOT exist on disk

Scenario 4: Agent-side write (fs disabled)

• Initialize WITHOUT writeTextFile capability
• Set PermissionDecision::AllowOnce
• Prompt agent to write content to a file
• Assert: WriteTextFileRequest NOT sent (agent handles it locally)
• Assert: RequestPermissionRequest still sent (permission still applies)
• Assert: file exists on disk (written by agent via fs::write)

Scenario 5: Agent-side rejection (fs disabled)

• Initialize WITHOUT writeTextFile capability
• Set PermissionDecision::RejectOnce
• Prompt agent to write content to a file
• Assert: file does NOT exist on disk

To record the OpenAI fixture data for these tests, follow the steps in #6969: run goose acp --with-builtin developer with a real provider, capture the SSE responses, and extract request patterns from llm_request.*.jsonl.

Goose-specific protocol values:

• allowOptionId: "allow_once", rejectOptionId: "reject_once"
• rejectedToolStatus: "failed"
• Spawn: goose acp --with-builtin developer

Phase 4: What NOT to recreate

Everything in this list exists only because of the smuggle-diffs-through-MCP approach. Do not recreate any of it:

• ManagedFileWrite struct
• decode_uri_component / encode_uri_component hand-rolled URI codec
• try_extract_file_diff — parsing goose-internal://file-diff/ URIs from embedded resources
• ToolCallContentWithDiffs — separating content from diffs
• build_tool_call_content — intercepting tool results to extract diffs
• Two-phase InProgress→write→Completed notification for diffs
• create_file_diff_content — wrapping content in fake MCP resources
• TextFileIOStrategy enum in goose-mcp
• ReadStrategyFn, TextFileWriteResult types in goose-mcp
• SpawnServerFn 4-parameter signature
• add_extension_with_context on extension manager
• spawn_developer custom function

Why: the server's job is transport. It should not parse custom URI schemes or process file diffs. The agent sends full content via fs/write_text_file; the editor computes the diff. Every editor listed above already does this.

Phase 5: Editors to test with

Don't just test with Zed. Test with multiple editors to validate spec compliance:

• Neovim via acp.nvim, avante.nvim, or codecompanion.nvim (all implement both read + write)
• VS Code via vscode-acp
• deepchat (desktop app)

[codefromthecrypt]

this is a bit strange.. I think if an agent created this to avoid clippy warnings it is probably best to undo it. as fr as I can tell, we create this structure then destructure it immediately.

[codefromthecrypt]

this is mixing abstraction concepts, the server code shouldn't be handling diffs, even if some agents aren't designed perfectly. I raised #7451 to suggest designing first before implementing. In there are some hints and source bases to explore.

The issue with doing things this way is that it is tech debt that takes a lot more attention to undo vs just do cleanly in the first place.

[codefromthecrypt]

server should not have the responsibility for being a URI encoder/decoder. look for existing crates or a utility, or in any case mention why custom decoding is an ACP concern required to implement this.

[codefromthecrypt]

this is a broken abstraction. The server's job is only to push the file capability to our dev extension which consumes it optionally as a trait. When that's missing, it uses the default implementation. What you can see happening here, is server inheriting a mix of concerns which ends up with having tight coupling and confusing code, where the core server handler is now also a diff machine.

[rabi]

Thanks @codefromthecrypt , I think most of the complexity came from bridging ACP through the old goose-mcp developer path, which mixed transport and file-diff concerns across layers.

Now that developer is a platform extension after #7466, ACP can now inject capability-gated read/write delegates directly as you mentioned, keeping server transport-only and letting clients render diffs natively.

I still think developer_file_io delegate on PlatformExtenstionContext would be extension specific leakage.

I'll rebase the PR make it aligned with the new developer platform extension. If there are still concerns from your side around the design, feel free to take over.

[codefromthecrypt]

Good job and thanks for listening @rabi! At a cursory view, this looks very close and I don't mind if someone else approves merges if they feel it is ready. I just don't have time today but I will in the next day or two. pre or post merge.

On my side, I have a local test suite I've been using for fs cap interop testing for the last several months, and will run that against this. it uses nvim avante

Someone myself or otherwise should do a build and re-run the zed flow, you could also do it on own by updating the PR desc similar to mine with exact commands and config you used. here's example #7112

out of date

[codefromthecrypt]

Ran my acp-test suite against this branch (commit a3dda06). Write delegation now passes, nice! But read delegation still fails.

The read_file closure is wired all the way through: build_developer_file_io() creates it, DeveloperFileIo carries it, EditTools stores it. But it's only ever called inside file_edit_with_cwd() to read a file before applying an edit. There's no standalone read tool in the developer extension — the tool list is write, edit, shell, tree.

So when asked to read a file, goose falls back to shell (cat) or computercontroller: cache (view), bypassing ACP delegation entirely.

Is read intentionally left out? The plumbing is all there — it just needs a read tool in mod.rs + a file_read_with_cwd method on EditTools to expose it?

cc @michaelneale as I'm really very out of time and must stop for a day or two

[codefromthecrypt]

TL;DR; add a read tool to developer that uses the existing read_file closure, then make sure there's a test for it that proves dispatch works. That's the cleanest. otherwise we need to merge something incomplete then make another couple PRs to follow-up, something odd because reads are more frequent editor use than writes.

[rabi]

TL;DR; add a read tool to developer that uses the existing read_file closure, then make sure there's a test for it that proves dispatch works.

I think I had initially kept read_text_file capability wiring as a future change for that same reason. Anyway, I think addition of read tool to the dev extension should be a separate PR. But I'll add it here as a separate commit if we want.

[codefromthecrypt]

I have to make an uncomfortable review, and something I very much don't enjoy doing. I think this PR should be abandoned, and I can help raise a new one

--

I applaud @rabi trying to achieve this and his effort put in. However, as a project we have to equally value its future. To add dozens of review and a couple more iterations to merge this and then try to adjust is a far more effort than starting from scratch. There's a lot of library design points that are not coherent. We can see comments special-casing certain paths which effectively means the abstraction is an "if statement" of ACP, needs to be unraveled.

There are also a number of areas where the wiring is complicated due to "developer_file_io" which probably should be named simpler, being optional. The plug in should override the default, meaning there's always a FS plugin, not an optional which causes an if statement in most places.

In this pass, record/replay of openai has been reproduced and doesn't use the mechanism that ACP had been using. We already have too many places in the code that have several implementation deviations, and having deviations in the same file is just more tech debt.

If other folks want to merge this they can, but I would hazard that performing maintenance on this would be their primary vote to merge. I don't have enough time to redo and rework successive PRs, so merging this cannot be implicitly adding immediate refactor to my plate.

I can and will do this feature, I had intentionally deferred it as ACP providers are very much more important than this. However, I can do it to prevent a feeling to need to merge something that will cause others more work in the near future.

[jamadeo]

Sorry I am late to get up to speed here (hadn't seen this PR til now). I definitely think goose should support these edit tools.

I implemented this in #7388 as a separate set of tools. I think that's the way to go, but I'm open to hearing otherwise!

[rabi]

@jamadeo, Hey! Thanks for jumping in. From what I've seen, existing ACP implementations like gemini-cli[1] use the same pattern i.e delegate existing file tool I/O through ACP client capabilities rather than exposing a separate set of ACP-specific tools.

A. Capability negotiation at initialize time
B. Per-capability fallback for readTextFile/writeTextFile independently
C. Session-scoped wiring during newSession/loadSession
D. Same ACP protocol methods (readTextFile, writeTextFile)

Gemini-cli uses an interface/service replacement pattern; goose uses closure injection based on earlier feedback.

@codefromthecrypt, developer_file_io being Option threaded through the stack adds unnecessary wiring is a fair point. I can refactor DeveloperFileIo to always be present (defaulting to default_local()) rather than Option, which would eliminate the None sites and simplify the wiring.

On the "special-casing" concern, in the current code, EditTools calls (self.read_file)(path) and (self.write_file)(path, content) uniformly. There are no ACP-specific if-statements at the tool execution level; the branching happens once at construction time in build_developer_file_io. That said, I'm open to hearing which specific paths you see as problematic.

On test infrastructure -- could you clarify the record/replay concern? I want to make sure the tests align with the existing ACP test patterns.

I'm confused by the shift between the Feb 27 review ("looks very close... I don't mind if someone else approves merges") and now the suggestion to abandon. I want to understand what specifically changed between those reviews beyond the optionality point, so I can address it. This is a feature asked by many users and something we wanted to use internally. I'm always for constructive feedback as the intent is to make goose better and learn in the process. We won't get everything right the first time. Having said that, if we think this implementation is completely off, I'm fine to go with a better implementation you propose.

[1] https://github.com/google-gemini/gemini-cli/blob/main/packages/cli/src/zed-integration/zedIntegration.ts

[codefromthecrypt]

Ps I have a complete impl for this as well, just cant push it until off flight. It corrects all the things mentioned with tests for all 4 options (pass through and delegated read vs write) as well a properly annotated read tool. I have not seen the other PRs since. If the PR I made isnt helpful I will drop it after raising it. However, I think it is worth raising just so that future folks and coding agents alike have a reference.

[codefromthecrypt]

#7388 is the most elegant way out. Thanks for the work on this folks!

I will raise a follow up PR to that to complete test backfill etc.

[codefromthecrypt]

#7668 is the code I had locally as promised.. I will figure out if it is easier to rewrite it or add a commit to switch to the style of #7388