feat(backend): ACP v1 client layer + fixture agent (Phase 1) - #60
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}
| } catch (error) { | ||
| const acpError = | ||
| error instanceof AcpError ? error : new AcpError('agent_error', String(error)); | ||
| await this.emit(sessionId, { type: 'error', error: acpError.toPayload() }); | ||
| throw acpError; | ||
| } |
There was a problem hiding this comment.
🟡 Un turno que falla queda marcado como "en ejecución" para siempre
Cuando un turno falla, sólo se avisa del fallo (emit(... type: 'error') en apps/backend/src/acp/session-manager.ts:119) y nunca se avisa de que el turno terminó, así que quien escucha la conversación la sigue viendo como activa indefinidamente.
Impact: Tras un error del agente la interfaz se queda con el indicador de "pensando" encendido y no permite saber que el turno acabó.
Máquina de estados sintetizada en v1: falta el `state: idle` en la ruta de error
En v1 el host sintetiza el ciclo de vida del turno: emite { type: 'state', state: 'running' } antes de session/prompt (apps/backend/src/acp/session-manager.ts:101) y { type: 'state', state: 'idle', stopReason } cuando la respuesta llega (apps/backend/src/acp/session-manager.ts:111-115).
Si prompt() rechaza (caída del agente, error JSON-RPC, timeout), el catch sólo emite un evento error y relanza; nunca se emite idle. El consumidor documentado en docs/specs/acp/02-architecture.md reduce estos eventos a un estado de sesión, por lo que la sesión queda en running permanentemente. Además, en esa ruta tampoco se limpian las llamadas a herramientas activas (session.activeToolCalls), que quedarán marcadas como en curso y podrán cancelarse erróneamente en un turno posterior.
| } catch (error) { | |
| const acpError = | |
| error instanceof AcpError ? error : new AcpError('agent_error', String(error)); | |
| await this.emit(sessionId, { type: 'error', error: acpError.toPayload() }); | |
| throw acpError; | |
| } | |
| } catch (error) { | |
| const acpError = | |
| error instanceof AcpError ? error : new AcpError('agent_error', String(error)); | |
| await this.emit(sessionId, { type: 'error', error: acpError.toPayload() }); | |
| await this.emit(sessionId, { type: 'state', state: 'idle' }); | |
| throw acpError; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (event.type === 'tool_call') { | ||
| if ( | ||
| event.status === 'completed' || | ||
| event.status === 'failed' || | ||
| event.status === 'cancelled' | ||
| ) { | ||
| session.activeToolCalls.delete(event.toolCallId); | ||
| } else { | ||
| session.activeToolCalls.add(event.toolCallId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Una herramienta ya finalizada vuelve a contarse como pendiente y se cancela por error
Cualquier actualización de una herramienta que no incluya estado se contabiliza como pendiente (session.activeToolCalls.add(...) en apps/backend/src/acp/session-manager.ts:179) aunque ya hubiera terminado, de modo que al cancelar el turno se la marca como cancelada.
Impact: Una herramienta que ya se completó puede aparecer como cancelada en el historial de la conversación.
Clasificación por `status` en `handleUpdate`
normalizeV1Update sólo incluye status si el session/update lo trae (apps/backend/src/acp/normalize/v1.ts:124). En ACP v1 un tool_call_update es un upsert parcial: puede traer sólo content, locations o rawOutput sin status.
En handleUpdate la rama else (apps/backend/src/acp/session-manager.ts:178-180) trata "sin status" igual que "en curso" y vuelve a insertar el toolCallId en activeToolCalls después de que un completed/failed/cancelled previo lo hubiera eliminado. Si el turno termina luego con stopReason: 'cancelled', cancelUnfinishedTools (apps/backend/src/acp/session-manager.ts:187-197) emitirá un tool_call con status: 'cancelled' para esa herramienta ya finalizada, justo el escenario que el PR dice evitar.
Solución: sólo añadir a activeToolCalls cuando status esté presente y sea no terminal (o cuando el evento sea el tool_call inicial).
Prompt for agents
En apps/backend/src/acp/session-manager.ts, handleUpdate clasifica los eventos tool_call en activos/terminados usando event.status. Como normalizeV1Update omite status cuando el session/update no lo trae (actualizaciones parciales de tipo upsert, p.ej. sólo content o rawOutput), la rama else vuelve a insertar el toolCallId en activeToolCalls aunque la herramienta ya se hubiera completado o fallado. Después, cancelUnfinishedTools emitiría un estado 'cancelled' para una herramienta ya terminada. Ajusta la clasificación para que un evento sin status no reactive una herramienta: por ejemplo, sólo añadir al conjunto cuando status esté definido y no sea terminal.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async closeSession(sessionId: string): Promise<void> { | ||
| const session = this.requireSession(sessionId); | ||
| try { | ||
| await this.requireAgent(session.agentId).closeSession(session.acpSessionId); | ||
| } finally { | ||
| this.sessions.delete(sessionId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Cerrar una sesión que el agente no soporta cerrar la elimina igualmente en el servidor
La sesión se borra del registro local (this.sessions.delete(sessionId) en apps/backend/src/acp/session-manager.ts:147) también cuando la petición de cierre al agente falla, de modo que la conversación queda huérfana: viva en el agente pero inexistente para nosotros.
Impact: Tras un cierre fallido la conversación deja de ser accesible aunque el agente la siga manteniendo abierta, y se filtran recursos en el agente.
`finally` incondicional en `closeSession`
AgentConnection.closeSession lanza capability_unsupported sin llegar a enviar nada cuando el agente no anuncia la capacidad close (apps/backend/src/acp/connection.ts:253-256), y también propaga cualquier error JSON-RPC. Como el delete está en un finally, el estado local se destruye igual, y el llamador recibe un error sobre una sesión que ya no puede reintentar ni consultar (requireSession lanzará Unknown DevMentorAI session).
Lo esperable es eliminar la sesión sólo cuando el cierre tuvo éxito, o bien comprobar la capacidad antes y decidir explícitamente si se descarta el estado local.
| async closeSession(sessionId: string): Promise<void> { | |
| const session = this.requireSession(sessionId); | |
| try { | |
| await this.requireAgent(session.agentId).closeSession(session.acpSessionId); | |
| } finally { | |
| this.sessions.delete(sessionId); | |
| } | |
| } | |
| async closeSession(sessionId: string): Promise<void> { | |
| const session = this.requireSession(sessionId); | |
| await this.requireAgent(session.agentId).closeSession(session.acpSessionId); | |
| this.sessions.delete(sessionId); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| async connect(): Promise<AcpConnectionCapabilities> { | ||
| if (this.connection) return this.capabilities; | ||
| this.closing = false; |
There was a problem hiding this comment.
🟡 Dos peticiones simultáneas de arranque del agente hacen fallar a la segunda
Si se pide arrancar el agente mientras otro arranque sigue en curso, la segunda llamada devuelve las capacidades antes de que existan (return this.capabilities en apps/backend/src/acp/connection.ts:132), lo que provoca un error de "conexión no inicializada".
Impact: Crear dos sesiones a la vez sobre el mismo agente puede fallar de forma aleatoria con un error de inicialización engañoso.
Falta de deduplicación del handshake en `AgentConnection.connect`
connect() asigna this.connection antes de esperar a initialize (apps/backend/src/acp/connection.ts:179-187) y sólo asigna this._capabilities cuando la respuesta llega (apps/backend/src/acp/connection.ts:198). Una segunda llamada concurrente entra por el atajo if (this.connection) return this.capabilities;, y el getter capabilities lanza AcpError('agent_launch_failed', 'ACP connection is not initialized') (apps/backend/src/acp/connection.ts:108-113).
Es alcanzable desde AcpSessionManager.createSession, que llama a connectAndGetAgent en cada creación (apps/backend/src/acp/session-manager.ts:76 y 218-222); dos createSession en paralelo sobre el mismo agentId disparan el fallo. La solución habitual es memorizar la promesa del handshake y que las llamadas concurrentes la esperen.
Prompt for agents
AgentConnection.connect en apps/backend/src/acp/connection.ts no deduplica handshakes concurrentes: asigna this.connection antes de esperar la respuesta de initialize y usa 'if (this.connection) return this.capabilities;' como atajo. Una segunda llamada concurrente entra por ese atajo mientras _capabilities aún es undefined y el getter capabilities lanza 'ACP connection is not initialized'. Esto es alcanzable desde AcpSessionManager.createSession (llama a connect en cada creación). Propuesta: guardar la promesa en curso del connect (p.ej. un campo connectPromise) y devolverla a los llamadores concurrentes, limpiándola en caso de error.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async shutdown(timeoutMs = 1_000): Promise<ProcessExit | undefined> { | ||
| if (this.exitResult) return this.exitResult; | ||
| this.kill('SIGTERM'); | ||
| const timeout = new Promise<undefined>((resolve) => { | ||
| setTimeout(() => { | ||
| if (!this.exitResult) this.kill('SIGKILL'); | ||
| resolve(undefined); | ||
| }, timeoutMs).unref(); | ||
| }); | ||
| return (await Promise.race([this.exited, timeout])) ?? this.exitResult; | ||
| } |
There was a problem hiding this comment.
🔍 El apagado del proceso puede resolverse antes de que el hijo muera realmente
AgentProcess.shutdown hace Promise.race([this.exited, timeout]); si vence el temporizador, se envía SIGKILL y se resuelve inmediatamente con undefined ?? this.exitResult (también undefined), sin esperar a la salida efectiva. AgentLauncher.shutdown (apps/backend/src/acp/launcher.ts:128-130) espera esas promesas, así que el apagado del backend puede considerarse completo con procesos agente aún vivos — justo lo que exige R-007 AC3 ("no orphans remain") en docs/specs/acp/03-requirements.md. Sería más robusto esperar this.exited después del SIGKILL (con un segundo margen) en lugar de resolver al vencer el temporizador.
Was this helpful? React with 👍 or 👎 to provide feedback.
| launch(spec: LaunchSpec, stderrLimit = DEFAULT_STDERR_LIMIT): AgentProcess { | ||
| let child: ChildProcessWithoutNullStreams; | ||
| try { | ||
| child = spawn(spec.cmd, spec.args ?? [], { | ||
| cwd: spec.cwd, | ||
| env: { | ||
| ...process.env, | ||
| ...spec.env, | ||
| }, | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
| } catch (error) { | ||
| throw new AcpError('agent_launch_failed', `Failed to launch ${spec.cmd}`, { | ||
| cause: error instanceof Error ? error.message : String(error), | ||
| command: spec.cmd, | ||
| }); | ||
| } | ||
|
|
||
| const agentProcess = new AgentProcess(child, stderrLimit); | ||
| this.processes.add(agentProcess); | ||
| void agentProcess.exited.finally(() => this.processes.delete(agentProcess)); | ||
| return agentProcess; | ||
| } |
There was a problem hiding this comment.
🔍 spawn con comando inexistente no lanza de forma síncrona: connect() podría quedar colgado
El try/catch alrededor de spawn sólo captura errores síncronos (argumentos inválidos); un ENOENT por comando inexistente llega como evento error asíncrono, que aquí se convierte en una resolución de exited (apps/backend/src/acp/launcher.ts:61-68). En ese caso AgentConnection.connect ya habrá enviado initialize y depende de que el SDK rechace las peticiones pendientes al cerrarse el stream; si no lo hace, connect() no resuelve nunca y sólo se notifica por onAgentCrash. Conviene verificar con el SDK 1.3.0 y, en cualquier caso, añadir un timeout de handshake o cortar connect() cuando process.exited se resuelva antes de la respuesta.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Revisión atendida en |
1 participant
Description
Phase 1 of the ACP migration (spec: #59, which this PR is stacked on). Adds the ACP host layer in
apps/backend/src/acp/and nothing else — no routes call it, no UI touches it, the Copilot SDK path is untouched.@agentclientprotocol/sdkis pinned to1.3.0and, by test, importable only fromapps/backend/src/acp/**.The layer is v1-only on purpose (ADR-0001) — the SDK's stable entrypoint is
PROTOCOL_VERSION === 1; v2 lands in Phase 9 behind a flag as a second normaliser.launcher.ts— spawns an agent over stdio intondJsonStream, bounded stderr ring buffer,SIGTERM→SIGKILLshutdown, and a registry so backend shutdown leaves no orphans.connection.ts— one process per agent:initialize, strict version check (anything but 1 →protocol_version_unsupported, no session), capability record, and the client-side handlers. Permissions are deny-by-default:requestPermissionpicks the agent's reject option unless a policy is injected — replacing today'sapproveAll, with the real prompt arriving in Phase 4.session-manager.ts— our session id →{ agentId, acpSessionId, cwd, protocolVersion, capabilities, configOptions }, multiplexing many sessions over one connection. Update routing is keyed by(agentId, acpSessionId), since two agents may hand out the same session id string.normalize/v1.ts—session/update→ the internalAcpEventunion (packages/shared). Unknown variants, content types,kind/statusvalues and_-prefixed extensions are preserved as generic events rather than dropped or thrown on, because-32601/unknown ≠ broken.errors.ts+capabilities.ts— the error taxonomy, and gating so we never call a method or send content the agent didn't advertise.Three v1 quirks the host has to paper over, all measured against real agents during the spike:
state_update, so the host synthesisesstate: runningon prompt dispatch andstate: idlecarrying thestopReasonfrom the prompt response.messageId, so ids are minted per turn and per role (agent-provided ids win); otherwise every turn of a session collapses into one message bubble.session/cancel, and the turn only ends at the prompt response — so unfinished tool calls are markedcancelledwhen the turn resolves withstopReason: 'cancelled', never eagerly, and a tool call the agent completes after the cancel is not overwritten.Testing is against an SDK-built fixture agent (
acp/fixtures/) driving the real wire protocol rather than mocks of our own shapes: full turn, tool-call lifecycle, permission round-trip, cancel ordering, crash surfacingagent_crashedwithout taking the backend down, two agents with colliding session ids not cross-talking, and shutdown leaving no orphan processes — plus golden-file normalisation tests per update variant.Requirements covered: R-001..R-007, R-012 (platform resolution helper), R-060.
Type of Change
Related Issues
Stacked on #59 (the approved spec).
Checklist
pnpm typecheck)pnpm lint) — scoped Biome run over the new files is clean; the repo-wide run is red onmasterwith 38 pre-existing errors in unrelated files, untouched herepnpm test) — backend 153, extension 166; E2E skipped (no Chromium on this machine)Screenshots (if applicable)
n/a
Link to Devin session: https://app.devin.ai/sessions/bab32da5729e4a95a9cb79f1648f005e
Requested by: @BOTOOM