feat(acp): support serverless remote sessions - #1589
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry-junior[bot]](https://avatars.githubusercontent.com/in/2960336?s=60&v=4){kind=small}
There was a problem hiding this comment.
Direction looks right: sessionId = Conversation id, durable mailbox/worker path, and StateAdapter-backed transport/cancellation instead of inventing a side ACP runtime. The multi-instance recovery coverage is also the right shape.
One real contract gap before I'd merge this:
streamPrompt returns the prompt end_turn as soon as it sees turn_completed, but API Turn control is only cleared in finishCancellationAfterTerminal after mailbox acknowledge. A client that immediately sends the next session/prompt can still observe the previous control as active. The recovery path in acceptPrompt refuses to clear that control while the message remains pending, so the follow-up fails with "already has an active prompt" even though the previous Turn already completed. The old in-process path finished cancellation before returning the prompt result, so this is a back-to-back prompt regression.
Please release durable control at/after the terminal Turn event (or otherwise make admit treat terminal+pending-ack as finished), and add an integration case that sends the next prompt as soon as end_turn arrives without waiting for worker cleanup.
There was a problem hiding this comment.
Looks good to me now. The terminal handoff waits until the mailbox input is acknowledged before emitting the prompt result, so a follow-up cannot overlap retryable work; once acknowledgement is visible, acceptPrompt can safely clear any lagging control and admit the next Turn. The cross-instance integration coverage exercises both sides of that boundary.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
5c023cb to
53f0b1b
Compare
Dashboard visual evidenceMode: path-selected Triggered by:
Component gallery index · desktop
Conversations · desktop
Conversations · mobile
Conversation detail · desktop
Conversation detail · mobile
Conversation detail · focused composer · mobile
Full-page screenshots from the mock dashboard. Not a pixel-diff gate. |
This comment has been minimized.
This comment has been minimized.
53f0b1b to
854229d
Compare
ec9139b to
f07b73b
Compare
f07b73b to
10ec6d6
Compare
10ec6d6 to
4bef923
Compare
4bef923 to
da46b23
Compare
There was a problem hiding this comment.
A couple edge contracts still need tightening:
- Enabling
experimental.acpwithout a mounted dashboard still exposes/api/acp, but every authentication URL points to/api/acp/auth/:id, which is only registered by the dashboard app. That configuration initializes successfully and then all remote sign-ins 404. Please either require an enabled dashboard when ACP is configured or mount the authorization handler independently, with coverage for the supported configuration. acceptAcpRequest()creates the receipt before checking stream capacity. Forsession/prompt, that admits durable mailbox work, thenqueueReceipt()can returnfulland the HTTP request responds 503. The client is told the request failed even though the Turn is running and blocks another prompt. Please reserve/check output capacity before the side effect, or return an accepted response that preserves a reliable way to recover the stored receipt.
Add the ACP transport, browser OAuth flow, dashboard authorization, and Junior conversation adapter on top of the provider-neutral runtime contracts.
da46b23 to
b4ccf63
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b4ccf63. Configure here.
| conversationId, | ||
| state: options.state, | ||
| })) | ||
| ) { |
There was a problem hiding this comment.
Terminal gated on other turns
Medium Severity
readTurn only surfaces a turn’s terminal event when the whole Conversation has no runnable work. That check is conversation-wide, not scoped to the turn being streamed, so a follow-up admitted after cleanup can keep the prior prompt from ever getting end_turn / cancelled while later work stays active. Concurrent connections or a fast reconnect on the same session can hit this window and leave the earlier prompt hanging.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b4ccf63. Configure here.
|
|
||
| function isJsonContentType(contentType: string | null): boolean { | ||
| return ( | ||
| contentType?.split(";", 1)[0]?.trim().toLowerCase() === ACP_JSON_MIME_TYPE |
There was a problem hiding this comment.
isJsonContentType crashes on missing Content-Type due to unguarded array access after optional chain
When the Content-Type header is absent on ACP POST requests, isJsonContentType(null) throws a TypeError because the optional chain short-circuits to undefined, and the subsequent [0] member access is not guarded. This causes the handler to return HTTP 500 instead of the intended 415 Unsupported Media Type.
Evidence
request.headers.get("Content-Type")returnsnullwhen the header is absent.null?.split(";", 1)short-circuits toundefined.[0]onundefinedthrowsTypeError: Cannot read properties of undefined (reading '0').- The outer
catchincreateAcpHttpHandlercatches this and returns HTTP 500 "ACP transport failed", bypassing the intended 415 response.
Identified by Warden · code-review · SJB-S7V
| reserveRoute?: AcpStreamRoute; | ||
| requestKey: string; | ||
| state: StateAdapter; | ||
| }): Promise<"accepted" | "busy" | "full"> { |
There was a problem hiding this comment.
acceptAcpRequest creates side effects on expired connections
acceptAcpRequest never checks whether the ACP connection is still live before calling the side-effecting createReceipt callback, so a retried accept can spawn a Conversation turn and queue output for a connection that was already deleted.
Evidence
completeAcpRequestat line 512 guards every path withreadAcpConnectionand returns"expired"when the connection is gone.acceptAcpRequestat line 548 jumps straight to readingstoredand callingcreateReceipt()without any connection-liveness guard.- The caller (
route.tshandleConnectedPost) checksrequireConnectionbefore invokingacceptAcpRequest, but that check is outside the lock, so the connection can be deleted in the race window before the lock is acquired. - The missing guard means
createReceipt()—which callsconversations.prompt()and creates durable Conversation work—runs for a dead connection, leaving orphaned Turns.
Identified by Warden · code-review · 4CH-62M
| conversations: args.conversations, | ||
| connectionId: args.route.connectionId, | ||
| emit, | ||
| output: item.output, | ||
| sessionId, | ||
| signal: abort.signal, | ||
| state: args.state, | ||
| }); | ||
| } | ||
| if (!delivered || abort.signal.aborted) return; | ||
| await retainLease(); | ||
| if (item.output.kind === "replay") { | ||
| await args.state.set( | ||
| streamItemCompletionKey(args.route.connectionId, { | ||
| itemId: item.id, | ||
| sessionId: args.route.sessionId!, | ||
| }), | ||
| true, |
There was a problem hiding this comment.
Post-emit lease renewal failure causes duplicate delivery on reconnect
After a message, replay, or prompt is successfully emitted, a thrown retainLease() aborts pump() before the stream cursor is updated. The stale cursor causes the same item to be re-delivered when the client reconnects.
Evidence
emit(),streamSessionReplay(), andstreamPrompt()all enqueue SSE data and returntruebeforepump()reaches the firstawait retainLease()on line 943.retainLease()explicitly throws whenextendLockreturnsfalseor whenextendLockthrows after the lease expiry window.- If this call throws, the exception propagates out of
pump(),fail()tears the stream down, and thestreamCursorKeywrite on line 954 is skipped. - On the next SSE connection with the same route,
readStreamCursorreturns the old cursor, so the already-delivered item is still treated as pending and re-emitted. - There is no guard or idempotency check in
streamSessionReplayorstreamPromptto prevent re-emitting the same chunks on a laterpump()invocation.
Identified by Warden · code-review · UM4-644
1 participant
Stacked on #1643. Remote ACP is opt-in and runs on Junior's existing durable Conversation runtime. An ACP session id is the Conversation id. Prompts use the mailbox, queue, lease, checkpoint, and event log, so accepted work continues after an SSE disconnect and
session/loadreplays stored Messages after reconnect.Authentication uses ACP URL elicitation and the existing dashboard Google OAuth session. The client shows a one-time verification code that the signed-in user must enter in the browser before Junior binds the canonical User to the ACP connection. Personal tokens and other
Authorizationheaders do not grant ACP access.@sentry/junior-acpowns JSON-RPC, SSE, browser authorization, and short-lived transport records behind a narrowConversationPort. Junior implements that port in one adapter. Production transport state uses the configured RedisStateAdapter, so requests can reach any app instance without process affinity or a separate ACP service. This PR has no production changes underpackages/junior/src/chat.ACP v1 still requires live SSE requests. If the hosting request limit closes a stream, the client must reconnect and load the Conversation; accepted work keeps running. Review
packages/junior-acpfirst, thenpackages/junior/src/api/acp-conversations.tsand the app mount.