fix(acp): stabilize ACP connection and surface errors to clients#29061
Open
m0n99 wants to merge 2 commits into
Open
fix(acp): stabilize ACP connection and surface errors to clients#29061m0n99 wants to merge 2 commits into
m0n99 wants to merge 2 commits into
Conversation
Contributor
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Contributor
|
The following comment was made by an LLM, it may be inaccurate: SummaryNo duplicate PRs found. PR #29061 appears to be unique in its combination of fixes. There is one related PR that addresses a similar issue domain but is distinct:
|
Contributor
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
…ion is ready The AgentSideConnection constructor calls the toAgent factory before its inner Connection is initialised, so accessing connection.signal synchronously in the Agent constructor crashes. Wrap both the abort listener and startEventSubscription in queueMicrotask to defer until the Connection is available.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue for this PR
Closes #27779
Closes #24494
Related #26378
Related #28453
Related #24481
Type of change
What does this PR do?
Three stability fixes for the ACP agent connection layer:
1. Bind agent lifecycle to
connection.signalThe
Agentconstructor now listens onconnection.signalfor abort events. When the ACP client disconnects (WebSocket close, transport error, etc.), the signal fires and the agent aborts its event subscription loop cleanly. Previously, a client disconnect left the agent'sfor awaitloop hanging indefinitely — no cleanup, no error propagation, just a ghost subscription consuming resources.2. Surface connection errors via
sessionUpdateAdded a
case "session.error"branch inrunEventSubscriptionthat translates SDK error variants into ACPsession/updateframes withstopReason: "error". Before this, when the retry policy was exhausted, the turn would silently stall from the client's perspective: no final error notification and nostopReason. The error classification prefers structured headers (x-llm-error-type,x-llm-error-retryable,x-llm-error-reset-at,retry-after) and falls back to HTTP status-code heuristics when headers are absent.Two
session.errorvariants are intentionally not emitted asagent_error:ContextOverflowError— compaction handles this and the turn can continue on a smaller context window.MessageAbortedError— this is the normal user-stop path; the prompt RPC already reports cancellation viastopReason: "cancelled".3. Add
writeTextFileclient capabilityThe
acpCLI command now declareswriteTextFilein itsclientCapabilitiesso the server can sendwriteTextFilerequests. Without this, the server would silently skip file-write operations.How did you verify your code works?
bun test test/acp/— 13 pass / 0 fail / 39 expect() callsbun turbo typecheck— clean, 0 errors across 21 packagesoxlinton changed files — no new warningsAbortController-basedsignalandclosedpromise to match the new constructor contractScreenshots / recordings
N/A — backend / ACP wire protocol changes only.
Checklist