-
Notifications
You must be signed in to change notification settings - Fork 2
feat: set up ACP and Claude adapter #174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
joshsny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - left some comments :)
| this.clientCapabilities = request.clientCapabilities; | ||
|
|
||
| // Default authMethod | ||
| const authMethod: any = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probs should change this?
|
|
||
| const options: Options = { | ||
| systemPrompt, | ||
| settingSources: ["user", "project", "local"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how will this behave if the user has claude code installed locally and has settings there, how do we want it to behave?
Should we inherit their settings, or set this to local and make sure we control the settings in Array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should inherit user settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am presuming most of this is copied from zed's existing adapter?
The code is not great, but if we're copying it over let's not change it as we know it works reasonably well.
Can we leave a comment saying where the code came from if that's the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add this.
| }, | ||
| agentInfo: { | ||
| name: packageJson.name, | ||
| title: "Claude Code", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we give it a different name?
| } | ||
|
|
||
| async authenticate(_params: AuthenticateRequest): Promise<void> { | ||
| throw new Error("Method not implemented."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Is this because we are passing the API key and we don't want to allow the user to do their own authentication?
| } | ||
|
|
||
| async prompt(params: PromptRequest): Promise<PromptResponse> { | ||
| if (!this.sessions[params.sessionId]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work with handing off cloud -> local - I guess we can hydrate the session locally in that case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we'll call loadSession first usually, this is just a safety check
| } | ||
|
|
||
| async function getAvailableModels(query: Query): Promise<SessionModelState> { | ||
| const models = await query.supportedModels(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what request this makes? We'll need to support it in the llm gateway
| }, | ||
| }; | ||
| break; | ||
| case "thinking": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get loading states via this? would be nice to get those messages claude code gives
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For sure, will add it to the list.
| } | ||
| } | ||
|
|
||
| export function runAcp() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what this does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this sets up the communication stream between the ACP agent and the ACP client, and is used to kick off the agent connection
| completed: validatedData.metadata?.completed ?? 0, | ||
| }); | ||
|
|
||
| await this.writeTaskFile(taskId, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of scope of these PRs, but I think this and any other changes that aren't code related should be persisted in S3 and not locally for resumability and handoff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we'll start persisting these by reading the agent plan notifications from the S3 logs instead. https://agentclientprotocol.com/protocol/agent-plan
9063af6 to
b72851b
Compare
Merge activity
|

Based on Zed`s ACP claude adapter: https://github.com/zed-industries/claude-code-acp/tree/main