feat(backend): ACP agent catalog, profiles, install, credentials, auth (Phase 2) - #61
feat(backend): ACP agent catalog, profiles, install, credentials, auth (Phase 2)#61BOTOOM 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}
| resolveReference(reference: string): string | undefined { | ||
| const match = | ||
| /^(?:credential:|credential:\/\/)(.+)$/.exec(reference) ?? | ||
| /^\$\{credential:([^}]+)\}$/.exec(reference); | ||
| return match ? this.get(match[1]) : undefined; | ||
| } |
There was a problem hiding this comment.
🟡 Las credenciales escritas con el formato credential:// nunca se encuentran y el agente no arranca
El nombre de la credencial se extrae mal (/^(?:credential:|credential:\/\/)(.+)$/ en apps/backend/src/acp/catalog/credentials.ts:50) cuando la referencia usa la forma con doble barra, de modo que se busca un nombre inexistente y el arranque del agente falla con un error de credencial no configurada.
Impact: Un perfil que referencia su clave con la forma credential://nombre nunca puede lanzarse, aunque la credencial exista guardada.
Alternancia de la expresión regular: `credential:` gana antes que `credential://`
En una alternancia, el motor prueba las ramas en orden: para credential://provider-key la rama credential: casa primero y el grupo (.+) captura //provider-key. this.get('//provider-key') devuelve undefined.
En resolveEnvironment (apps/backend/src/acp/catalog/credentials.ts:55-69) el valor sí se detecta como referencia (isReference es true), por lo que se lanza AcpError('agent_launch_failed', ...) y el lanzamiento del agente se aborta. La forma credential:nombre sí funciona, lo que hace el fallo silencioso hasta que alguien usa la variante documentada con //.
| resolveReference(reference: string): string | undefined { | |
| const match = | |
| /^(?:credential:|credential:\/\/)(.+)$/.exec(reference) ?? | |
| /^\$\{credential:([^}]+)\}$/.exec(reference); | |
| return match ? this.get(match[1]) : undefined; | |
| } | |
| resolveReference(reference: string): string | undefined { | |
| const match = | |
| /^credential:(?:\/\/)?(.+)$/.exec(reference) ?? | |
| /^\$\{credential:([^}]+)\}$/.exec(reference); | |
| return match ? this.get(match[1]) : undefined; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const capabilities = await connection.connect(); | ||
| this.authMethods.set(profileId, capabilities.authMethods); | ||
| this.writeAuthState(profileId, 'unknown', capabilities.authMethods); | ||
| await connection.authenticate(methodId); | ||
| this.authStates.set(profileId, 'authenticated'); | ||
| this.writeAuthState(profileId, 'authenticated', capabilities.authMethods); | ||
| } catch (error) { | ||
| if (error instanceof AcpError && error.code === 'auth_required') { | ||
| this.authStates.set(profileId, 'required'); | ||
| this.writeAuthState(profileId, 'required', this.authMethods.get(profileId) ?? []); | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
🟡 Un intento fallido de inicio de sesión borra el estado "autenticado" ya guardado del agente
El estado guardado del agente se sobrescribe a "desconocido" (writeAuthState(profileId, 'unknown', ...) en apps/backend/src/acp/catalog/agent-service.ts:143) antes de intentar el inicio de sesión, así que si ese intento falla el agente queda marcado como no autenticado aunque siga estándolo.
Impact: La interfaz mostrará que el agente necesita autenticarse de nuevo tras cualquier reintento fallido, aunque su sesión previa siga siendo válida.
Secuencia de escritura en `authenticate`
En apps/backend/src/acp/catalog/agent-service.ts:140-153, tras connection.connect() se persiste incondicionalmente 'unknown' junto con los authMethods. Solo si connection.authenticate(methodId) tiene éxito se vuelve a escribir 'authenticated'; si falla con un error distinto de auth_required (por ejemplo, un fallo de red o capability_unsupported lanzado en AgentConnection.authenticate, apps/backend/src/acp/connection.ts:244-251), no se restaura nada y la fila queda en 'unknown'.
Además AcpAgentService.list() (apps/backend/src/acp/catalog/agent-service.ts:82) da prioridad al valor de la base de datos sobre el caché en memoria, así que el estado degradado es el que se expone.
Una posible corrección: persistir solo los authMethods antes del intento y no tocar auth_state, o releer/preservar el estado previo cuando el intento falla.
Prompt for agents
En AcpAgentService.authenticate (apps/backend/src/acp/catalog/agent-service.ts) se persiste incondicionalmente auth_state='unknown' justo después de conectar y antes de llamar a connection.authenticate. Si la autenticación falla con un error que no sea auth_required, el estado persistido queda en 'unknown', perdiendo un 'authenticated' previo válido. Considera persistir únicamente los authMethods en ese punto (sin modificar auth_state), o leer el estado actual con readAuthState y conservarlo cuando el intento falle por causas no relacionadas con la autenticación.
Was this helpful? React with 👍 or 👎 to provide feedback.
| async install(agentId: string): Promise<AgentCatalogEntry> { | ||
| const entry = await this.requireEntry(agentId); | ||
| await this.installer.install(entry); | ||
| this.installed.add(agentId); | ||
| return { ...entry, installState: 'installed' }; | ||
| } |
There was a problem hiding this comment.
🔍 install() sobre agentes npx/uvx lanza un error de plataforma engañoso
AcpAgentService.install() llama siempre a AgentInstaller.install, que en selectBinary (apps/backend/src/acp/catalog/agent-installer.ts:153-162) exige una distribución binary para la clave de plataforma. Para un agente npx/uvx (estado lazy, platformAvailability.available === true) esto lanza capability_unsupported con el mensaje por defecto "Agent is unavailable on this platform", que es falso: el agente sí está disponible, simplemente no requiere instalación. uninstall() es asimétrico (no falla, simplemente borra un directorio inexistente). Convendría que install() sea un no-op que devuelva installState: 'lazy' cuando no hay distribución binaria, antes de que Phase 3 exponga esto por HTTP.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (const profile of profiles) { | ||
| const raw = profile.agentId | ||
| ? merged.get(profile.agentId) | ||
| : sourceEntry( | ||
| { | ||
| id: profile.id, | ||
| name: profile.name, | ||
| distribution: { | ||
| command: { cmd: profile.cmd ?? '', args: profile.args, env: profile.env }, | ||
| }, | ||
| }, | ||
| 'custom' | ||
| ); | ||
| if (!raw) continue; | ||
| merged.set(profile.id, { ...raw, id: profile.id, name: profile.name, source: 'custom' }); | ||
| } |
There was a problem hiding this comment.
🔍 Los perfiles que apuntan a un agentId desconocido se descartan silenciosamente del catálogo
En AgentCatalog.list(), si profile.agentId no está en el mapa fusionado (por ejemplo, estando offline sin caché de registro, o si el agente desapareció del registro), raw es undefined y el perfil se omite con continue. El perfil sigue existiendo en SQLite y resolveLaunch lo tratará como personalizado (entry undefined en AgentLaunchResolver.resolve, apps/backend/src/acp/catalog/launch-resolver.ts:33-36), fallando con Custom profile ... has no command si no tiene cmd. Sería preferible listar el perfil con installState: 'unavailable' y una razón, en lugar de hacerlo invisible en la UI.
Was this helpful? React with 👍 or 👎 to provide feedback.
| save(profile: AgentProfile): AgentProfile { | ||
| const now = new Date().toISOString(); | ||
| this.db | ||
| .prepare( | ||
| `INSERT INTO acp_profiles | ||
| (id, name, agent_id, command, args_json, env_json, default_cwd, transport, created_at, updated_at) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
| ON CONFLICT(id) DO UPDATE SET | ||
| name = excluded.name, | ||
| agent_id = excluded.agent_id, | ||
| command = excluded.command, | ||
| args_json = excluded.args_json, | ||
| env_json = excluded.env_json, | ||
| default_cwd = excluded.default_cwd, | ||
| transport = excluded.transport, | ||
| updated_at = excluded.updated_at` | ||
| ) | ||
| .run( | ||
| profile.id, | ||
| profile.name, | ||
| profile.agentId ?? null, | ||
| profile.cmd ?? null, | ||
| JSON.stringify(profile.args), | ||
| JSON.stringify(profile.env), | ||
| profile.defaultCwd, | ||
| profile.transport, | ||
| now, | ||
| now | ||
| ); | ||
| return profile; | ||
| } |
There was a problem hiding this comment.
🔍 La bandera custom del perfil no se persiste y se deriva de agent_id
AgentProfileStore.save no guarda profile.custom; toProfile (apps/backend/src/acp/catalog/profile-store.ts:27) lo reconstruye como custom: true únicamente cuando agent_id es null. Un perfil creado con agentId y custom: true pierde la bandera al releerse, lo que cambia la rama tomada en AgentLaunchResolver.resolve (profile.custom || !entry): el cmd explícito del perfil se ignora y se resuelve la distribución del catálogo. Merece confirmar si esa combinación es válida o debería rechazarse en createProfile.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (isZip) { | ||
| await run(extractor, ['-q', archive, '-d', temporary], temporary); | ||
| } else { | ||
| await run(extractor, ['--no-absolute-names', '-xf', archive, '-C', temporary], temporary); | ||
| } |
There was a problem hiding this comment.
🔍 La opción --no-absolute-names de tar es específica de GNU tar
En macOS (y en BSD en general) tar es bsdtar, que no acepta --no-absolute-names (usa -P para el comportamiento inverso y rechaza opciones largas desconocidas). En esas plataformas la extracción fallaría con un código de salida distinto de cero mapeado a Unable to install agent ..., no al error accionable de extractor ausente. Dado que la validación previa de entradas en validateArchiveEntries ya rechaza rutas absolutas y .., valdría la pena comprobar la disponibilidad del flag o detectar el sabor de tar.
Was this helpful? React with 👍 or 👎 to provide feedback.
| this.onProgress?.({ phase: 'extract' }); | ||
| const isZip = extractor === 'unzip'; | ||
| const listed = isZip | ||
| ? await runCapture(extractor, ['-Z1', archive], temporary) | ||
| : await runCapture(extractor, ['-tf', archive], temporary); | ||
| validateArchiveEntries(listed.split(/\r?\n/).filter(Boolean)); | ||
| if (isZip) { | ||
| await run(extractor, ['-q', archive, '-d', temporary], temporary); | ||
| } else { | ||
| await run(extractor, ['--no-absolute-names', '-xf', archive, '-C', temporary], temporary); | ||
| } | ||
| const command = path.resolve(temporary, binary.cmd); | ||
| if (!isWithin(temporary, command)) { | ||
| throw new AcpError('agent_launch_failed', 'Agent command escapes the install directory'); | ||
| } | ||
| const commandStat = await fs.stat(command); | ||
| if (!commandStat.isFile()) { | ||
| throw new AcpError('agent_launch_failed', 'Installed agent command was not found'); | ||
| } |
There was a problem hiding this comment.
🟨 La extracción de archivos descargados no protege contra enlaces simbólicos maliciosos
El instalador valida los nombres de entrada del archivo (validateArchiveEntries en apps/backend/src/acp/catalog/agent-installer.ts:40-53) rechazando rutas absolutas y .., pero la extracción con tar o unzip puede seguir creando enlaces simbólicos dentro del directorio temporal que apunten fuera de él; un archivo posterior en el mismo tar puede escribirse a través de ese enlace, permitiendo escritura arbitraria de ficheros fuera de ~/.devmentorai/agents. El origen del archivo proviene de un documento de registro remoto (ACP_REGISTRY_URL), por lo que el contenido no es de confianza.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Revisión atendida en |
1 participant
Description
Phase 2 of the ACP migration, stacked on #60 (which is stacked on the spec, #59). Everything an agent needs before a session exists: what agents there are, how to launch them, how to install them, and how to authenticate. Still no HTTP/WS wiring (Phase 3) and no UI; the Copilot path is untouched.
The catalog is data, never code (ADR-0005). Three merged sources — user profiles > cached ACP registry > a built-in pinned set — so a registry entry this codebase has never heard of is listable and launchable. That's asserted by a test that launches an agent the code knows nothing about through a custom-command profile. The registry client takes an injected fetcher (no network in tests), caches under
~/.devmentorai/with a TTL, and falls back cache→built-in so being offline degrades rather than fails. A single malformed registry entry is skipped, not fatal.Profiles are the variant mechanism (R-016): a named launch tuple persisted in SQLite, so one agent can exist many times with different configuration —
devin acpvsdevin acp --cloud,copilot --acp --stdiovs--acp --port N, one profile per BYOK provider, or a non-registry agent like MiniMax'smini-agent-acp. Distribution args and profile args concatenate (getting this wrong silently drops exactly the flags that make a variant a variant, so there's a test per distribution kind).Launch resolution covers
npx,uvx, per-platformbinaryand rawcommand, mappingprocess.platform/archonto the registry's platform keys; an entry with no build for this platform is reported unavailable with a reason instead of being spawned blindly. TCP transport is typed but returns an unsupported error until Phase 5.Installer: on-demand download into
~/.devmentorai/agents/<id>/<version>/, sha256-verified, extract into a temp dir and rename, nothing executed and nothing left behind on mismatch, and installs are cached instead of re-downloading per session. Hardening, since we're extracting archives named by a remote document: absolute-path and..entries are rejected, tar runs with--no-absolute-names, the resolved command must resolve inside the install directory, and a missingtar/unzipproduces an actionable error rather than a bare exit code.Credentials are stored encrypted (AES-256-GCM) at
~/.devmentorai/credentials,0600, referenced from profiles by name and resolved to values only when injecting the agent's process env — a test asserts values never reach an API response, log or error. The threat model is written down honestly in ADR-0007 rather than overclaimed: the key sits beside the ciphertext, so this protects against accidental disclosure (backups, logs, casual reads), not against someone who already controls the local account.Auth (R-013):
authMethodsfrominitializeare exposed and persisted,authenticateis added to the connection, and an auth failure maps toauth_requiredcarrying the agent's own instructions — measured against Copilot, which answers{ code: -32000, message: "Authentication required" }and advertisescopilot-login→ "Runcopilot loginin the terminal".AcpAgentServiceexposes exactly the surface Phase 3's gateway will publish asui/agents.*(list, install, uninstall, profile CRUD, authenticate, resolveLaunch) with no transport attached.Requirements: R-010..R-016, R-062. The conformance probe (R-017) is Phase 5 per the roadmap.
Type of Change
Related Issues
Stacked on #60 → #59.
Checklist
pnpm typecheck)pnpm lint) — scoped Biome clean; repo-wide run is pre-existing-red on unrelated filespnpm test) — backend 167, extension 166; E2E skipped (no Chromium here)Screenshots (if applicable)
n/a
Link to Devin session: https://app.devin.ai/sessions/bab32da5729e4a95a9cb79f1648f005e
Requested by: @BOTOOM