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 · desktop
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
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 854229d. Configure here.
| connectionId: args.connectionId, | ||
| credentialHash: args.connection.credentialHash, | ||
| request: args.request, | ||
| requestId, | ||
| requestKey: identifiedRequestKey, | ||
| state: args.state, | ||
| }); | ||
| } | ||
| if (!args.authenticated) { |
There was a problem hiding this comment.
ACP consent URL binds attacker connection to victim account
Browser completion of ACP authenticate only requires a logged-in dashboard session and same-origin POST, not proof that the approver owns the pending connection. An attacker can start authenticate, send the elicitation URL to a victim, and after the victim clicks Connect operate ACP as that user.
Evidence
handleConnectedPoststarts auth withbeginAcpAuthorization(), which stores onlyconnectionId+credentialHashand returns a URL/api/acp/auth/${transactionId}over the attacker's SSE stream.handleDashboardAcpAuthorizationcompletes on any authorized Google dashboard session after a same-origin confirm POST; it never readsjunior_acp_connectionor otherwise binds the browser to the ACP client.completeAcpAuthorization()thenbindAcpConnectionUser()attaches the victim User to the attacker's connection using the stored hash, so latersession/new/session/promptrun as that user.
Identified by Warden · security-review · 6NQ-3R9
| } | ||
| const heartbeat = options.keepAlive | ||
| ? setInterval( | ||
| () => void state.extendLock(lock, ttlMs), |
There was a problem hiding this comment.
withLock keepAlive heartbeat discards rejected extendLock Promise
The keepAlive heartbeat callback fires void state.extendLock(lock, ttlMs) without a rejection handler. If the state adapter throws on transient Redis or network failures, it becomes an unhandled promise rejection that crashes the Node process under default behavior.
Evidence
withLock(..., { keepAlive: true })is called fromtransport.ts(lines 304, 376, 505, 553) andauth.ts(lines 277, 327) for every mutating ACP operation.state.extendLockwraps Redis/network I/O and propagates adapter connection and command errors without catching them (seecreateConnectingStateAdapterinpackages/junior/src/chat/state/adapter.ts).() => void state.extendLock(lock, ttlMs)evaluates the async call and discards the returned Promise with no.catch()handler, so any rejection is unhandled.- Node.js default since v15 is to throw on unhandled promise rejections, which crashes the process.
- The same transport file correctly handles this for its SSE lease heartbeat by writing
void retainLease().catch(fail)(line 809), confirming the pattern needed here.
Identified by Warden · code-review · AQH-NMV
|
|
||
| function cookieAttributes(request: Request): string { | ||
| return [ | ||
| "Path=/api/acp", |
There was a problem hiding this comment.
Empty cookie value terminates loop and shadows later valid credential
cookieValue returns undefined on the first matching empty-valued cookie, exiting the loop instead of continuing to search for a non-empty match. If a browser or proxy sends the same cookie name twice (e.g. a cleared sub-path cookie before a valid parent-path cookie), hasAcpConnectionCredential incorrectly returns false and the ACP request is rejected as unauthorized.
Evidence
cookieValuesplits theCookieheader on;and immediately returns when it finds a name match.return value || undefinedexits on an empty string, never inspecting remaining parts.- Browsers can legitimately send multiple cookies with the same name when paths differ, and may order a sub-path empty cookie ahead of a valid parent-path one.
requireConnectioncallshasAcpConnectionCredential, so an empty first-match causes a 401 unauthorized response despite a valid credential being present.
Identified by Warden · code-review · RXE-M8H
| state: args.state, | ||
| user: userSchema.parse(args.user), | ||
| }); | ||
| if (binding !== "completed") return binding; |
There was a problem hiding this comment.
completeAcpAuthorization leaves authenticate request hanging on user conflict
When bindAcpConnectionUser returns conflict, completeAcpAuthorization returns it to the dashboard without calling finishAuthorization. The client never receives a completion or error for its pending authenticate request.
Evidence
completeAcpAuthorizationcallback line 265 returns rawbindingwhen it is not"completed", skipping bothfinishAuthorizationanddelete.- The
expiresAtMsbranch directly above (line 247-257) demonstrates the intended contract: non-success outcomes must callfinishAuthorizationto complete the ACP request before returning. bindAcpConnectionUserintransport.tsreturns"conflict"whenconnection.user && connection.user.id !== args.user.id.finishAuthorizationenqueues the final response receipt throughcompleteAcpRequest, which the client consumes over SSE.- Without it, the dashboard renders a conflict error but the ACP elicitation stays open in the stream queue indefinitely.
Identified by Warden · code-review · YCA-Q2N
| if (!isRecord(value)) return value; | ||
| return Object.fromEntries( | ||
| Object.entries(value) | ||
| .sort(([left], [right]) => left.localeCompare(right)) |
There was a problem hiding this comment.
canonicalJson uses localeCompare, producing non-deterministic retry keys across serverless instances
localeCompare makes requestKey vary by runtime locale, so serverless retries can produce different keys and bypass deduplication.
Evidence
canonicalJsonsorts object keys withleft.localeCompare(right)at line 143.- This feeds
requestKey(line 155), which generates astableHexhash used for idempotency inacceptAcpRequest. acceptAcpRequeststores receipts byrequestKey(transport.ts:491) and only skips reprocessing when the key matches exactly._metaandclientCapabilitiesare declared asz.record(z.string(), z.unknown()), allowing arbitrary user keys that may include locale-sensitive Unicode.- Node.js default locale is environment-dependent, so the same payload can sort differently across serverless instances.
Identified by Warden · code-review · W7K-QED
| export async function completeAcpRequest(args: { | ||
| connectionId: string; | ||
| receipt: AcpRequestReceipt; | ||
| requestKey: string; | ||
| state: StateAdapter; | ||
| }): Promise<"completed" | "expired"> { | ||
| const result = await withLock( | ||
| args.state, | ||
| receiptLockKey(args.connectionId, args.requestKey), | ||
| async (lock) => { | ||
| if (!(await readAcpConnection(args.state, args.connectionId))) { | ||
| return "expired" as const; | ||
| } | ||
| const receipt = parseReceipt(args.receipt); | ||
| await fenceLock(args.state, lock, MUTATION_LOCK_TTL_MS); | ||
| await args.state.set( | ||
| receiptKey(args.connectionId, args.requestKey), | ||
| receipt, | ||
| ACP_STATE_TTL_MS, | ||
| ); | ||
| await fenceLock(args.state, lock, MUTATION_LOCK_TTL_MS); | ||
| await queueReceipt({ | ||
| connectionId: args.connectionId, | ||
| receipt, | ||
| requestKey: args.requestKey, | ||
| state: args.state, | ||
| }); | ||
| return "completed" as const; | ||
| }, | ||
| { | ||
| keepAlive: true, | ||
| ttlMs: MUTATION_LOCK_TTL_MS, | ||
| waitMs: LOCK_WAIT_MS, | ||
| }, | ||
| ); | ||
| if (!result.acquired) { |
There was a problem hiding this comment.
completeAcpRequest does not catch AcpStreamFullError thrown by queueReceipt
completeAcpRequest leaves AcpStreamFullError unhandled, so callers treating the function as the sole queueReceipt entry point will crash instead of gracefully handling a full stream.
Evidence
completeAcpRequestcallsqueueReceiptinsidewithLock(line 475) without atry/catchforAcpStreamFullError.queueReceipt->appendStreamOutputthrowsAcpStreamFullErrorwhenexisting.length >= MAX_STREAM_ITEMSand the cursor is not at the last item.- Callers in
auth.ts(finishAuthorization) expect only"completed" | "expired"and do not catchAcpStreamFullError, which causes the OAuth callback to return 500 for a client-side stream-full condition. acceptAcpRequest(line 517) wraps the samequeueReceiptcall in atry/catchand returns"full"onAcpStreamFullError, demonstrating the intended pattern.
Identified by Warden · code-review · KGV-HGD
1 participant
Remote ACP is now an opt-in, serverless-safe transport over Junior's durable Conversation runtime. An ACP session id is the Conversation id. Prompts use the existing mailbox, queue, lease, checkpoint, and event log. Accepted work continues after an SSE disconnect, and
session/loadreplays stored Messages after reconnect.Production authentication uses the existing dashboard Google OAuth session through ACP URL elicitation. The verified account resolves to the canonical Junior User and binds to the ACP connection. Personal tokens and all other
Authorizationheaders are rejected. The browser route now shows an explicit confirmation page, and only a same-originPOSTcan complete the connection. An expired sign-in also returns a terminal JSON-RPC error instead of leaving the ACP authenticate request open.@sentry/junior-acpowns JSON-RPC, SSE, browser authorization, transport state, and theConversationPortit needs. Junior implements the six port operations in one adapter. ACP is not a Junior plugin, and the plugin API has no ACP contract. Core loads the ACP runtime only when the app enables it. Core may import ACP types, but the architecture check still rejects static ACP runtime imports.The core runtime changes are limited to two reusable API Turn capabilities. The Conversation mutation lock can admit and append a mailbox Message only when no runnable work exists. Durable cancellation stores only the active
turnIdand cancellation flag in the existingStateAdapter. ACP keeps its request receipts, stream items, cursors, and leases in that same adapter, so production uses the existing Redis deployment and needs no process affinity or new service.ACP v1 still depends on live SSE requests. If the hosting request limit closes a stream, the client must create a new connection and load the Conversation. Junior continues accepted work during that disconnect. Review
packages/junior-acpfirst, then the narrow adapter inpackages/junior/src/api/acp-conversations.tsand the generic mailbox and cancellation changes.The clean full suites pass with 2,696 Junior tests and 313 dashboard tests. This includes ordinary Slack Conversation work, API Turn work, reconnect recovery, Google sign-in confirmation, and ACP HTTP behavior.