Uh oh!
There was an error while loading. Please reload this page.
Add ACP registry provider support - #2439
Conversation
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 Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: ACP Registry agents use incorrect Cursor-specific auth
- Added authMethodId and clientCapabilities to CursorAdapterLiveOptions, forwarded them through makeCursorAdapter to makeCursorAcpRuntime, and set AcpRegistryDriver to use authMethodId: "none" with empty clientCapabilities instead of the Cursor-specific defaults.
Or push these changes by commenting:
@cursor push 78fb05040a
Preview (78fb05040a)
diff --git a/apps/server/src/provider/Drivers/AcpRegistryDriver.ts b/apps/server/src/provider/Drivers/AcpRegistryDriver.ts--- a/apps/server/src/provider/Drivers/AcpRegistryDriver.ts+++ b/apps/server/src/provider/Drivers/AcpRegistryDriver.ts@@ -137,6 +137,8 @@
readyReason: "ACP session ready",
applyCursorModelOptions: false,
normalizeModel: (model) => model?.trim() || "default",
+ authMethodId: "none",+ clientCapabilities: {},
...(eventLoggers.native ? { nativeEventLogger: eventLoggers.native } : {}),
spawn: ({ cwd, environment: spawnEnv }) => ({
command: effectiveConfig.command.trim(),
diff --git a/apps/server/src/provider/Layers/CursorAdapter.ts b/apps/server/src/provider/Layers/CursorAdapter.ts--- a/apps/server/src/provider/Layers/CursorAdapter.ts+++ b/apps/server/src/provider/Layers/CursorAdapter.ts@@ -101,6 +101,8 @@
};
readonly normalizeModel?: (model: string | null | undefined) => string;
readonly applyCursorModelOptions?: boolean;
+ readonly authMethodId?: string;+ readonly clientCapabilities?: EffectAcpSchema.InitializeRequest["clientCapabilities"];
/**
* Selections are honored when `modelSelection.instanceId` matches this value.
* Defaults to the legacy built-in instance id (`cursor`).
@@ -546,6 +548,10 @@
cwd,
...(resumeSessionId ? { resumeSessionId } : {}),
clientInfo: { name: "t3-code", version: "0.0.0" },
+ ...(options?.authMethodId ? { authMethodId: options.authMethodId } : {}),+ ...(options?.clientCapabilities+ ? { clientCapabilities: options.clientCapabilities }+ : {}),
...acpNativeLoggers,
}).pipe(
Effect.provideService(Scope.Scope, sessionScope),You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 46fc8ff. Configure here.
| readonly env?: NodeJS.ProcessEnv; | ||
| }; | ||
| readonly normalizeModel?: (model: string | null | undefined) => string; | ||
| readonly applyCursorModelOptions?: boolean; |
There was a problem hiding this comment.
ACP Registry agents use incorrect Cursor-specific auth
Medium Severity
The CursorAdapterLiveOptions interface was extended with provider, spawn, normalizeModel, and applyCursorModelOptions but not with authMethodId or clientCapabilities. The AcpRegistryDriver calls makeCursorAdapter which internally calls makeCursorAcpRuntime, where these default to "cursor_login" and CURSOR_PARAMETERIZED_MODEL_PICKER_CAPABILITIES. Generic ACP registry agents don't support Cursor login, so session initialization will use incorrect auth and capability negotiation. The CursorAcpRuntimeInput was extended with optional authMethodId and clientCapabilities in this same PR, but the adapter layer doesn't forward them.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 46fc8ff. Configure here.
ApprovabilityVerdict: Needs human review This PR introduces a significant new feature (ACP Registry provider support) with binary download/installation capabilities and extensive new UI workflows. There is also an unresolved medium-severity bug regarding incorrect auth configuration for ACP agents. The scope and nature of these changes warrant human review. You can customize Macroscope's approvability policy. Learn more. |
Uh oh!
There was an error while loading. Please reload this page.
| interface PendingApproval { | ||
| readonly decision: Deferred.Deferred<ProviderApprovalDecision>; | ||
| } | ||
There was a problem hiding this comment.
🟢 LowLayers/GenericAcpAdapter.ts:80
respondToUserInput reads from ctx.pendingUserInputs (line 666), but nothing in the adapter ever writes entries to this map. The map is created at line 241 but remains empty, so every call to respondToUserInput returns "Unknown pending user-input request". Unlike pendingApprovals which is populated in the handleRequestPermission callback, the corresponding population logic for user inputs is missing. If this adapter is intended to support user input requests, the handler that populates pendingUserInputs needs to be added.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/GenericAcpAdapter.ts around line 80:
`respondToUserInput` reads from `ctx.pendingUserInputs` (line 666), but nothing in the adapter ever writes entries to this map. The map is created at line 241 but remains empty, so every call to `respondToUserInput` returns "Unknown pending user-input request". Unlike `pendingApprovals` which is populated in the `handleRequestPermission` callback, the corresponding population logic for user inputs is missing. If this adapter is intended to support user input requests, the handler that populates `pendingUserInputs` needs to be added.
Evidence trail:
apps/server/src/provider/Layers/GenericAcpAdapter.ts line 241 (map creation), line 397 (stored in ctx), line 666 (read via .get()). git_grep for `pendingUserInputs.set(` returns no results. git_grep for `pendingApprovals.set(` returns line 304, confirming the parallel pattern exists for approvals but is missing for user inputs. All references to pendingUserInputs: lines 92, 120, 123, 191, 241, 397, 631, 666 — none are writes.
| } | ||
| }).pipe(Effect.scoped); | ||
| const extractArchive = (archivePath: string, destinationDir: string) => { |
There was a problem hiding this comment.
🟢 Lowacp/AcpRegistryBinaryInstaller.ts:220
On Windows, extractArchive calls PowerShell with -LiteralPath and -DestinationPath as separate arguments, but -Command joins all arguments with spaces into a single command string. If archivePath or destinationDir contain spaces, the paths are split incorrectly and Expand-Archive fails. For example, a path like C:\Users\John Smith\Temp\file.zip becomes two tokens (C:\Users\John and Smith\Temp\file.zip). Consider passing the entire command as a single quoted string to -Command, or using proper escaping.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/acp/AcpRegistryBinaryInstaller.ts around line 220:
On Windows, `extractArchive` calls PowerShell with `-LiteralPath` and `-DestinationPath` as separate arguments, but `-Command` joins all arguments with spaces into a single command string. If `archivePath` or `destinationDir` contain spaces, the paths are split incorrectly and `Expand-Archive` fails. For example, a path like `C:\Users\John Smith\Temp\file.zip` becomes two tokens (`C:\Users\John` and `Smith\Temp\file.zip`). Consider passing the entire command as a single quoted string to `-Command`, or using proper escaping.
Evidence trail:
- `apps/server/src/provider/acp/AcpRegistryBinaryInstaller.ts` lines 225-233 (REVIEWED_COMMIT): PowerShell invocation passes archivePath and destinationDir as separate args after `-Command`
- `https://github.com/PowerShell/PowerShell` `src/Microsoft.PowerShell.ConsoleHost/host/msh/CommandLineParameterParser.cs` line 1552: `_commandLineCommand = string.Join(' ', args, i, args.Length - i);` — confirms args after `-Command` are joined with spaces from the already-parsed (quotes-stripped) argv
- Same file lines 1041-1048: `_commandHasArgs` is only true for `-commandwithargs`/`-cwa`, not for `-command`/`-c`
- Same file line 1629: `private bool _commandHasArgs;` — defaults to false
| const stopSessionInternal = (ctx: GenericAcpSessionContext) => | ||
| Effect.gen(function* () { | ||
| if (ctx.stopped) return; | ||
| ctx.stopped = true; | ||
| yield* settlePendingApprovalsAsCancelled(ctx.pendingApprovals); | ||
| yield* settlePendingUserInputsAsEmptyAnswers(ctx.pendingUserInputs); | ||
| if (ctx.notificationFiber) { | ||
| yield* Fiber.interrupt(ctx.notificationFiber); | ||
| } | ||
| yield* Effect.ignore(Scope.close(ctx.scope, Exit.void)); | ||
| sessions.delete(ctx.threadId); | ||
| yield* offerRuntimeEvent({ | ||
| type: "session.exited", | ||
| ...(yield* makeEventStamp()), | ||
| provider: providerKind, | ||
| threadId: ctx.threadId, | ||
| payload: { exitKind: "graceful" }, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 MediumLayers/GenericAcpAdapter.ts:186
threadLocksRef leaks memory: getThreadSemaphore creates and stores a Semaphore for each unique threadId, but stopSessionInternal only removes the session from sessions without removing the corresponding semaphore entry. Over time the map grows unboundedly with every stopped session. Consider deleting the semaphore entry when stopping a session, or using a cleanup mechanism like Effect.addFinalizer in withThreadLock.
const stopSessionInternal = (ctx: GenericAcpSessionContext) =>
Effect.gen(function* () {
if (ctx.stopped) return;
ctx.stopped = true;
+ yield* SynchronizedRef.update(threadLocksRef, (map) => {+ const next = new Map(map);+ next.delete(ctx.threadId);+ return next;+ });
yield* settlePendingApprovalsAsCancelled(ctx.pendingApprovals);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/Layers/GenericAcpAdapter.ts around lines 186-204:
`threadLocksRef` leaks memory: `getThreadSemaphore` creates and stores a `Semaphore` for each unique `threadId`, but `stopSessionInternal` only removes the session from `sessions` without removing the corresponding semaphore entry. Over time the map grows unboundedly with every stopped session. Consider deleting the semaphore entry when stopping a session, or using a cleanup mechanism like `Effect.addFinalizer` in `withThreadLock`.
Evidence trail:
apps/server/src/provider/Layers/GenericAcpAdapter.ts lines 148-149 (sessions map and threadLocksRef creation), lines 158-168 (getThreadSemaphore adding entries), lines 186-203 (stopSessionInternal deleting from sessions but not threadLocksRef). git_grep for 'threadLocksRef' confirms no removal of entries anywhere in the codebase.
| const json = yield* Effect.try({ | ||
| try: () => JSON.parse(raw.value) as unknown, | ||
| catch: () => null, | ||
| }); | ||
| if (json === null) return null; |
There was a problem hiding this comment.
🟡 Mediumacp/AcpRegistryBinaryInstaller.ts:177
When JSON.parse throws on invalid JSON, Effect.try maps the exception to null as an error value, not a success value. The yield* then short-circuits the generator with failure rather than assigning null to json. This contradicts the intended behavior of treating invalid manifest files as "no manifest" — instead, parse errors propagate and can fail the entire listAcpRegistryAgents call. Consider using Effect.option (like on line 182-184) or Effect.either to catch and handle the parse error locally.
- const json = yield* Effect.try({- try: () => JSON.parse(raw.value) as unknown,- catch: () => null,- });- if (json === null) return null;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/provider/acp/AcpRegistryBinaryInstaller.ts around lines 177-181:
When `JSON.parse` throws on invalid JSON, `Effect.try` maps the exception to `null` as an *error* value, not a success value. The `yield*` then short-circuits the generator with failure rather than assigning `null` to `json`. This contradicts the intended behavior of treating invalid manifest files as "no manifest" — instead, parse errors propagate and can fail the entire `listAcpRegistryAgents` call. Consider using `Effect.option` (like on line 182-184) or `Effect.either` to catch and handle the parse error locally.
Evidence trail:
File: apps/server/src/provider/acp/AcpRegistryBinaryInstaller.ts lines 165-200 (viewed at REVIEWED_COMMIT). Effect.try API behavior confirmed via Effect-TS community patterns on GitHub (e.g., https://github.com/jpb06/effect-errors, https://github.com/Effect-TS/effect/issues/3563) showing catch callback returns the error channel value, not the success channel value.
- Introduce ACP Registry driver and client scaffolding - Thread provider icon URLs through server and UI models - Expand Cursor ACP adapter hooks for alternate providers
- Route ACP registry sessions through a generic ACP runtime - Make ACP auth optional and skip Cursor-only extensions for generic providers
- Route ACP registry through the shared generic adapter - Simplify cursor adapter session setup and model selection - Preserve cursor-specific extension handling in the cursor layer
- Move ACP binary install and launch resolution into a shared module - Switch stream exposure to a getter so runtime events stay fresh - Keep WebSocket RPC handlers thin and reuse registry install helpers
- Convert ACP registry binary path helpers to Effect-based access - Reuse shared path resolution for install previews and installs - Keep archive command validation and manifest lookup behavior intact
- Normalize search tokens and score field matches - Sort dialog results by best query match instead of substring order
0a467a9 to
9b74682CompareThe latest updates on your projects. Learn more about Vercel for GitHub.
|
juliusmarminge
commented
Jun 19, 2026
will be part of #2829 |
Summary
ACP Registryprovider driver backed by the ACP registry flow, including session startup and model selection handling.iconUrlthrough server snapshot construction and driver creation.Testing
Note
High Risk
High risk because it introduces a new provider driver that spawns external ACP agent processes and adds server RPCs that download/extract/install binaries from remote registry URLs, expanding the attack surface and potential platform-specific failure modes.
Overview
Adds first-class ACP Registry support end-to-end: a new
AcpRegistryDriver(multi-instance) built on the existing ACP/Cursor session adapter, plus contracts/settings updates to persist ACP registry configuration and defaults.Extends provider instance/snapshot metadata to carry
iconUrlthrough the server registry, unavailable snapshots, and UI rendering, and updates several drivers to accept/pass throughiconUrl.Generalizes
makeCursorAdapterso it can be reused by non-Cursor ACP providers (customproviderkind, spawn command/args/env, model normalization, optional Cursor-specific model-option application, and customizable “ready” reason).Adds server websocket RPCs
server.listAcpRegistryandserver.installAcpRegistryBinarythat fetch the ACP registry index and optionally download/extract an agent binary into the server state dir, and updates the web UI wizard/model pickers to browse registry agents, install binaries, and create ACP registry-backed provider instances (including env var editing and hiding the default ACP registry catalog entry from model selection).Reviewed by Cursor Bugbot for commit 46fc8ff. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add ACP Registry provider support with binary install and model picker integration
acpRegistryprovider driver that spawns a configured command as an ACP agent, registers it as a built-in driver, and exposes it in settings with configurablecommandandargsfields.server.listAcpRegistryandserver.installAcpRegistryBinary) so the client can browse a remote registry index, resolve per-platform launch specs, and install binary agents.isModelPickerProviderInstanceEntryto exclude the default ACP Registry catalog instance from model selection while keeping explicitly imported agents visible.iconUrlthrough provider snapshots,ProviderInstanceConfig, driver create inputs, and UI components (ProviderInstanceIcon,ModelListRow,ModelPickerContent, etc.) with image-load fallback to glyph or initials.argsfields in existing provider instance configs are now stored and parsed asstring[]; any code readingargsas a plain string will need updating.📊 Macroscope summarized 121f70b. 5 files reviewed, 1 issue evaluated, 0 issues filtered, 1 comment posted
🗂️ Filtered Issues