feat: ACP providers for claude code and codex#6605
feat: ACP providers for claude code and codex#6605codefromthecrypt wants to merge 1 commit intomainfrom
Conversation
|
@michaelneale @baxen @alexhancock I may be out of time this week and I wanted this to be a collaborative thing anyway. if either of you want to work on this branch it would be gratefully accepted. The main thing I found is that I can't really test claude because I downgraded my subscription and so get a wait for problem later. I started testing codex, and first ran into the npx wrapper not working out, so was doign a pattern to cache the rust binary to run directly, similar to how zed works around the same issue. Hopefully somethjign like this works in the future for claude and codex and when things work well enough we can kill the. CLI providers. Anyway I'm done for at least a day I think so it is safe to work on if one feels like it. |
|
nice - tested with claude and codex - did see odd artifacts like: but it did work and call things - nice! |
|
tagging @DOsinga as this adds a crate etc, but seems to work nice. this will replace the CLI providers. |
|
Nice! In https://discord.com/channels/1287729918100246654/1408153538537721966/1463661278202695754 I had moved the ACP server code to the I love the idea of having ACP wrappers for Claude Code and Codex but wonder if there are ones available already we could use? cc @benbrandt |
|
@alexhancock on
So, this uses the same wrappers as zed uses and maintains
I ran into a problem using the npm launched in codex-acp, remember codex-acp is actually a rust binary. Then I inspected the zed code and found they are extracting it from the github repo. So, we are doing the same more or less here. One thing to follow-up on is once the ACP registry is live, this code will be a lot easier. If your question is about us needed mappings, that's because this change is about goose -> acp and we are not making a large change to goose itself. So we need to adapt our concepts of approval mode etc to these providers who are not 100pct consistent. that's because ACP is designed to propagate approval and mode choices all the way to the client. Currently Goose CLI, UI etc is not designed to work with third party modes. Hence, we have some glue that we need until that's the case. OTOH for clients calling into goose, nothing changed here. This PR is about adapting our provider infrastructure so that we can have an option besides completely bespoke "CLI provider" glue. Hope these pointers help! p.s. goose is lacking modularity for shared packages which is why there's crate re-org. If there was a goose-api crate, and types like provider etc were there, we'd not have the dependency cycle issues which forced into goose-acp-server and goose-acp-client. Since goose isn't used as a library formally anyway, this is tech debt we can clean up and prevent most things getting stuck in goose crate for the same reason. |
|
rebase of this is a bear due to intentional session-id tightening. I got it. TL;DR; since ACP agents own the sessions, we need to rejig a bit to not fight over the ID. Right now ACP spec does not allow BYO id you have to get one back from the agent. correlation mapping is not great because ops like terminal cross processes and that would force us to add an external id into the DB. The other way is to rejig so that initial session ID creation algo can be overridden based on the provider type, defaulting to normal session manager. So, for ACP, we create a session then get its id and insert it into the normal session manager. That allows us to have 1-1 correlation of session ID. Next is how to do cold resume of session... depending on agent it is load or resume and there is nuance on how to handle this, but we have the code to handle both. If/when ACP changes the session new/load/resume we can replumb, but anyway while what I describe sounds creaky it isn't more troublesome than permission managing. I have a plan for this and the next push will implement it and update the desc with a diagram how. |
|
Yes we manage the claude and codex wrappers. Though for clarity:
Regarding codex's npx usage... I worked with another team to add npm support. We don't use it ourselves, but I would hope it works. If it needs to be fixed, and you can provide more information, we can also do that so you can use that if you'd rather |
|
@benbrandt on codex-acp I ran into unexplained unresponsiveness, which could be due to internal bootstrapping perhaps. I tried several times despite |
0c4bc41 to
505e47e
Compare
|
update on this. This branch is really expensive to maintain, particularly as there are so many model gaps between the provider design and ACP sessions. Each time I touch this I lose several hours and it still isn't a great bridge between provider and ACP. I'd like folks to pipe in if this should ship as-is or continue at all. I think Provider -> ACP is really awkward and the days spent on this direction while instructive could have been better spent making a whole new API backend. |
| .await | ||
| } | ||
|
|
||
| pub async fn create_session_with_id( |
There was a problem hiding this comment.
provider -> acp since there is no way to "bring your own ID" means that we should accept the ACP agent's view of the ID. in order to resolve the chicken egg, we need to be able to accept an external ID or correlate with one. This change prefers to accept it, so that session_id is coherent, and also that future change for load/resume/list sessions will all have the same ID.
| self.core.stream(session_id, system, messages, tools).await | ||
| } | ||
|
|
||
| async fn create_session( |
There was a problem hiding this comment.
there are some copy/paste cruft to buff out into the base type, but I don't want to invest any more hours on this until I've a sense if we think bolting more and more things on the "provider" type is the right way out. IMHO there shuld be an api crate for v2 types and we can clean up all the things like the session chicken egg in new types and migrate to cleaner types. that would also rid the awkward 2 crates for ACP which we only have due to cyclic type dependencies.
| use crate::providers::base::{MessageStream, PermissionRouting, ProviderUsage, Usage}; | ||
| use crate::providers::errors::ProviderError; | ||
|
|
||
| pub struct AcpProviderCore { |
There was a problem hiding this comment.
this type is only here because we don't have an api crate and depdency cycles abound. I think our normal way would be to fold everything into the goose module because otherwise tangles like this trying to decouple..
this stuff is one of the biggest reasons an api crate would help even if provider wasn't awkward to adapt: we end up stuffing everything into the goose crate to avoid cycles
|
I'm going to flatten this stuff into the goose crate as working around it is a cure worse than the disease. that we had a goose-acp crate at all only worked for the server side as it did't need access to goose core types, which are mixed with impl and cause a lot of tension that results in a bit of a mess. best path out is to just keep adding things to goose core crate until we work out modularity as this isn't about ACP rather ACP is just another example of the same tension, which is inability to implement a provider neatly except inside goose crate. |
cb16395 to
4b78e8f
Compare
|
once this is in probably for real rebase. this PR levels non-acp claude and codex and what they need to stitch MCP through. this will make the ACP variants more straightforward to compare as they are very similar to CLI providers #6972 For this reason I've been toying with us calling the formerly unqualified Provider "LLM Provider" and either CLI or ACP as "Agentic Provider" |
4b78e8f to
919285d
Compare
|
I've had something personal come up, I've pushed the latest code I have and if someone can/desires to take this over please do. I updated the PR desc. |
|
Add Provider implementation that connects to ACP agents (claude-code-acp, codex-acp) over stdio transport with support for MCP extensions, permissions, model listing, and model switching. Signed-off-by: Adrian Cole <adrian@tetrate.io>
919285d to
58c14dd
Compare
Summary
Adds
AcpProvider, aProviderimplementation that connects to ACP agents over stdio transport. This lets Goose use any ACP-compatible agent (claude-code-acp, codex-acp, etc.) as a provider with full support for MCP extensions, permissions, model listing, and model switching.AcpProviderincrates/goose/src/acp/provider.rshandles the ACP client protocol:session/new,session/prompt,session/set_model, notifications, and permission requests.claude_code_acp.rsandcodex_acp.rswire up the concrete provider definitions with mode mapping and permission option IDs.new_session()returnsSessionModelStatefrom the agent'ssession/newresponse, enabling model listing and switching through the existingProvidertrait.set_model()sendssession/set_modelviaUntypedMessage(sacp doesn't have typed support yet).fetch_supported_models()creates a session to retrieve available models from the ACP agent.goose-acp/tests/provider_test.rsexercise the full Client→Provider→Server→OpenAI stack in-process using duplex streams.Type of Change
AI Assistance
Testing
Claude Code ACP
Codex ACP
Related Issues
Relates to #6605
Screenshots/Demos (for UX changes)
N/A