feat(acp): agent catalog UI, profiles, TCP transport, conformance probe (Phase 5) - #64
feat(acp): agent catalog UI, profiles, TCP transport, conformance probe (Phase 5)#64BOTOOM wants to merge 6 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}
| <AcpProfileEditor | ||
| client={acpCatalogClient} | ||
| onSaved={handleAcpProfileSelected} | ||
| profile={selectedAcpProfile} | ||
| /> |
There was a problem hiding this comment.
🔴 Al elegir un perfil guardado y pulsar Guardar se sobrescribe con datos vacíos
El formulario de perfiles se rellena una sola vez al abrirse y no se recarga cuando se elige otro perfil (<AcpProfileEditor profile={selectedAcpProfile}> en apps/extension/src/entrypoints/sidepanel/SidePanel.tsx:517-521), así que al guardar se envían los campos en blanco que el usuario ve y el perfil elegido queda destruido.
Impacto: El usuario pierde el nombre, el comando y la carpeta de trabajo de un perfil ya configurado con un solo clic.
Estado inicial congelado en useState y ruta de guardado
apps/extension/src/components/AcpProfileEditor.tsx:11-18 inicializa todos los estados con useState(profile?...), que solo se evalúa en el primer montaje. En SidePanel.tsx el editor se monta una vez al abrir el modal (inicialmente con profile === undefined, es decir campos vacíos) y luego handleAcpProfileSelected (SidePanel.tsx:350-352) cambia la prop profile sin remontar el componente.
Al pulsar "Save profile", save() (AcpProfileEditor.tsx:19-33) detecta profile definido y llama client.updateProfile(profile.id, input) con name: '', cmd: '', defaultCwd: '', args: [], env: {}. En el backend AcpAgentService.updateProfile (apps/backend/src/acp/catalog/agent-service.ts:133-143) hace merge sin validar defaultCwd absoluto, por lo que persiste el perfil corrupto.
Una solución simple es forzar el remontaje con key={selectedAcpProfile?.id ?? 'new'} o sincronizar el estado con un useEffect sobre profile.
| <AcpProfileEditor | |
| client={acpCatalogClient} | |
| onSaved={handleAcpProfileSelected} | |
| profile={selectedAcpProfile} | |
| /> | |
| <AcpProfileEditor | |
| client={acpCatalogClient} | |
| key={selectedAcpProfile?.id ?? 'new'} | |
| onSaved={handleAcpProfileSelected} | |
| profile={selectedAcpProfile} | |
| /> |
Was this helpful? React with 👍 or 👎 to provide feedback.
| ...(profile.transport === 'tcp' | ||
| ? { transport: 'tcp' as const, host: profile.host ?? '127.0.0.1', port: profile.port } | ||
| : {}), |
There was a problem hiding this comment.
🟡 Los perfiles por red que no indican un comando local se rechazan al arrancar
Un perfil personalizado se rechaza cuando no tiene comando (if (!profile.cmd) throw en apps/backend/src/acp/catalog/launch-resolver.ts:28) incluso cuando el nuevo modo por red no lanza ningún programa local, de modo que no se puede conectar a un agente que ya está escuchando.
Impacto: El usuario que configura host y puerto sin comando recibe un error de arranque y nunca llega a conectarse.
Camino TCP no necesita `cmd` pero la resolución lo exige
Con transport: 'tcp', AgentLauncher.launch (apps/backend/src/acp/launcher.ts:127-131) solo usa spec.host/spec.port y jamás usa spec.cmd. Sin embargo AgentLaunchResolver.resolve sigue exigiendo profile.cmd para perfiles custom/sin entrada de catálogo antes de construir el launchSpec con transport: 'tcp'.
El editor de la UI (apps/extension/src/components/AcpProfileEditor.tsx:20-28) envía custom: true, cmd: '' cuando no hay agentId, por lo que un perfil TCP creado desde la interfaz siempre cae en esa rama y falla con agent_launch_failed. El propio test tuvo que añadir un cmd: 'copilot' artificial (apps/backend/tests/acp/catalog-phase2.test.ts:393) para pasar.
Posible arreglo: omitir la validación de cmd cuando profile.transport === 'tcp' y validar en su lugar port.
Was this helpful? React with 👍 or 👎 to provide feedback.
| onClick={() => { | ||
| setBusy(entry.id); | ||
| void client.installAgent(entry.id).then((installed) => { | ||
| setEntries((current) => | ||
| current.map((candidate) => | ||
| candidate.id === installed.id ? installed : candidate | ||
| ) | ||
| ); | ||
| setBusy(null); | ||
| }); | ||
| }} |
There was a problem hiding this comment.
🟡 Si falla la instalación de un agente el botón queda inhabilitado para siempre
La instalación se marca como en curso y solo se limpia si termina bien (.then(...) sin catch en apps/extension/src/components/AcpCatalogView.tsx:56-63), así que un fallo deja el botón bloqueado sin ningún mensaje.
Impacto: Tras un error de instalación el usuario no puede reintentar ni sabe qué pasó; hay que cerrar y reabrir el panel.
Promesa sin manejo de rechazo
setBusy(entry.id) se ejecuta antes de la llamada; setBusy(null) solo está dentro del callback de éxito. Si client.installAgent rechaza (por ejemplo agent_launch_failed o desconexión del WebSocket, ver apps/extension/src/services/acp-client.ts:188-190), el estado busy permanece igual al id de la entrada y el botón sigue disabled. Además la promesa rechazada no se captura, generando un unhandled rejection.
Conviene usar try/finally (o .catch(...).finally(...)) y exponer el error al usuario.
| onClick={() => { | |
| setBusy(entry.id); | |
| void client.installAgent(entry.id).then((installed) => { | |
| setEntries((current) => | |
| current.map((candidate) => | |
| candidate.id === installed.id ? installed : candidate | |
| ) | |
| ); | |
| setBusy(null); | |
| }); | |
| }} | |
| onClick={() => { | |
| setBusy(entry.id); | |
| void client | |
| .installAgent(entry.id) | |
| .then((installed) => { | |
| setEntries((current) => | |
| current.map((candidate) => | |
| candidate.id === installed.id ? installed : candidate | |
| ) | |
| ); | |
| }) | |
| .catch(() => undefined) | |
| .finally(() => setBusy(null)); | |
| }} |
Was this helpful? React with 👍 or 👎 to provide feedback.
| <select | ||
| aria-label="ACP profile" | ||
| className="ml-2 rounded border px-1" | ||
| onChange={(event) => { | ||
| const profile = profiles.find((candidate) => candidate.id === event.target.value); | ||
| if (profile) onProfileSelected?.(profile); | ||
| }} | ||
| > |
There was a problem hiding this comment.
🟡 El primer perfil de la lista aparece elegido pero el botón para usarlo sigue apagado
La lista de perfiles muestra el primero como opción visible sin comunicarlo hacia fuera (falta de valor controlado y de aviso inicial en apps/extension/src/components/AcpCatalogView.tsx:83-90), así que quien no cambie la opción tiene el botón de iniciar sesión deshabilitado.
Impacto: El usuario ve un perfil elegido en pantalla pero no puede usarlo hasta cambiar a otro y volver.
Select no controlado y sin notificación inicial
El <select> no recibe value ni defaultValue y solo llama onProfileSelected en onChange. El navegador muestra la primera <option> por defecto, pero SidePanel mantiene selectedAcpProfile === undefined (apps/extension/src/entrypoints/sidepanel/SidePanel.tsx:44), lo que deja el botón "Use selected profile for ACP session" con disabled={!selectedAcpProfile} (SidePanel.tsx:524-526). Al seleccionar la primera opción no se dispara change, por lo que el estado nunca se rellena.
Una opción es notificar el primer perfil al cargar (en refresh) y/o convertir el select en controlado con un estado local de id.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import fs from 'node:fs'; | ||
| import { type AcpProbeReport, renderSupportTable } from '../apps/backend/src/acp/probe.js'; | ||
|
|
||
| const reportPath = process.argv[2] ?? 'acp-probe-results.json'; |
There was a problem hiding this comment.
🟡 El generador de la tabla de compatibilidad busca los resultados en una ruta inexistente
El generador lee por defecto un archivo en la raíz del repositorio (process.argv[2] ?? 'acp-probe-results.json' en scripts/generate-acp-support-table.ts:4) mientras que los resultados se escriben dentro de la carpeta de documentación, por lo que ejecutarlo sin argumentos falla.
Impacto: Regenerar la tabla de agentes tal como está documentado da un error de archivo no encontrado.
Desajuste con el script de sondeo
scripts/run-acp-probes.ts:83-86 escribe en path.resolve('docs/acp-probe-results.json'), pero el generador espera acp-probe-results.json en el cwd cuando no se pasa argumento; fs.readFileSync lanzará ENOENT.
| const reportPath = process.argv[2] ?? 'acp-probe-results.json'; | |
| const reportPath = process.argv[2] ?? 'docs/acp-probe-results.json'; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const handleStartAcpSession = useCallback(async () => { | ||
| if (!selectedAcpProfile) return; | ||
| await acpCatalogClient.connect(); | ||
| await acpCatalogClient.createSession(selectedAcpProfile.id, selectedAcpProfile.defaultCwd); | ||
| setShowAcpAgents(false); | ||
| }, [acpCatalogClient, selectedAcpProfile]); |
There was a problem hiding this comment.
🔍 La sesión ACP creada desde el modal se descarta
handleStartAcpSession crea una sesión contra un AcpClient independiente (acpCatalogClient) y descarta el AcpSessionRecord devuelto; ni useSessions ni useChat (que usa su propia instancia de AcpClient) se enteran. El resultado visible es que el modal se cierra y no cambia nada en la interfaz, aunque en el backend queda una sesión y una conexión de agente vivas (apps/backend/src/acp/gateway.ts:358-405). Además el panel abre un segundo WebSocket permanente con reconexión automática que nunca se cierra.
Was this helpful? React with 👍 or 👎 to provide feedback.
| report.checks.cancel = await connection.probeRequest('session/cancel', { | ||
| sessionId: session.sessionId, | ||
| }); |
There was a problem hiding this comment.
🔍 session/cancel se sondea como petición cuando el protocolo lo define como notificación
probeRequest('session/cancel', ...) envía una petición JSON-RPC con id; en ACP la cancelación es una notificación (así la envía AgentConnection.cancel, apps/backend/src/acp/connection.ts:271-278). Por eso los resultados guardados muestran -32601 Method not found para agentes que sí soportan cancelar, lo que puede llevar a interpretar mal la tabla generada.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (spec.transport === 'tcp') { | ||
| if (!spec.port) throw new AcpError('agent_launch_failed', 'TCP profile requires a port'); | ||
| const socket = net.createConnection({ host: spec.host ?? '127.0.0.1', port: spec.port }); | ||
| return this.track(new AgentProcess(socket, stderrLimit)); | ||
| } |
There was a problem hiding this comment.
🟨 El endpoint de sondeo de conformidad permite conectarse a hosts y puertos arbitrarios y arrancar comandos sin validación
El nuevo método ui/agents.probe del gateway (apps/backend/src/acp/gateway.ts:347-348) resuelve el perfil y ejecuta runConformanceProbe, que abre la conexión definida por el perfil. Con el nuevo transporte TCP, un perfil creado vía ui/agents.create_profile puede fijar host/port arbitrarios (apps/backend/src/acp/catalog/launch-resolver.ts:39-41, apps/backend/src/acp/launcher.ts:127-131) y el backend abrirá una conexión saliente a ese destino desde la máquina del usuario, incluyendo direcciones internas. Ni host ni port se validan (rango, loopback, formato) en AcpAgentService.createProfile (apps/backend/src/acp/catalog/agent-service.ts:103-115), que solo valida que defaultCwd sea absoluto.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Revisión atendida en |
1 participant
Description
Phase 5, stacked on #63 → #62 → #61 → #60 → #59. This is the phase that makes "every ACP agent on the market" true in the product rather than only in the backend. The acid test throughout: a registry entry this codebase has never heard of must be findable, installable, configurable and usable through the UI with no code change — asserted by a test that injects a fake registry entry, gives it a profile and launches it.
devin acp --cloud,copilot --acp --port N, a custom non-registry command, per-profile env referencing stored credentials by name only (values never leave the backend), default cwd.agent_crashed, and clean shutdown.resource_link(R-033, deferred from Phase 4 until there was a workspace surface): referenced files go out with absolutefile://URIs.Conformance probe (R-017) — per agent it runs, as far as that agent allows:
initialize,session/new, a scripted prompt, a slash command, an image block, a permission round-trip, a history probe and cancel, recording protocol version, capabilities,loadSession, advertised commands, auth methods and failures with a real timestamp. An unadvertised method answering-32601is a result, not an error. The support table indocs/ACP.mdis generated from probe results — a hand-maintained matrix is stale the day it's written.Worth flagging for reviewers, because it nearly shipped as confident misinformation: the first generated table reported Copilot as having neither image support nor history load, contradicting the Phase 0 spike, which measured
loadSession: trueandpromptCapabilities.image: truedirectly. The cause was the table sourcing request outcomes rather than advertised capabilities — Copilot's image prompt was blocked byAuthentication required, and that rendered as "no images". Advertised capability and scripted-probe outcome are now separate, with a third state so that "we couldn't measure this" never renders as "the agent doesn't support it":These are real runs on a machine with no agent credentials, so rows honestly record what was blocked on auth. A fixture test now pins that an agent advertising
image: true/loadSession: trueis recorded as such, so a future capability-shape refactor breaks a test instead of quietly downgrading every agent in the docs.Requirements: R-010..R-018, R-033. Playwright ACs stay unverified — Chromium is installed now, but the harness is blocked on legacy Copilot auth until Phase 8 removes it.
Type of Change
Related Issues
Stacked on #63 → #62 → #61 → #60 → #59.
Checklist
pnpm typecheck)pnpm lint) — scoped Biome clean; repo-wide run is pre-existing-red on unrelated filespnpm test) — backend ACP 53+, extension 10; Playwright blocked on legacy Copilot authdocs/ACP.mdis generated, not hand-writtenScreenshots (if applicable)
n/a — no UI verification captured while the E2E harness is blocked on legacy Copilot auth.
Link to Devin session: https://app.devin.ai/sessions/bab32da5729e4a95a9cb79f1648f005e
Requested by: @BOTOOM