feat(providers): add ACP registry for Gemini, Pi, and more - #6071
feat(providers): add ACP registry for Gemini, Pi, and more#6071adhyaay-karnwal wants to merge 1 commit into
Conversation
Ship one generic acpRegistry driver with a featured catalog and live ACP index so extra agents can be added as catalog rows without new adapters. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
vouch:unvouched
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed the new Effect service code in this PR (AcpRegistryDriver.ts, AcpRegistryProvider.ts, GenericAcpAdapter.ts, GenericAcpSupport.ts, AcpRegistryCatalog.ts, contracts/settings/rpc additions). Module namespace imports, Effect.fn usage, layer/Effect.service acquisition in the driver and catalog, and Schema.TaggedErrorClass usage all look consistent with the conventions.
Two convention issues, both in newly added server code:
GenericAcpSupport.makeGenericAcpRuntimetakes aChildProcessSpawner["Service"]instance and re-provides it throughLayer.succeed, so an owned Effect dependency is injected as a value instead of being required from the environment.GenericAcpAdapterwraps failures withdetail: cause.message, which makes the wrapper's caller-visiblemessagea restatement of the cause instead of stable structural context.
AcpRegistryDriver.ts has the same cause-derived detail shape (detail: \Failed to build ACP Registry snapshot: ${cause.message ?? String(cause)}`); it mirrors the existing drivers, so it is noted here rather than commented inline, but new code should prefer a fixed structural detail with the failure preserved only as cause`.
Posted via Macroscope — Effect Service Conventions
| export interface GenericAcpRuntimeInput extends Omit< | ||
| AcpSessionRuntime.AcpSessionRuntimeOptions, | ||
| "spawn" | ||
| > { | ||
| readonly childProcessSpawner: ChildProcessSpawner.ChildProcessSpawner["Service"]; |
There was a problem hiding this comment.
makeGenericAcpRuntime accepts ChildProcessSpawner.ChildProcessSpawner["Service"] and re-provides it with Layer.succeed(...) (line 58), while production callers (GenericAcpAdapter.startSession, AcpRegistryProvider.discoverModelsViaAcp) do yield* ChildProcessSpawner.ChildProcessSpawner only to hand the instance back in. That is service-instance injection for a dependency this runtime owns, and it hides the requirement from the returned Effect type.
Suggest dropping the childProcessSpawner field, letting Layer.build(AcpSessionRuntime.layer(input)) take the spawner from the environment, and widening the return type to Effect.Effect<..., EffectAcpErrors.AcpError, ChildProcessSpawner.ChildProcessSpawner | Crypto.Crypto | Scope.Scope> so callers can drop their manual yield*/pass-through.
Posted via Macroscope — Effect Service Conventions
| new ProviderAdapterProcessError({ | ||
| provider: PROVIDER, | ||
| threadId: input.threadId, | ||
| detail: cause.message, |
There was a problem hiding this comment.
detail here just copies cause.message, and ProviderAdapterProcessError.message is built from detail, so the wrapper message is derived from the cause rather than from stable structural attributes. Suggest a fixed detail describing the stage and keeping the underlying failure only as cause.
| detail: cause.message, | |
| detail: "Failed to start the ACP agent process.", |
Posted via Macroscope — Effect Service Conventions
| new ProviderAdapterRequestError({ | ||
| provider: PROVIDER, | ||
| method: "session/prompt", | ||
| detail: cause.message, |
There was a problem hiding this comment.
Same here: detail mirrors cause.message, so the request error's message restates the filesystem failure instead of the operation context. The attachment id is already available and is safe, bounded context.
| detail: cause.message, | |
| detail: `Failed to read attachment '${attachment.id}' from disk.`, |
Posted via Macroscope — Effect Service Conventions
| onValueChange={(value) => { | ||
| const parsed = parseAddProviderPickerValue(value); | ||
| setDriver(parsed.driver); | ||
| setCatalogId(parsed.catalogId); |
There was a problem hiding this comment.
🟡 Medium settings/AddProviderInstanceDialog.tsx:254
All ACP featured agents share the acpRegistry driver, so the featured-agent selection handler stores every agent's config draft under the same configByDriver[acpRegistry] key. Selecting a second featured agent overwrites the first agent's edited command/arguments, and switching back to the first agent recreates defaults instead of restoring the in-progress input. This breaks the dialog's draft-preservation behavior. Consider keying ACP drafts by catalog ID (or per picker entry) so each featured agent retains its own in-progress config.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/settings/AddProviderInstanceDialog.tsx around line 254:
All ACP featured agents share the `acpRegistry` driver, so the featured-agent selection handler stores every agent's config draft under the same `configByDriver[acpRegistry]` key. Selecting a second featured agent overwrites the first agent's edited command/arguments, and switching back to the first agent recreates defaults instead of restoring the in-progress input. This breaks the dialog's draft-preservation behavior. Consider keying ACP drafts by catalog ID (or per picker entry) so each featured agent retains its own in-progress config.
| }; | ||
| } | ||
|
|
||
| if (agent.distribution.npx) { |
There was a problem hiding this comment.
🟠 High acp/AcpRegistryCatalog.ts:54
registryEntry builds a launch command for live-registry npx and uvx agents (npx -y <package> / uvx <package>), so selecting one downloads and executes code from the remote registry. This contradicts the module's stated invariant that live entries must not download remote executables — only platform-binary-only entries are treated as unsupported. A compromised registry row can trigger unprompted remote package execution. Live npx/uvx entries should return distributionType: "unsupported" with launch: null, unless they resolve to a locally installed command.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/acp/AcpRegistryCatalog.ts around line 54:
`registryEntry` builds a launch command for live-registry `npx` and `uvx` agents (`npx -y <package>` / `uvx <package>`), so selecting one downloads and executes code from the remote registry. This contradicts the module's stated invariant that live entries must not download remote executables — only platform-binary-only entries are treated as unsupported. A compromised registry row can trigger unprompted remote package execution. Live `npx`/`uvx` entries should return `distributionType: "unsupported"` with `launch: null`, unless they resolve to a locally installed command.
| ) | ||
| : null; | ||
|
|
||
| const discovered = yield* discoverModelsViaAcp(settings, environment).pipe( |
There was a problem hiding this comment.
🟡 Medium Layers/AcpRegistryProvider.ts:233
checkAcpRegistryProviderStatus always returns status: "ready" once the --version command succeeds, even when discoverModelsViaAcp fails or times out. An executable that responds to --version but is not an ACP server, or an ACP server that fails initialization or authentication, is reported as installed: true and status: "ready" with the message "ACP command found. Models will load when a session starts." This masks the actual failure and misleads users into thinking the provider is healthy.
The issue is that discoverModelsViaAcp is wrapped with Effect.timeoutOption followed by Effect.option, which converts both timeouts and runtime failures into Option.none(). Since discoveredModels is then [], the code falls through to the else branch that reports status: "ready" with the fallback models, regardless of whether the failure was a timeout or an actual ACP error. Consider distinguishing failure from timeout/success and setting status: "error" (or "warning") with an appropriate message when the discovery itself errors.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/AcpRegistryProvider.ts around line 233:
`checkAcpRegistryProviderStatus` always returns `status: "ready"` once the `--version` command succeeds, even when `discoverModelsViaAcp` fails or times out. An executable that responds to `--version` but is not an ACP server, or an ACP server that fails initialization or authentication, is reported as `installed: true` and `status: "ready"` with the message "ACP command found. Models will load when a session starts." This masks the actual failure and misleads users into thinking the provider is healthy.
The issue is that `discoverModelsViaAcp` is wrapped with `Effect.timeoutOption` followed by `Effect.option`, which converts both timeouts and runtime failures into `Option.none()`. Since `discoveredModels` is then `[]`, the code falls through to the `else` branch that reports `status: "ready"` with the fallback models, regardless of whether the failure was a timeout or an actual ACP error. Consider distinguishing failure from timeout/success and setting `status: "error"` (or `"warning"`) with an appropriate message when the discovery itself errors.
| return c !== undefined && !c.stopped; | ||
| }); | ||
|
|
||
| const stopAll: ProviderAdapterShape<ProviderAdapterError>["stopAll"] = () => |
There was a problem hiding this comment.
🟡 Medium Layers/GenericAcpAdapter.ts:1407
stopAll iterates a one-time snapshot of sessions and calls stopSessionInternal without acquiring the per-thread lock, so a concurrent startSession on the same thread can insert a new live context after the snapshot is taken. stopAll then returns while a child ACP process for that thread remains running. Acquiring the thread lock inside stopAll before stopping each session would prevent the race with concurrent startSession.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Layers/GenericAcpAdapter.ts around line 1407:
`stopAll` iterates a one-time snapshot of sessions and calls `stopSessionInternal` without acquiring the per-thread lock, so a concurrent `startSession` on the same thread can insert a new live context after the snapshot is taken. `stopAll` then returns while a child ACP process for that thread remains running. Acquiring the thread lock inside `stopAll` before stopping each session would prevent the race with concurrent `startSession`.
| * Split a launch-args string the way Codex/Claude settings do: whitespace | ||
| * separated, with simple single/double quotes. | ||
| */ | ||
| export function parseAcpLaunchArgs(value: string | null | undefined): ReadonlyArray<string> { |
There was a problem hiding this comment.
🟠 High src/acpRegistry.ts:213
parseAcpLaunchArgs is not a true inverse of formatAcpLaunchArgs, so round-tripping launch args through settings corrupts the argument list in two ways.
First, formatAcpLaunchArgs escapes inner double quotes as \" inside a quoted argument, but parseAcpLaunchArgs has no escape handling — it keeps the backslashes and treats the " as a quote delimiter, splitting one argument into two. An argument like say "hello world" round-trips into say and hello world\ instead of the original.
Second, empty arguments are silently dropped. formatAcpLaunchArgs(["--value", ""]) emits --value (a trailing space), and parseAcpLaunchArgs only pushes current when it is non-empty, so the empty string is lost. A command that intentionally requires an empty positional or option value is spawned with the wrong argument list.
Consider aligning the parser with the formatter by handling \\ and \" escapes inside double-quoted segments and emitting empty tokens via explicit quoting (e.g. ""), or document that this codec cannot represent arguments containing double quotes or empty strings.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/contracts/src/acpRegistry.ts around line 213:
`parseAcpLaunchArgs` is not a true inverse of `formatAcpLaunchArgs`, so round-tripping launch args through settings corrupts the argument list in two ways.
First, `formatAcpLaunchArgs` escapes inner double quotes as `\"` inside a quoted argument, but `parseAcpLaunchArgs` has no escape handling — it keeps the backslashes and treats the `"` as a quote delimiter, splitting one argument into two. An argument like `say "hello world"` round-trips into `say` and `hello world\` instead of the original.
Second, empty arguments are silently dropped. `formatAcpLaunchArgs(["--value", ""])` emits `--value ` (a trailing space), and `parseAcpLaunchArgs` only pushes `current` when it is non-empty, so the empty string is lost. A command that intentionally requires an empty positional or option value is spawned with the wrong argument list.
Consider aligning the parser with the formatter by handling `\\` and `\"` escapes inside double-quoted segments and emitting empty tokens via explicit quoting (e.g. `""`), or document that this codec cannot represent arguments containing double quotes or empty strings.
| const adapter = yield* makeGenericAcpAdapter( | ||
| { | ||
| enabled: effectiveConfig.enabled, | ||
| command: effectiveConfig.command.trim() || "acp", |
There was a problem hiding this comment.
🟠 High Drivers/AcpRegistryDriver.ts:136
An enabled ACP instance with a blank command silently launches an executable named acp from PATH instead of rejecting the invalid configuration. The makeGenericAcpAdapter call uses effectiveConfig.command.trim() || "acp", so an empty command falls back to the literal string "acp", bypassing the blank-command validation the adapter would otherwise enforce. This contradicts the snapshot/probe path, which treats a blank command as unconfigured/disabled. Require a non-empty command for enabled ACP instances — or at minimum drop the || "acp" fallback so the adapter's own validation rejects the empty value.
| command: effectiveConfig.command.trim() || "acp", | |
| command: effectiveConfig.command.trim(), |
Also found in 1 other location(s)
packages/contracts/src/settings.ts:434
AcpRegistrySettings.commandaccepts an empty string and defaults to"", even though the ACP provider treats an empty command as unconfigured/disabled and rejects session startup. Selecting “Custom ACP” produces no draft command, so the dialog can successfully save an enabled provider instance with blankcommand; it then appears added but cannot run until manually repaired. Require a non-empty command for enabled ACP instances or block saving the custom entry until one is supplied.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/provider/Drivers/AcpRegistryDriver.ts around line 136:
An enabled ACP instance with a blank `command` silently launches an executable named `acp` from `PATH` instead of rejecting the invalid configuration. The `makeGenericAcpAdapter` call uses `effectiveConfig.command.trim() || "acp"`, so an empty command falls back to the literal string `"acp"`, bypassing the blank-command validation the adapter would otherwise enforce. This contradicts the snapshot/probe path, which treats a blank command as unconfigured/disabled. Require a non-empty `command` for enabled ACP instances — or at minimum drop the `|| "acp"` fallback so the adapter's own validation rejects the empty value.
Also found in 1 other location(s):
- packages/contracts/src/settings.ts:434 -- `AcpRegistrySettings.command` accepts an empty string and defaults to `""`, even though the ACP provider treats an empty command as unconfigured/disabled and rejects session startup. Selecting “Custom ACP” produces no draft command, so the dialog can successfully save an enabled provider instance with blank `command`; it then appears added but cannot run until manually repaired. Require a non-empty command for enabled ACP instances or block saving the custom entry until one is supplied.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bba5635. Configure here.
| } | ||
| return input.runtime | ||
| .setSessionModel(input.requestedModelId) | ||
| .pipe(Effect.mapError(input.mapError), Effect.as(input.requestedModelId)); |
There was a problem hiding this comment.
Placeholder model forces set_model
High Severity
When model discovery fails or has not run yet, the ACP registry fallback model is the slug default, and DEFAULT_MODEL_BY_PROVIDER for acpRegistry is also default. applyGenericAcpModelSelection then calls session/set_model with that placeholder whenever it differs from the agent’s real currentModelId (including undefined). That matches the probe path that keeps the instance usable and says models load at session start, so thread start can fail against Gemini/Copilot/Pi and other agents that do not expose a model id named default.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit bba5635. Configure here.
| enabled: effectiveConfig.enabled, | ||
| command: effectiveConfig.command.trim() || "acp", | ||
| args: parseAcpLaunchArgs(effectiveConfig.launchArgs), | ||
| }, |
There was a problem hiding this comment.
Empty command spawns acp binary
Medium Severity
AcpRegistryDriver passes command: effectiveConfig.command.trim() || "acp" into the adapter. startSession only rejects a missing command when settings.command is empty, so an blank/whitespace command is rewritten to acp before that check and the guard never fires. A Custom ACP instance saved without a command can still attempt to spawn a non-existent acp binary instead of failing with a configuration error.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bba5635. Configure here.
ApprovabilityVerdict: Needs human review 6 blocking correctness issues found. This PR introduces a substantial new feature (ACP Registry provider) with ~3100 lines of new code that spawns external CLI processes. Multiple unresolved high-severity findings exist around security (live registry entries could trigger remote package execution) and correctness (empty command fallback, argument parsing issues). New capability of this scope with security implications requires human review. You can customize Macroscope's approvability policy. Learn more. |
1 participant
Summary
acpRegistrydriver so Gemini, GitHub Copilot, Pi, Hermes, Qwen, Kimi, and custom ACP CLIs work through one generic ACP adapter instead of per-agent drivers.server.listAcpRegistrywithout downloading binaries.ACP_FEATURED_AGENTS, not a new adapter.Test plan
vp test runfocused ACP suite (contracts catalog, registry merge, provider probe, mock ACP adapter session, auth scope, add-provider picker, settings form fields) — 47 passinggemini --acp, install CLI on PATH, refresh status, start a threadMade with Cursor
Note
Add ACP registry provider driver supporting Gemini, Pi, and other ACP-speaking CLIs
acpRegistryprovider driver (AcpRegistryDriver.ts) that spawns ACP CLI agents, discovers models via a short-lived session, and streamsProviderRuntimeEvententries through a new GenericAcpAdapter.ts.npx/uvx/local).server.listAcpRegistryWebSocket RPC (ws.ts) fetches and merges the live registry index (8 s timeout) with featured agents, falling back to featured-only on failure.AcpSessionRuntimenow picks an auth method automatically whenauthMethodIdis omitted, selectingnoneif advertised or the first available method.acpRegistryproviders fail immediately withTextGenerationError; only session-based chat is supported.📊 Macroscope summarized bba5635. 27 files reviewed, 0 issues evaluated, 0 issues filtered, 0 comments posted
🗂️ Filtered Issues
No issues evaluated.