-
Notifications
You must be signed in to change notification settings - Fork 84
session/resume proof of concept #196
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
|
We require contributors to sign our Contributor License Agreement, and we don't have @AObuchow on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
@cla-bot check |
|
We require contributors to sign our Contributor License Agreement, and we don't have @AObuchow on file. You can sign our CLA at https://zed.dev/cla. Once you've signed, post a comment here that says '@cla-bot check'. |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
This adds support for the ACP session/load protocol, enabling clients to reconnect to existing sessions after disconnection. Key changes: - Add loadSession capability declaration in initialize() - Pass session ID to SDK via extraArgs["session-id"] in newSession() to synchronize ACP and SDK session IDs for persistence - Implement loadSession() method that uses SDK's resume option to load the existing conversation history Note: extraArgs["session-id"] is not part of the public SDK API and may break in future versions. The loadSession capability should migrate to session/resume once it's added to the ACP schema. Signed-off-by: Andrew Obuchowicz <[email protected]>
Tests that a session can be resumed after process restart: - Creates a session and establishes context with a stored code - Kills the subprocess to simulate disconnection - Starts a new subprocess and calls loadSession with the original session ID - Verifies the context is preserved by asking about the stored code Includes proper subprocess cleanup using killAndWait to ensure processes fully exit before test completion. Signed-off-by: Andrew Obuchowicz <[email protected]>
063cfda to
59c4c10
Compare
|
I had missed the recent #193 so I rebased my changes off of that and added @benbrandt it seems like you were about to naturally arrive that the same solution proposed by this PR? |
The extraArgs with session-id was already set earlier in the options object (added in commit 3f44543). This duplicate was a merge artifact. Moved the explanatory comment to the correct location. Signed-off-by: Andrew Obuchowicz <[email protected]>
59c4c10 to
01ff678
Compare
|
FYI I've supported resuming in my client by passing the session id through |
|
@AObuchow we are working with the unstable flags, starting with fork, on #183 That one has a more unified way to set the settings for a session, and I think we will expand resume from that one. Thanks for doing this, but I think I will close this one for now to avoid everyone dealing with lots of merge conflicts and we can revisit once fork lands. thanks! |
|
@benbrandt that makes sense to me - thank you for taking a look into this :) |
This PR was made in anticipation of session/resume. Since
session/resumeis not yet officially part of the ACP spec, I implementedsession/loadto illustrate the approach that could be taken. Related to #64 and agentclientprotocol/agent-client-protocol#279 (comment)The key idea is that Claude Code has an undocumented feature: you can pass
extraArgs: {"session-id": <UUID>}to the options when sending the initial prompt, and Claude Code will use that UUID as the session ID. This lets us map the ACP session ID to the Claude Code session ID.I'm not a fan of relying on undocumented behaviour: Anthropic could break this implementation. However, I've been sitting on this idea for a while and haven't seen it mentioned yet in any ACP discussions, so I figured it was worth bringing up :)
If this approach is something that would be worth adding to claude-code-acp, I'd be happy to clean up this PR further and get it merge ready.
Side note: I've been working on my own ACP client since September that I'm excited to eventually unveil, and hope to help contribute to the ACP ecosystem :) Zed Industries & the ACP community are doing an amazing job at bringing the standardization that is needed in the agents space - thank you for all your great work.