feat(acp): ACP v2 readiness behind ACP_V2 (Phase 9) - #68
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}
| const agentCapabilities = | ||
| response.protocolVersion >= 2 | ||
| ? { | ||
| loadSession: Boolean(v2Session), | ||
| promptCapabilities: { | ||
| image: Boolean(v2Prompt?.image), | ||
| audio: Boolean(v2Prompt?.audio), | ||
| embeddedContext: Boolean(v2Prompt?.embeddedContext), | ||
| }, | ||
| } | ||
| : normalizeCapabilities(response.agentCapabilities as AgentCapabilities | null | undefined); |
There was a problem hiding this comment.
🟡 Con la nueva versión del protocolo se pierden las capacidades de sesión, y cerrar sesión o cambiar opciones deja de funcionar
Al traducir las capacidades del agente de la nueva versión solo se conservan historial y tipos de contenido (apps/backend/src/acp/connection.ts:82-92), descartando las capacidades de sesión anunciadas, así que el sistema cree que el agente no sabe cerrar sesiones ni exponer opciones.
Impact: Con la nueva versión activada, cerrar una sesión o cambiar su configuración devuelve siempre un error de capacidad no soportada.
Mecanismo: sessionCapabilities ausente en el mapeo v2
supportsSessionCapability (apps/backend/src/acp/capabilities.ts:29-35) consulta agentCapabilities.sessionCapabilities, que en la rama v2 nunca se rellena (v2 anuncia esas capacidades bajo capabilities.session.*). Por eso closeSession (apps/backend/src/acp/connection.ts:411-420) lanza capability_unsupported para cualquier agente v2, aunque su respuesta de initialize declare session.close. Lo mismo aplica a otras capacidades de sesión consultadas por nombre.
Was this helpful? React with 👍 or 👎 to provide feedback.
| prompt | ||
| ); | ||
| await session.updateChain; | ||
| if (session.protocolVersion >= 2) return; |
There was a problem hiding this comment.
🔍 En v2 no se pre-marcan como canceladas las herramientas en curso
El retorno temprano para v2 salta tanto la síntesis de state: idle (correcto, la envía el agente) como cancelUnfinishedTools. La investigación propia del repo indica que en ambas versiones "the Client should pre-mark unfinished tool calls as cancelled" (docs/specs/acp/01-research.md:52-54), y los tests v1 que verifican ese comportamiento (apps/backend/tests/acp/integration.test.ts:354-362) no tienen equivalente v2. Con un agente v2 que no emita tool_call_update con estado final tras session/cancel, las tarjetas de herramienta quedarían colgadas en pending/in_progress indefinidamente.
Was this helpful? React with 👍 or 👎 to provide feedback.
| case 'tool_call_content_chunk': | ||
| return tool(update, 'append'); |
There was a problem hiding this comment.
🔍 tool_call_content_chunk probablemente pierde el contenido incremental
La rama tool_call_content_chunk reutiliza tool(), que solo acepta content si es un array (toolContent devuelve undefined para cualquier otra cosa, líneas 52-62). Si en v2 el chunk transporta un único ToolCallContent (como ocurre con las variantes *_chunk de mensaje), el contenido se descartaría silenciosamente y el evento tool_call con mode: 'append' llegaría sin datos. Ningún test ni el fixture v2 ejercitan esta variante, por lo que conviene verificar la forma real del esquema.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Revisión atendida en Sobre el pre-marcado de tools en v2: se implementó con registro de tool calls terminadas, de modo que un update parcial sin |
| report.checks.cancel = await connection.probeRequest('session/cancel', { | ||
| sessionId: session.sessionId, | ||
| }); |
There was a problem hiding this comment.
🟡 La comprobación de cancelación del diagnóstico de agentes siempre sale como no soportada
La verificación de cancelación se envía ahora esperando respuesta (probeRequest('session/cancel') en apps/backend/src/acp/probe.ts:111) en lugar de enviarse como aviso sin respuesta, y como los agentes solo atienden esa operación como aviso, la comprobación siempre falla.
Impact: El informe de conformidad de cualquier agente indica que no admite cancelar, aunque sí lo haga.
Mecanismo: session/cancel es notification-only en ACP v1 y v2
En ACP, session/cancel es una notificación (sin id). El agente de fixture lo registra con .onNotification('session/cancel', ...) (apps/backend/src/acp/fixtures/fixture-agent.ts:324) y el fixture v2 igual (apps/backend/src/acp/fixtures/fixture-agent-v2.ts:129). Al enviarlo como request, el SDK del agente responde con error method not found, por lo que probeRequest devuelve { supported: false } de forma permanente. Además AgentConnection.probeNotification (apps/backend/src/acp/connection.ts:354) queda sin usarse, lo que sugiere que el cambio fue accidental.
| report.checks.cancel = await connection.probeRequest('session/cancel', { | |
| sessionId: session.sessionId, | |
| }); | |
| report.checks.cancel = await connection.probeNotification('session/cancel', { | |
| sessionId: session.sessionId, | |
| }); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| session.terminatedToolCalls.add(event.toolCallId); | ||
| } else if ( | ||
| !session.terminatedToolCalls.has(event.toolCallId) && | ||
| (session.protocolVersion >= 2 || event.status !== undefined) | ||
| ) { |
There was a problem hiding this comment.
🟡 Tras cancelar un turno, herramientas que reutilizan identificador quedan marcadas como en curso para siempre
Los identificadores de herramientas ya finalizadas se guardan de forma permanente en la sesión (terminatedToolCalls.add en apps/backend/src/acp/session-manager.ts:199) y nunca se limpian al iniciar un nuevo turno, así que si el agente vuelve a usar el mismo identificador esa herramienta ya no se considera activa.
Impact: Al cancelar un turno posterior, esa herramienta no se marca como cancelada y la interfaz la muestra en progreso indefinidamente.
Mecanismo: el conjunto de finalizadas vive toda la sesión
prompt() reinicia session.messageIds en cada turno (apps/backend/src/acp/session-manager.ts:97-101) pero no activeToolCalls/terminatedToolCalls. La nueva guarda en apps/backend/src/acp/session-manager.ts:200-203 impide volver a añadir a activeToolCalls cualquier toolCallId visto antes con estado completed/failed/cancelled, incluso en un turno distinto. cancelUnfinishedTools (apps/backend/src/acp/session-manager.ts:201-211) itera solo activeToolCalls, por lo que no se emite el evento sintético status: 'cancelled'. El propio fixture reutiliza el id fixture-tool en cada turno, mostrando que la reutilización de ids entre turnos es posible en agentes reales. El conjunto además crece sin límite mientras viva la sesión.
Prompt for agents
El nuevo conjunto terminatedToolCalls en ManagedSession (apps/backend/src/acp/session-manager.ts) evita que una herramienta ya finalizada vuelva a considerarse activa por una actualización parcial posterior, lo cual es correcto dentro del mismo turno. El problema es que el conjunto persiste durante toda la vida de la sesión: prompt() reinicia messageIds pero no activeToolCalls ni terminatedToolCalls, de modo que si un agente reutiliza el mismo toolCallId en un turno posterior, ese tool call nunca entra en activeToolCalls y cancelUnfinishedTools no emitirá el estado 'cancelled' sintético, dejando la herramienta visualmente en curso en la UI. También implica crecimiento de memoria sin límite en sesiones largas. Conviene delimitar el alcance del conjunto al turno (por ejemplo, limpiarlo al comenzar cada prompt) manteniendo la protección contra reactivación por actualizaciones tardías del mismo turno.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const response = (await this.connection.agent.request('initialize', { | ||
| protocolVersion: v2Enabled ? 2 : acp.PROTOCOL_VERSION, | ||
| ...(v2Enabled | ||
| ? { capabilities: { elicitation: { form: {} } } } | ||
| : { clientCapabilities: {} }), | ||
| ...(v2Enabled | ||
| ? { info: { name: this.clientName, version: this.clientVersion } } | ||
| : { clientInfo: { name: this.clientName, version: this.clientVersion } }), | ||
| })) as InitializeResponse; |
There was a problem hiding this comment.
🟡 Con la bandera de la nueva versión activada, los agentes de la versión estable pueden fallar al conectar
Al activar la bandera se envía siempre el saludo inicial con el formato nuevo (capabilities/info en apps/backend/src/acp/connection.ts:259-267) antes de saber qué versión habla el agente, por lo que un agente de la versión estable recibe parámetros que no reconoce.
Impact: Con la bandera activada, agentes que validan estrictamente su entrada dejan de poder conectarse, en contra de la promesa de que se comportan igual.
Mecanismo: negociación de versión con parámetros ya específicos de v2
initialize en ACP v1 exige clientCapabilities (y usa clientInfo), mientras v2 usa capabilities/info. El código elige la forma únicamente por process.env.ACP_V2 === '1', no por la versión negociada, que solo se conoce al recibir la respuesta (apps/backend/src/acp/connection.ts:268-271 acepta que el agente responda 1). Un agente v1 que valide el esquema de InitializeRequest responderá con invalid params y connect() fallará; el fixture v1 del repo no valida, por lo que las pruebas no lo detectan. Además el cliente y el stream usados siguen siendo los de @agentclientprotocol/sdk/experimental/v2 incluso cuando la versión negociada es 1.
Prompt for agents
En AgentConnection.connectInternal (apps/backend/src/acp/connection.ts) la forma de los parámetros de 'initialize' se decide solo por la bandera ACP_V2, enviando capabilities/info (v2) aunque el agente sea v1. En ACP v1 el request initialize requiere clientCapabilities y clientInfo, por lo que un agente que valide su esquema rechazará el handshake cuando la bandera esté activa, rompiendo la garantía de que los agentes v1 se comportan igual con o sin bandera. Posibles enfoques: enviar ambos conjuntos de campos (los v1 y los v2) en el request de negociación, o intentar v2 y reintentar el handshake con la forma v1 (y el cliente/stream v1) si el agente responde con error de parámetros o negocia la versión 1. Considerar también que tras negociar versión 1 se sigue usando el cliente y el ndJsonStream del paquete experimental v2.
Was this helpful? React with 👍 or 👎 to provide feedback.
| await expect(connection.connect()).resolves.toMatchObject({ protocolVersion: 2 }); | ||
| expect(connection.capabilities.agentCapabilities.elicitation).toBeUndefined(); |
There was a problem hiding this comment.
🔍 La aserción de elicitation contradice el mapeo de capacidades v2
El fixture v2 anuncia capabilities.elicitation = { form: {} } y capabilities.session.elicitation (apps/backend/src/acp/fixtures/fixture-agent-v2.ts:18-27), y capabilitiesFromResponse añade elicitation: true cuando cualquiera de esos campos está presente (apps/backend/src/acp/connection.ts:101-110). Sin embargo esta prueba exige que agentCapabilities.elicitation sea undefined con la bandera activada. O el mapeo nunca se ejecuta (la respuesta de initialize llega sin capabilities, lo que convertiría ese bloque en código muerto y dejaría también loadSession/promptCapabilities v2 sin datos reales) o la aserción es incorrecta. Conviene verificar cuál de las dos hipótesis se cumple, porque la primera invalidaría todo el mapeo de capacidades v2.
Was this helpful? React with 👍 or 👎 to provide feedback.
| default: | ||
| return { | ||
| type: 'unknown', | ||
| ...(typeof update.sessionUpdate === 'string' | ||
| ? { sessionUpdate: update.sessionUpdate } | ||
| : {}), | ||
| data: update, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🔍 El normalizador v2 no cubre usage_update ni session_info_update
normalizeV2Update no tiene casos para usage_update, session_info_update ni current_mode_update, que sí existen en la ruta v1 (apps/backend/src/acp/normalize/v1.ts:177-200). Con la bandera activada esas notificaciones caen al caso default y se emiten como eventos unknown, de modo que el consumo de tokens/coste y el título de sesión dejan de actualizarse en la UI para agentes v2. Merece confirmarse contra el borrador v2 si esos nombres se mantienen.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const extensionPath = path.resolve('../../apps/extension/.output/chrome-mv3'); | ||
| const context = await chromium.launchPersistentContext('', { | ||
| headless: false, | ||
| args: [ | ||
| `--disable-extensions-except=${extensionPath}`, | ||
| `--load-extension=${extensionPath}`, | ||
| '--no-sandbox', | ||
| ], | ||
| }); | ||
| let [worker] = context.serviceWorkers(); | ||
| if (!worker) worker = await context.waitForEvent('serviceworker'); | ||
| const extensionOrigin = worker.url().split('/').slice(0, 3).join('/'); | ||
| await context.close(); |
There was a problem hiding this comment.
🔍 El arranque del servidor E2E ahora exige un navegador con display
El webServer de Playwright se ejecuta siempre, incluso cuando hasDisplayServer es falso y la lista de proyectos queda vacía (tests/e2e/playwright.config.ts:9-10,30-47). El nuevo script lanza chromium.launchPersistentContext('', { headless: false }) antes de arrancar el backend, por lo que en entornos sin DISPLAY el arranque fallará y la ejecución E2E dejará de pasar «sin tests» como antes. También conviene revisar que path.resolve('.e2e-home') se resuelva respecto al directorio donde Playwright ejecuta el comando.
Was this helpful? React with 👍 or 👎 to provide feedback.
| name: 'DevMentorAI', | ||
| description: 'DevOps mentoring and writing assistant powered by GitHub Copilot', | ||
| version: '1.9.0', | ||
| key: 'MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxAJFCdzqrdDm/BndxDSg6pkLmBncHIoOG1OvZTaZQVclFvKhIyoZAYP2h2sZF2KMjOeTRY84BBRXhAZYZvqfNU/7gDYxIubOw0OX5nTT8HnEKZV4zyTOmodiP5SRRpPNXQEwhuvToUGy04e5R5blcCxGfXxUhtBjGdhPkWlRL8TMbF9uDtsjAiFT3BFSvRWvZAarW5r33uy9JqH0g3AVigt7pGfthidiaq70VNGOrQsok9sNmCWkLLgJehBEJsPomfBXLnlsp2223bfIPvZ1etqEWGOZ0C3jjtla2PuZeiLxisHunVP+f5zUL8q7s60CVEs5XtNUSjZBN6z7OoHfQQIDAQAB', |
There was a problem hiding this comment.
🔍 Clave del manifest añadida sin relación directa con ACP v2
Se fija una key en el manifest, lo que congela el ID de la extensión en desarrollo (probablemente para que el origen calculado por el nuevo script E2E sea estable). Conviene confirmar que esa clave pública coincide con la de la extensión publicada; si no coincide, el ID en producción cambiaría o la subida a la tienda sería rechazada. Además queda fuera del alcance declarado del PR.
Was this helpful? React with 👍 or 👎 to provide feedback.
| name: 'DevMentorAI', | ||
| description: 'DevOps mentoring and writing assistant powered by GitHub Copilot', | ||
| version: '1.9.0', | ||
| key: 'MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAxAJFCdzqrdDm/BndxDSg6pkLmBncHIoOG1OvZTaZQVclFvKhIyoZAYP2h2sZF2KMjOeTRY84BBRXhAZYZvqfNU/7gDYxIubOw0OX5nTT8HnEKZV4zyTOmodiP5SRRpPNXQEwhuvToUGy04e5R5blcCxGfXxUhtBjGdhPkWlRL8TMbF9uDtsjAiFT3BFSvRWvZAarW5r33uy9JqH0g3AVigt7pGfthidiaq70VNGOrQsok9sNmCWkLLgJehBEJsPomfBXLnlsp2223bfIPvZ1etqEWGOZ0C3jjtla2PuZeiLxisHunVP+f5zUL8q7s60CVEs5XtNUSjZBN6z7OoHfQQIDAQAB', |
There was a problem hiding this comment.
🟨 Clave pública de la extensión fijada en el manifiesto (ID de extensión predecible usado para autorizar orígenes)
El manifiesto de la extensión incorpora ahora un campo key fijo en apps/extension/wxt.config.ts:59, lo que fija el ID de la extensión en cualquier compilación. El backend autoriza conexiones por origen de extensión (apps/backend/src/acp/gateway.ts:153-157), de modo que un ID conocido y compartido facilita que una compilación de terceros cargada localmente con la misma clave obtenga el mismo origen y sea aceptada por el gateway local. Aunque la key es material público (no una clave privada), fijarla en el repositorio elimina la unicidad del ID y acopla el control de acceso del backend a un valor conocido por cualquiera.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Description
Phase 9, the last of the plan, stacked on #67 → #66 → #65 → #64 → #63 → #62 → #61 → #60 → #59.
Phase 8 deleted a feature flag; this one adds one that stays. ACP v2 is a draft: the SDK hides it behind
@agentclientprotocol/sdk/experimental/v2warning the wire format may change incompatibly in any release, and the v2 docs tell implementers to gate it and keep serving v1 peers. SoACP_V2is off by default, v2 is offered only when it's on, and v1 agents behave identically either way.normalize/v2.tsbeside the v1 normaliser, with every version-specific shape confined to those two files.state_update-driven lifecycle replacing the turn state we synthesise for v1's blockingsession/prompt,session/resumewithreplayFromfeeding the Phase 6 history path,plan_update, andterminal_update— where R-037's terminal rendering finally lands after Phase 4 deferred it to the generic surface.AcpEvents. The Phase 6 reconciliation guarantees are re-run against v2 replay rather than re-implemented: each message once, replay twice is a no-op, local-only content retained and marked, tool calls reconciled by id. The probe now records the negotiated version.Did ADR-0001's bet hold? The ADR claimed adopting v2 would be "a normaliser plus a flag flip, not a rewrite", because
AcpEventwas modelled on v2 with v1 as the constrained case. It held: the union, the session model and the UI are untouched. The entire version-specific seam in the host is three lines in the session manager — a normaliser ternary keyed on the negotiated version and one early return skipping v1-only turn-state synthesis. ADR-0001's consequences now record where that seam sits so nobody has to rediscover it.Known boundaries, deliberate rather than forgotten: elicitation is minimal — with the flag on, requests are answered
{ action: 'cancel' }; with it off the capability is neither advertised nor registered. There's no interactive form UI yet, and advertised-but-unhandled was the one option ruled out. Replay usesreplayFrom: { type: 'start' }; finer-grained cursors aren't implemented.One E2E fix rides along:
chat.spec.ts › should not send empty messageswas a stale assertion, not a v2 regression — the send button is intentionally replaced by a Stop/Cancel control while streaming, so the spec now asserts that and the disabled textarea.Requirements: R-002 AC2, R-003 AC2, R-037, R-040.
Type of Change
Related Issues
Stacked on #67 → #66 → #65 → #64 → #63 → #62 → #61 → #60 → #59. Completes the roadmap in #59.
Checklist
pnpm typecheck)pnpm lint) — 25 errors / 14 warnings, unchanged from Phase 8's ceiling, all pre-existingpnpm test) — backend 170, extension 173, acp-openai-agent 13, E2E 53 passed / 2 skippedScreenshots (if applicable)
n/a
Link to Devin session: https://app.devin.ai/sessions/bab32da5729e4a95a9cb79f1648f005e
Requested by: @BOTOOM