feat(acp): WebSocket JSON-RPC gateway + extension transport (Phase 3) - #62
feat(acp): WebSocket JSON-RPC gateway + extension transport (Phase 3)#62BOTOOM wants to merge 5 commits into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (acpEnabled()) { | ||
| setError(null); | ||
| setIsSending(true); | ||
| try { | ||
| await acpClient.prompt(sessionId, content); | ||
| } catch (sendError) { | ||
| setError(sendError instanceof Error ? sendError.message : 'ACP prompt failed'); | ||
| } finally { | ||
| setIsSending(false); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔴 Con el modo ACP activo, ningún mensaje del chat llega al agente
El identificador de sesión que usa el panel es el de la sesión creada por la API REST y se envía tal cual al canal ACP (acpClient.prompt(sessionId, content) en apps/extension/src/hooks/useChat.ts:147) sin haber creado antes la sesión en ese canal, así que cada envío falla con un error de sesión desconocida.
Impact: Con la función ACP habilitada, el usuario no puede enviar ningún mensaje: todo termina en error y su propio mensaje tampoco aparece en pantalla.
Desajuste entre las sesiones REST y las sesiones del gateway ACP
- Las sesiones del panel se crean con
apiClient.createSession(apps/extension/src/hooks/useSessions.ts:85), que genera un id en la tablasessionsdel backend. - El gateway solo conoce sesiones creadas mediante
ui/session.create(apps/backend/src/acp/gateway.ts:334-369), que las registra enthis.sessionsy en elAcpSessionManager. AcpClient.createSession(apps/extension/src/services/acp-client.ts:106) existe pero nunca se invoca en la extensión (no hay llamadas fuera de su definición).- En
dispatch,resolveSessionId(apps/backend/src/acp/gateway.ts:326-332) devuelve el valor sin cambios cuando no lo conoce yAcpSessionManager.promptlanzaUnknown DevMentorAI session(apps/backend/src/acp/session-manager.ts:232-238). - Además, en la rama ACP de
sendMessageno se agrega el mensaje del usuario al estado (el reducer solo materializa eventosmessagedel agente,apps/extension/src/services/acp-reducer.ts:33-49), por lo que el texto enviado no se muestra.
Prompt for agents
En modo ACP (`acpEnabled()`), `useChat` envía el prompt usando el id de la sesión REST creada por `useSessions`/`ApiClient.createSession`, pero el gateway solo reconoce sesiones creadas mediante `ui/session.create` (`AcpGateway.createSession`). El resultado es que `manager.prompt` lanza 'Unknown DevMentorAI session' y ningún prompt funciona. Hace falta un mapeo explícito: crear la sesión ACP (via `AcpClient.createSession`) cuando se selecciona/crea una sesión con el flag activo, guardar el id devuelto y usarlo en `prompt`, `cancel` y `replay`; o bien hacer que el backend cree la sesión ACP bajo demanda cuando llega un prompt con un id de sesión REST conocido. Además, en la rama ACP no se inserta el mensaje del usuario en el estado del chat (el reducer solo procesa eventos del agente), por lo que el mensaje enviado no aparece en la UI.
Was this helpful? React with 👍 or 👎 to provide feedback.
| useEffect(() => { | ||
| if (!acpEnabled()) return; | ||
| const unsubscribe = acpClient.onEvent((eventSessionId, _seq, event) => { | ||
| if (eventSessionId === sessionId) dispatchAcpEvent({ sessionId: eventSessionId, event }); | ||
| }); | ||
| void acpClient.connect().catch((connectError: unknown) => { | ||
| setError(connectError instanceof Error ? connectError.message : 'ACP connection failed'); | ||
| }); | ||
| return unsubscribe; | ||
| }, [acpClient, sessionId]); |
There was a problem hiding this comment.
🟡 Al cambiar de conversación se siguen mostrando los mensajes de la anterior
El historial acumulado del canal ACP nunca se reinicia al cambiar de conversación (el reductor se crea una sola vez en useReducer(...) en apps/extension/src/hooks/useChat.ts:47-52 y el efecto de cambio de sesión solo re-suscribe), así que los mensajes de la conversación previa siguen visibles en la nueva.
Impact: El usuario ve mezclados en una conversación los mensajes que pertenecen a otra.
Estado del reductor compartido entre sesiones
El efecto de apps/extension/src/hooks/useChat.ts:72-81 depende de sessionId pero solo re-registra el manejador de eventos; no despacha ninguna acción de reinicio. acpState.messages se devuelve directamente como messages cuando el flag está activo (apps/extension/src/hooks/useChat.ts:542), y el reductor (apps/extension/src/services/acp-reducer.ts:17-58) no tiene ninguna acción de limpieza ni compara el sessionId almacenado. En la ruta no-ACP el estado sí se recarga por sesión (loadMessages).
Prompt for agents
El estado del reductor ACP (`acpState`) persiste entre cambios de sesión en `useChat`. Cuando `sessionId` cambia, los mensajes acumulados de la sesión anterior se siguen mostrando porque `messages` devuelve `acpState.messages` sin filtrar ni reiniciar. Posibles enfoques: añadir una acción de tipo 'reset' al reductor de `apps/extension/src/services/acp-reducer.ts` y despacharla en el efecto que depende de `sessionId`, o almacenar el estado por sesión (mapa sessionId -> AcpChatState) y seleccionar el de la sesión activa.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async shutdown(): Promise<void> { | ||
| for (const timer of this.timers.values()) clearTimeout(timer); | ||
| this.timers.clear(); | ||
| await this.manager.shutdown(); | ||
| for (const client of this.clients) client.socket.close(); | ||
| this.clients.clear(); | ||
| } |
There was a problem hiding this comment.
🟡 Procesos de agentes quedan vivos tras apagar el servidor
Al cerrar el gateway no se cierran las conexiones de agente abiertas por el servicio de agentes (shutdown() en apps/backend/src/acp/gateway.ts:186-192 solo apaga el gestor de sesiones y los sockets), por lo que esos procesos externos siguen ejecutándose tras el apagado.
Impact: Al detener el backend pueden quedar procesos de agente huérfanos consumiendo recursos.
Conexiones creadas por AcpAgentService no se cierran
AcpAgentService.authenticate (apps/backend/src/acp/catalog/agent-service.ts:133-154) crea y guarda instancias de AgentConnection en su propio mapa connections, que solo se cierran con AcpAgentService.shutdown() (apps/backend/src/acp/catalog/agent-service.ts:164-167). El gateway expone ui/agents.authenticate (apps/backend/src/acp/gateway.ts:313-318) pero su shutdown() nunca llama a this.agentService.shutdown(); solo this.manager.shutdown(), que apaga las conexiones registradas por createSession.
| async shutdown(): Promise<void> { | |
| for (const timer of this.timers.values()) clearTimeout(timer); | |
| this.timers.clear(); | |
| await this.manager.shutdown(); | |
| for (const client of this.clients) client.socket.close(); | |
| this.clients.clear(); | |
| } | |
| async shutdown(): Promise<void> { | |
| for (const timer of this.timers.values()) clearTimeout(timer); | |
| this.timers.clear(); | |
| await this.manager.shutdown(); | |
| await this.agentService.shutdown(); | |
| for (const client of this.clients) client.socket.close(); | |
| this.clients.clear(); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| connect(): Promise<void> { | ||
| if (this.socket?.readyState === WebSocket.OPEN) return Promise.resolve(); | ||
| return new Promise((resolve, reject) => { | ||
| const socket = new WebSocket(this.url); | ||
| this.socket = socket; | ||
| socket.onopen = () => { | ||
| for (const handler of this.connectionHandlers) handler(true); | ||
| resolve(); | ||
| void this.replayKnownSessions(); | ||
| }; | ||
| socket.onerror = () => reject(new Error('ACP WebSocket connection failed')); | ||
| socket.onmessage = (message) => { | ||
| void this.handleMessage(message.data); | ||
| }; | ||
| socket.onclose = () => { | ||
| for (const pending of this.pending.values()) | ||
| pending.reject(new Error('ACP WebSocket closed')); | ||
| this.pending.clear(); | ||
| for (const handler of this.connectionHandlers) handler(false); | ||
| if (this.reconnect) this.scheduleReconnect(); | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🟡 Se pueden abrir conexiones duplicadas al servidor y quedar una colgada
La conexión solo se reutiliza si ya está totalmente abierta (comprobación readyState === WebSocket.OPEN en apps/extension/src/services/acp-client.ts:75), así que una segunda llamada mientras la primera aún se está estableciendo crea otra conexión y abandona la anterior.
Impact: Se abren conexiones extra al backend que nunca se cierran, y las peticiones en curso pueden fallar o reconectar innecesariamente.
Estado CONNECTING no considerado en connect()
connect() (apps/extension/src/services/acp-client.ts:74-96) solo devuelve temprano cuando readyState === OPEN. Como request() (apps/extension/src/services/acp-client.ts:129-136) llama a await this.connect() en cada petición y el efecto de montaje también llama a connect() (apps/extension/src/hooks/useChat.ts:77), una petición emitida durante el handshake instancia un segundo WebSocket y sobreescribe this.socket. El primer socket queda abierto sin referencia; cuando se cierre disparará onclose, rechazando las peticiones pendientes del mapa compartido y programando una reconexión. Conviene guardar la promesa de conexión en curso y reutilizarla (o tratar CONNECTING esperando el evento open).
Prompt for agents
En `AcpClient.connect()` (apps/extension/src/services/acp-client.ts) solo se reutiliza el socket si ya está en estado OPEN. Si se llama a `connect()` mientras el socket está en CONNECTING (por ejemplo, desde `request()` justo después del `connect()` del efecto de montaje en useChat), se crea un segundo WebSocket y se sobrescribe `this.socket`, dejando el primero huérfano y provocando rechazos de peticiones pendientes y reconexiones espurias cuando ese socket huérfano se cierre. Una solución es memorizar la promesa de conexión en curso y devolverla mientras el socket esté en CONNECTING, limpiándola en open/close/error.
Was this helpful? React with 👍 or 👎 to provide feedback.
| onRequest: async (request: FastifyRequest, reply: FastifyReply): Promise<void> => { | ||
| if (!this.isOriginAllowed(request.headers.origin)) { | ||
| reply.code(403).send({ error: 'WebSocket origin is not allowed' }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🔍 El hook onRequest de rechazo por origen no devuelve la respuesta
En el hook onRequest asíncrono se llama a reply.code(403).send(...) sin devolver reply. En Fastify, al responder desde un hook async se debe devolver el objeto reply para detener la cadena; de lo contrario el ciclo puede continuar y generar avisos de 'reply already sent'. En la práctica el impacto se mitiga porque el propio manejador vuelve a comprobar el origen y cierra el socket con código 1008 (apps/backend/src/acp/gateway.ts:158-164), y el test de origen rechazado pasa por esa segunda barrera. Aún así conviene devolver reply para que el rechazo del upgrade sea determinista.
Was this helpful? React with 👍 or 👎 to provide feedback.
| db.exec( | ||
| "UPDATE sessions SET imported_from = 'copilot-sdk' WHERE imported_from IS NULL AND agent_id IS NULL" | ||
| ); |
There was a problem hiding this comment.
🔍 Migración que marca sesiones como importadas en cada arranque
El UPDATE ... SET imported_from='copilot-sdk' WHERE imported_from IS NULL AND agent_id IS NULL se ejecuta en cada llamada a initDatabase, no solo una vez tras la migración. Cualquier sesión creada por la ruta Copilot después de este cambio quedará marcada como 'importada' en el siguiente arranque, lo que puede confundir a la UI que use importedFrom para distinguir historial heredado de sesiones nuevas.
Was this helpful? React with 👍 or 👎 to provide feedback.
| onRequest: async (request: FastifyRequest, reply: FastifyReply): Promise<void> => { | ||
| if (!this.isOriginAllowed(request.headers.origin)) { | ||
| reply.code(403).send({ error: 'WebSocket origin is not allowed' }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🟥 El rechazo por origen en el hook de upgrade no interrumpe el ciclo de vida de la petición
El hook onRequest responde 403 para orígenes no permitidos pero, al ser asíncrono, no devuelve reply, por lo que Fastify puede continuar la cadena y permitir la actualización a WebSocket. La comprobación redundante en el manejador (apps/backend/src/acp/gateway.ts:159-162) mitiga el problema cerrando el socket con código 1008, pero la defensa principal del allowlist de orígenes queda debilitada; si esa segunda comprobación desapareciera, cualquier página web podría conectarse a localhost y controlar los agentes (los WebSockets no están sujetos a CORS).
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Revisión atendida en |
1 participant
Description
Phase 3, stacked on #61 → #60 → #59. Connects the ACP host built in Phases 1–2 to the side panel. Everything is behind
ACP_ENABLED, default false: with the flag off the gateway isn't registered, the server keeps its current binding, anduseChatstill runs the Copilot SSE path unchanged.Why a WebSocket replaces SSE (ADR-0003): SSE is one-way, but an ACP agent issues requests to the client mid-turn —
session/request_permissionblocks the turn until the user answers. That direction can't be faked over SSE + POST without inventing correlation, so/acpspeaks JSON-RPC 2.0 in both directions with correlated ids and timeouts. Method surface:ui/session.*(create, prompt, cancel, close, set_config_option),ui/agents.*forwarded verbatim to the Phase 2 facade,ui/session.eventnotifications carrying theAcpEventunion, and backend→UIui/permission.request. The gateway is transport only — protocol logic stays in the Phase 1 session manager.WebSockets are not subject to CORS, so without a check any page the user visits could open a socket to
localhost:3847and drive their agents. The upgrade enforces anOriginallowlist (the extension origin plus configured dev origins) and the ACP listener binds to loopback.Reconnect is the normal case, not an edge case — the browser destroys and recreates the side panel constantly. Every event carries a per-session monotonic sequence number; the client reconnects with its last seen seq and receives exactly the missing events, and is told explicitly when it reconnected too late for the bounded buffer instead of silently seeing a hole. Pending permission requests survive the reconnect and re-route to the new socket, so a turn blocked on the user doesn't deadlock when the panel closes.
Also: configurable, no-longer-Copilot-specific idle/request timeouts that end a stalled turn recoverably (R-022); additive
sessionsmigration per architecture §6 with pre-migration rows markedimportedFrom: 'copilot-sdk'and nothing deleted (R-049); messages still written to the existing tables, now explicitly a display cache rather than the source of truth; a minimal permission card rendering the agent's own options (Phase 4 owns the polished UX); and a default profile seeded on first run so the flagged path isn't a dead end before the Phase 5 catalog UI exists — an unauthenticated agent then lands on theauth_requiredstate built in Phase 2.Tests drive a real
wsclient against a real listening server (ephemeral loopback port) talking to the Phase 1 fixture agent: full streamed turn, cancel, permission round-trip, reconnect mid-turn asserting exact contiguous replay, recoverable idle timeout, disallowed origin, profile-less session fallback. The test DB is built from the app's real migration rather than duplicated DDL. Two notes for reviewers: this file runs in vitest'sforkspool becausebetter-sqlite3was being finalized after worker teardown inthreads(Assertion failed: (env) != nullptrinStatement::~Statement()); and the reducer currently rendersmessage/state/erroronly, retaining other event types untouched — tool calls, plans, commands, config and usage get their surfaces in Phase 4, which is the MVP.Requirements: R-020, R-022, R-023 (partial), R-045, R-049.
Type of Change
Related Issues
Stacked on #61 → #60 → #59.
Checklist
pnpm typecheck)pnpm lint) — scoped Biome clean; repo-wide run is pre-existing-red on unrelated filespnpm test) — backend 173, extension 169; E2E skipped (no Chromium here)Screenshots (if applicable)
n/a
Link to Devin session: https://app.devin.ai/sessions/bab32da5729e4a95a9cb79f1648f005e
Requested by: @BOTOOM