feat: pool ACP provider runtimes - #186
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 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:
📝 WalkthroughWalkthroughThis change adds ACP-backed local-agent support for Cursor and Copilot. It manages executable discovery, child processes, ACP sessions, streamed responses, configuration overrides, permission selection, session reuse, resumption, cleanup, and runtime liveness. ChangesACP local-agent support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LocalAgentAdapters
participant AcpLocalAgentDriver
participant AcpRuntime
participant ACPProcess
participant ACPConnection
LocalAgentAdapters->>AcpLocalAgentDriver: create Cursor or Copilot driver
AcpLocalAgentDriver->>ACPProcess: resolve and spawn provider executable
AcpLocalAgentDriver->>ACPConnection: initialize ACP connection
AcpRuntime->>ACPConnection: create or resume session
ACPConnection->>ACPProcess: send prompt and configuration
ACPProcess-->>ACPConnection: stream agent updates
ACPConnection-->>AcpRuntime: deliver session updates
AcpRuntime-->>AcpLocalAgentDriver: return final response
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThe PR adds pooled ACP runtimes for Cursor and Copilot, including shared process lifecycle management and capability-aware sessions.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/local-agent-acp.ts | Implements pooled ACP process and session lifecycle, configuration, permissions, command resolution, bounded stderr consumption, and local cleanup. |
| src/local-agent-acp.test.ts | Exercises session reuse and resume, configuration overrides, permission selection, command arguments, cleanup, and liveness. |
| src/local-agent-adapters.ts | Routes Cursor and Copilot providers through the new ACP driver. |
Sequence Diagram
sequenceDiagram
participant Pool as Runtime Pool
participant Runtime as ACP Runtime
participant Provider as Cursor/Copilot ACP
Pool->>Runtime: acquire pooled runtime
Runtime->>Provider: session/new or session/resume
Runtime->>Provider: session/set_config_option
Runtime->>Provider: session/prompt
Provider-->>Runtime: session updates and response
Runtime-->>Pool: final response and session ID
Pool->>Runtime: releaseSession
Runtime->>Provider: session/close when supported
Reviews (2): Last reviewed commit: "fix: fail closed on Copilot ACP permissi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51416e33f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| export class AcpLocalAgentDriver implements LocalAgentDriver { |
There was a problem hiding this comment.
This exported driver never becomes reachable from the production subagent path: LocalAgentManager is constructed from createLocalAgentDrivers() in src/local-agent-adapters.ts, and a repo-wide search shows no import or registration of AcpLocalAgentDriver, so Cursor/Copilot still use the old per-run ACP adapter and no pooled ACP connection is used. Please wire this driver into the driver factory when enabling the runtime pool.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No change in this layer: ACP registration is intentionally completed by the final integration layer (#188), which registers Cursor and Copilot through AcpLocalAgentDriver. This intermediate PR isolates the pooled ACP runtime.
| mcpServers: [], | ||
| }); | ||
| this.queues.set(input.providerSessionId, { values: [] }); | ||
| await this.configureSession(input.providerSessionId, input, response); |
There was a problem hiding this comment.
When continuing a stored Cursor/Copilot session that has a profile model or thinking value, LocalAgentManager sends those values again, but session/resume is allowed to return an empty result once the session is ready. Passing that empty resume response into configureSession makes resolveAcp*ConfigUpdate throw ACP server does not expose a model/thinking option before the prompt is sent; cache the config IDs/state from session creation or skip reconfiguration when resume does not include config options.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 5c76011. On a cold resume, persisted model/thinking state no longer requires resume to re-advertise config metadata. A genuinely new per-turn override still fails closed if the provider exposes no config option to apply it, avoiding a silently ignored override.
51416e3 to
e8d6187
Compare
e8d6187 to
9e39ad5
Compare
b224c0f to
ca72c35
Compare
4bab3cb to
15e0fae
Compare
15e0fae to
e132c6b
Compare
54ca5d9 to
1f852ed
Compare
1f852ed to
561c9e8
Compare
2889f8e to
dc65494
Compare
dc65494 to
71b298d
Compare
71b298d to
821f7de
Compare
821f7de to
32b61c3
Compare
32b61c3 to
1077fa9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/local-agent-acp.test.ts (1)
160-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the Cursor write-mode variants.
The assertions cover Cursor only for
writeMode: "allowed". Copilot covers three modes. The Cursor branch adds--mode planforread_onlyand--sandbox disabled --forceforfull_access. These flags control sandboxing, so assert them.💚 Proposed additional assertions
assert.deepEqual(acpCommandArgs("cursor", cachedContext), [ "acp", "--sandbox", "enabled", "--workspace", "/tmp/project", ]); +assert.deepEqual(acpCommandArgs("cursor", { ...cachedContext, writeMode: "read_only" }), [ + "acp", "--sandbox", "enabled", "--workspace", "/tmp/project", "--mode", "plan", +]); +assert.deepEqual(acpCommandArgs("cursor", { ...cachedContext, writeMode: "full_access" }), [ + "acp", "--sandbox", "disabled", "--workspace", "/tmp/project", "--force", +]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-acp.test.ts` around lines 160 - 171, Extend the assertions around acpCommandArgs("cursor", cachedContext) to cover writeMode "read_only" and "full_access". Verify that read_only adds "--mode", "plan", while full_access uses "--sandbox", "disabled", and "--force", preserving the existing allowed-mode assertion.src/local-agent-acp.ts (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicated launch arguments and the unused runtime options.
ACP_COMMANDScarries the subcommand (acp,--acp), andacpCommandArgsrepeats it at Line 387 and Line 398. Only the executable name is read fromACP_COMMANDS(Line 246, Line 365). The two sources can drift.
AcpRuntimeOptions.commandandAcpRuntimeOptions.argsare assigned bycreateRuntimebut never read byAcpRuntime. Either expose them for diagnostics or drop them from the options type.Also applies to: 43-54, 383-406
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/local-agent-acp.ts` around lines 20 - 23, Remove subcommand entries from ACP_COMMANDS so it maps each AcpProvider only to its executable, and update the acpCommandArgs construction to use that executable mapping without duplicating launch arguments. Remove AcpRuntimeOptions.command and AcpRuntimeOptions.args from the type and from createRuntime assignments, since AcpRuntime never reads them.src/local-agent-adapters.ts (1)
64-65: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRoute ACP execution through
AcpLocalAgentDriver.The legacy
cursor/copilotadapter remains exported and ignoreswriteMode. No production caller currently uses it, but a future caller could bypass the Copilot permission policy. Remove this path or reuse the driver policy.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/local-agent-acp.ts`:
- Around line 360-379: Update resolveAcpCommand and its related executable
resolution path to test candidate availability with a filesystem permission
check such as fs.accessSync(candidate, fs.constants.X_OK), rather than calling
executableExists. Ensure PATH discovery never executes candidates, while
preserving the existing command selection, platform extension handling, and
undefined results for unavailable commands.
- Around line 92-102: Update run and the per-session queue handling to serialize
concurrent turns for the same sessionId, or reject a second run while one is
active. Ensure each session/prompt request exclusively owns its notification
updates so queue.values clearing and splicing cannot lose or mix chunks between
turns, while preserving concurrency across different sessions.
- Around line 291-311: Update the ACP initialization flow in createRuntime to
race connection.agent.request(methods.agent.initialize, ...) against a timeout,
ensuring timeout rejection reaches the existing catch block for child
termination and stderrTail attachment. Replace the hardcoded clientInfo version
with the version read from the package manifest.
- Around line 116-123: Update releaseSession to gate the session/close request
only on this.capabilities.close and this.isAlive(); remove the resume capability
requirement while preserving the existing cleanup and request behavior.
- Around line 224-226: Update additionalDirectoryParams and the related
resume/close capability checks to treat capabilities.additionalDirectories as
enabled only when its value is true, keeping detection consistent across all
call sites. When enabled, populate additionalDirectories with the actual allowed
directories; otherwise omit the parameter and capability entirely.
---
Nitpick comments:
In `@src/local-agent-acp.test.ts`:
- Around line 160-171: Extend the assertions around acpCommandArgs("cursor",
cachedContext) to cover writeMode "read_only" and "full_access". Verify that
read_only adds "--mode", "plan", while full_access uses "--sandbox", "disabled",
and "--force", preserving the existing allowed-mode assertion.
In `@src/local-agent-acp.ts`:
- Around line 20-23: Remove subcommand entries from ACP_COMMANDS so it maps each
AcpProvider only to its executable, and update the acpCommandArgs construction
to use that executable mapping without duplicating launch arguments. Remove
AcpRuntimeOptions.command and AcpRuntimeOptions.args from the type and from
createRuntime assignments, since AcpRuntime never reads them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 389972bc-938c-40be-819d-bc4ecbb28808
📒 Files selected for processing (3)
src/local-agent-acp.test.tssrc/local-agent-acp.tssrc/local-agent-adapters.ts
1077fa9 to
8ca3f2e
Compare
8ca3f2e to
4f6618b
Compare
5c76011 to
d48a195
Compare
d48a195 to
eac2264
Compare
1 participant
Cursor and Copilot ACP connections support multiple sessions on one long-lived connection. This stacked change adds shared ACP runtimes with capability-aware session resume and close behavior, session-level model/thinking configuration, permission selection, and process crash invalidation for each provider independently.
Summary by CodeRabbit
New Features
Tests