feat(acp): ship our own ACP agent for OpenAI-compatible endpoints (Phase 7) - #66
feat(acp): ship our own ACP agent for OpenAI-compatible endpoints (Phase 7)#66BOTOOM wants to merge 8 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}
| distribution: { | ||
| command: { | ||
| cmd: process.execPath, | ||
| args: [path.resolve(process.cwd(), 'apps/acp-openai-agent/dist/main.js')], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🔴 El nuevo agente integrado no arranca salvo que el servidor se ejecute desde la raíz del repositorio
La ruta del ejecutable del nuevo agente se calcula a partir del directorio de trabajo del proceso (path.resolve(process.cwd(), ...) en apps/backend/src/acp/catalog/agent-catalog.ts:43) al cargar el módulo, así que apunta a un archivo inexistente en cualquier arranque que no sea desde la raíz del repositorio.
Impacto: los usuarios que seleccionen el agente para endpoints compatibles con OpenAI verán un fallo al iniciarlo en instalaciones publicadas y en el modo de desarrollo del backend.
Resolución de ruta relativa al cwd en una constante de módulo
BUILT_IN_AGENTS se evalúa al importar el módulo. pnpm dev:backend ejecuta pnpm --filter devmentorai-server dev, cuyo cwd es apps/backend, por lo que la ruta resulta apps/backend/apps/acp-openai-agent/dist/main.js. En el paquete publicado devmentorai-server (que además solo incluye dist/, README.md y LICENSE según apps/backend/package.json) el agente ni siquiera está distribuido, y el cwd es el del usuario.
apps/backend/src/acp/catalog/launch-resolver.ts:86-90 toma distribution.command.cmd/args tal cual y apps/backend/src/acp/launcher.ts:132 hace spawn, de modo que Node fallará con «Cannot find module». Además availability() (apps/backend/src/acp/catalog/agent-catalog.ts:130) marca la entrada como disponible, así que la UI la ofrece igualmente.
Sería más robusto resolver la ruta respecto a la ubicación del propio módulo (import.meta.url), permitir sobreescribirla por variable de entorno y/o verificar la existencia del archivo para reflejarla en installState/platformAvailability.
Prompt for agents
La entrada integrada 'devmentorai-openai-compatible' en BUILT_IN_AGENTS (apps/backend/src/acp/catalog/agent-catalog.ts) fija el argumento del comando con path.resolve(process.cwd(), 'apps/acp-openai-agent/dist/main.js'), evaluado al cargar el módulo. El cwd del backend no es la raíz del monorepo ni en desarrollo (pnpm --filter ejecuta dentro de apps/backend) ni en el paquete npm publicado, donde el agente ni siquiera se distribuye. Hay que resolver la ubicación del bundle del agente de forma independiente del cwd (por ejemplo respecto a import.meta.url del backend compilado, o mediante una variable de entorno configurable), y considerar reflejar la ausencia del archivo en installState/platformAvailability para no ofrecer un agente que no se puede lanzar.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!line.startsWith('data:')) continue; | ||
| const payload = line.slice(5).trim(); | ||
| if (payload === '[DONE]') continue; | ||
| const parsed: unknown = JSON.parse(payload); | ||
| if (!isRecord(parsed) || !Array.isArray(parsed.choices)) continue; |
There was a problem hiding this comment.
🟡 Una línea de datos malformada del endpoint aborta toda la respuesta del modelo
Cada línea de datos recibida del endpoint se interpreta como JSON sin ninguna protección (JSON.parse(payload) en apps/acp-openai-agent/src/openai-compatible-agent.ts:263), de modo que una línea vacía o no válida hace fallar la petición completa.
Impacto: la conversación termina con error y se pierde todo lo ya generado cuando el servidor envía cualquier línea de mantenimiento o no estándar.
Parseo del flujo SSE sin tolerancia a fallos
En el bucle de lectura (apps/acp-openai-agent/src/openai-compatible-agent.ts:259-280) solo se filtran las líneas que no empiezan por data: y el marcador [DONE]. Un data: vacío (keep-alive habitual en algunas implementaciones compatibles con OpenAI) produce JSON.parse(''), que lanza una excepción. Esa excepción sale de complete() y, como el controlador no está abortado, prompt() la vuelve a lanzar (:215-217), devolviendo un error JSON-RPC al cliente en lugar de terminar el turno.
| if (!line.startsWith('data:')) continue; | |
| const payload = line.slice(5).trim(); | |
| if (payload === '[DONE]') continue; | |
| const parsed: unknown = JSON.parse(payload); | |
| if (!isRecord(parsed) || !Array.isArray(parsed.choices)) continue; | |
| if (!line.startsWith('data:')) continue; | |
| const payload = line.slice(5).trim(); | |
| if (payload === '' || payload === '[DONE]') continue; | |
| let parsed: unknown; | |
| try { | |
| parsed = JSON.parse(payload); | |
| } catch { | |
| continue; | |
| } | |
| if (!isRecord(parsed) || !Array.isArray(parsed.choices)) continue; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
| throw new Error('Tool-call loop exceeded its maximum number of rounds'); |
There was a problem hiding this comment.
🟡 Alcanzar el máximo de rondas de herramientas devuelve un error en vez de finalizar el turno
Cuando se agotan las ocho rondas del ciclo de herramientas se lanza una excepción (throw new Error('Tool-call loop exceeded...') en apps/acp-openai-agent/src/openai-compatible-agent.ts:214) en lugar de terminar el turno con un motivo de parada.
Impacto: el usuario ve un fallo genérico de la petición y pierde el contexto de la respuesta en vez de un aviso de que se alcanzó el límite.
Motivo de parada previsto por el protocolo
El protocolo ACP define un motivo de parada específico para este caso (max_turn_requests), pensado para que el cliente muestre la conversación y ofrezca continuar. Al lanzar una excepción, el manejador prompt propaga el error (:215-217) y la petición session/prompt se resuelve como error JSON-RPC; además los mensajes ya acumulados en la sesión quedan con llamadas de herramienta resueltas pero sin respuesta final del modelo.
| } | |
| throw new Error('Tool-call loop exceeded its maximum number of rounds'); | |
| } | |
| return { stopReason: 'max_turn_requests' }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| session.messages.push({ | ||
| role: 'assistant', | ||
| content: result.content || null, | ||
| tool_calls: result.toolCalls.map((call) => ({ | ||
| id: call.id, | ||
| type: 'function', | ||
| function: { name: call.name, arguments: call.arguments }, | ||
| })), | ||
| }); | ||
| const results = await this.resolveToolCalls( | ||
| session, | ||
| params.sessionId, | ||
| result.toolCalls, | ||
| client | ||
| ); | ||
| for (const toolResult of results) { | ||
| session.messages.push({ | ||
| role: 'tool', | ||
| tool_call_id: toolResult.call.id, | ||
| content: toolResult.rejected | ||
| ? 'Tool execution rejected by the user.' | ||
| : (toolResult.result ?? ''), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🔍 El estado de la sesión puede quedar inconsistente si el turno falla tras registrar las llamadas de herramienta
En apps/acp-openai-agent/src/openai-compatible-agent.ts:188-211 se añade primero el mensaje de asistente con tool_calls y después, en un paso separado, los mensajes role: 'tool'. Si algo lanza en medio (por ejemplo, la petición de permiso al cliente falla o la conexión ACP se cae durante client.notify en :308-318), la sesión queda con un mensaje de asistente con tool_calls sin las respuestas tool correspondientes. Como session.messages persiste entre prompts, todas las peticiones posteriores de esa sesión enviarán un historial que la mayoría de endpoints compatibles con OpenAI rechazan con 400, dejando la sesión inutilizable de forma permanente. Convendría rebobinar los mensajes añadidos en el catch/finally de prompt().
Was this helpful? React with 👍 or 👎 to provide feedback.
| "scripts": { | ||
| "build": "tsc -p tsconfig.json", | ||
| "test": "vitest", | ||
| "typecheck": "tsc --noEmit" | ||
| }, |
There was a problem hiding this comment.
🔍 Las pruebas dependen de un dist compilado previamente y del cwd del paquete
connectAgent lanza node dist/main.js con cwd: process.cwd() (apps/acp-openai-agent/src/openai-compatible-agent.test.ts:79-81), pero el script test del paquete (apps/acp-openai-agent/package.json:11) solo ejecuta vitest sin compilar antes. En un checkout limpio (pnpm test sin pnpm build previo) todas las pruebas fallarán por módulo inexistente, y el fallo se manifestará como timeout del handshake ACP en lugar de un mensaje claro. Sería mejor encadenar la compilación (pretest) o usar tsx sobre src/main.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
| { | ||
| id: 'devmentorai-openai-compatible', | ||
| name: 'OpenAI-compatible endpoint', | ||
| description: 'DevMentorAI ACP agent for OpenAI-compatible chat-completions endpoints', | ||
| distribution: { | ||
| command: { | ||
| cmd: process.execPath, | ||
| args: [path.resolve(process.cwd(), 'apps/acp-openai-agent/dist/main.js')], | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🔍 La entrada integrada no puede alcanzar el estado «instalado»
Para distribuciones de tipo command, defaultInstallState (apps/backend/src/acp/catalog/agent-catalog.ts:145-163) cae en la rama final y devuelve entry.installState, que para source === 'builtin' se fija a 'not_installed' en sourceEntry (:103). Por tanto el nuevo agente aparecerá siempre como no instalado aunque su binario exista, y al no tener npx/binary tampoco hay flujo de instalación que pueda cambiarlo. Conviene verificar cómo trata la UI de Fase 4 ese estado antes de exponer la entrada.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (call.name === 'run_shell') { | ||
| const result = await execFileAsync('/bin/sh', ['-c', stringValue(input.command)], { | ||
| cwd: session.cwd, | ||
| maxBuffer: 1024 * 1024, | ||
| ...(session.controller ? { signal: session.controller.signal } : {}), | ||
| }); | ||
| return `${result.stdout}${result.stderr}`; | ||
| } |
There was a problem hiding this comment.
🟨 El agente concede acceso al sistema de ficheros y ejecución de shell basada solo en el permiso de la UI
El agente expone una herramienta run_shell que ejecuta cualquier cadena recibida del modelo mediante /bin/sh -c (apps/acp-openai-agent/src/openai-compatible-agent.ts:377). El comando procede directamente de la respuesta del endpoint compatible con OpenAI (potencialmente un servidor remoto como OpenRouter), y el único control es la solicitud de permiso al cliente ACP. A diferencia de read_file/write_file, que están confinados con safePath, el comando de shell no tiene ningún confinamiento más allá del cwd inicial: puede leer/escribir fuera del workspace, exfiltrar credenciales del entorno (el proceso hereda process.env, incluida OPENAI_COMPATIBLE_API_KEY) o establecer conexiones salientes.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const permission = await client.request(acp.methods.client.session.requestPermission, { | ||
| sessionId, | ||
| toolCall: { toolCallId: call.id, title: call.name, kind: 'execute', status: 'pending' }, | ||
| options: [ | ||
| { optionId: 'allow', name: 'Allow once', kind: 'allow_once' }, | ||
| { optionId: 'reject', name: 'Reject', kind: 'reject_once' }, | ||
| ], | ||
| }); |
There was a problem hiding this comment.
🟨 La solicitud de permiso no muestra qué va a hacer la herramienta
La petición session/request_permission (apps/acp-openai-agent/src/openai-compatible-agent.ts:319-326) envía únicamente el nombre de la herramienta como título, sin los argumentos (ruta a escribir, comando a ejecutar). El usuario aprueba una acción cuyo contenido desconoce, lo que degrada la eficacia del único control interactivo previo a la ejecución de escrituras y comandos.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Revisión atendida en Sobre 🟨 |
1 participant
Description
Phase 7, stacked on #65 → #64 → #63 → #62 → #61 → #60 → #59. LM Studio, Ollama, vLLM, llama.cpp and OpenRouter expose OpenAI-compatible APIs, not ACP. Per ADR-0006 we neither special-case them in the host (that would reintroduce the provider abstraction this refactor exists to delete) nor depend on an unvetted third-party bridge — we ship our own minimal ACP agent,
apps/acp-openai-agent, spawned through a built-in catalog entry like any other agent.The invariant: nothing under
apps/backend/src/acp/knows LM Studio or Ollama exists — verified, no endpoint-specific names there. Local models are just another ACP agent.What the agent does:
initialize/session/new/session/prompt/session/cancelon the agent side of the same pinned SDK; ACP content blocks mapped onto chat-completions messages; streaming relayed asagent_message_chunk;session/cancelaborting the upstream request mid-stream rather than merely dropping output; model options populated from the endpoint'sGET /v1/models, degrading to the configured model when it doesn't answer.The tool loop is the substance of this phase, and the first cut of it wasn't real — it asked permission and then marked the call
completed, never executing anything, never appending atoolresult, never continuing the completion, and never sending atoolsarray in the first place, so no endpoint could have emitted a tool call at all. It now sends the tool set, accumulates streamed tool-call deltas by index until the fragmentedfunction.argumentsJSON parses (the real streaming shape), requests permission, executes on approval, appends the result and re-invokes until the model stops asking, capped at eight rounds, withpending → in_progress → completed/failedtracked as ACP tool-call updates. The Phase 4 permission model applies here in full: our own agent doesn't get to bypass it.Tools are
read_file,write_fileandrun_shell, all confined to the sessioncwd— enforced in the agent, not by the permission prompt, which is a UI-side gate and the wrong place for a security boundary. Paths are resolved before comparison so../traversal, absolute paths outsidecwdand symlinks pointing out are all rejected with the call endingfailedand nothing touched on disk; there are tests for each, because a refactor could otherwise remove that guard silently.Two capabilities were being advertised dishonestly and are fixed, which matters more here than usual since Phase 4's UI is driven entirely off what an agent claims:
embeddedContext: truewas advertised whileresourceblocks were silently dropped (so context-aware mode's page content vanished without a warning — they're mapped now), and an API-keyauthMethodwas advertised whoseauthenticatewas a no-op that resolved successfully and then failed on the first request (withdrawn; credentials stay backend-resolved).Tests drive the agent end-to-end over the real ACP stdio transport against an in-process fake OpenAI-compatible server — streaming, fragmented tool calls, a rejected permission, upstream errors, a held stream for cancellation, an approved
write_file→read_fileround-trip the model then answers from, the three escape attempts, and cancel mid-tool-loop.One incidental fix: the new package's Vitest was discovering the ACP SDK's own compiled tests under
node_modules/.../dist, inflating its result to 425 tests. Scoped tosrc/**/*.test.ts— the honest count is 13, and no other suite's coverage was narrowed (backend 184, extension 177).Type of Change
Related Issues
Stacked on #65 → #64 → #63 → #62 → #61 → #60 → #59. Implements ADR-0006.
Checklist
pnpm typecheck)pnpm lint) — scoped Biome clean; repo-wide run is pre-existing-red on unrelated filespnpm test) — acp-openai-agent 13, backend 184, extension 177; Playwright blocked on legacy Copilot auth (Phase 8)Screenshots (if applicable)
n/a
Link to Devin session: https://app.devin.ai/sessions/bab32da5729e4a95a9cb79f1648f005e
Requested by: @BOTOOM