feat(daemon): wire ACP host callbacks in bypass mode - #2786
Conversation
Wire the ACP terminal manager and safe-filesystem host callbacks into query startup when the session has a workspace: terminals run with an allowlisted environment and inherit the process-tree owner, filesystem reads and writes are confined to the workspace with size and range limits, and cwd or env overrides on terminal creation are rejected. Permission requests from the agent are auto-allowed, matching the bypass-permissions trust model the SDK path already uses; the AskUserQuestion approval plumbing for ACP moves to the deferred unified permission UX (#2782). Extracted from #2711 (ACP split 8/10).
|
@codex review |
![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: e043635fdc
ℹ️ 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".
| this.ctx.trackAgentProcess(proc as unknown as TrackedAgentProcess), | ||
| onStderr: (data) => logger.warn(`ACP agent stderr: ${data.trimEnd()}`), | ||
| onPermissionRequest: (params) => handleAcpPermissionRequest(params, canUseTool), | ||
| onPermissionRequest: allowAcpPermissionRequest, |
There was a problem hiding this comment.
When the session or global permission mode is acceptEdits, dontAsk, or plan, QueryOptionsBuilder preserves that value in queryOptions.permissionMode, but this unconditional handler still selects an allow option for every ACP permission request. Consequently, an ACP agent can receive approval for terminal execution or filesystem writes despite the user's restrictive setting; only auto-allow when the effective mode is bypassPermissions, and retain approval/denial behavior for other modes.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
…not load The safe-fs FFI backend fails to load inside some CI shard processes where a native-library sibling test poisons the dynamic loader; the backend itself stays covered by its dedicated suite. Probe the backend before the two wiring tests instead of failing the shard, and apply canonical import formatting.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Extracted from #2711 (ACP split 8/10).
What this adds
AcpTerminalManager(allowlisted env viabuildAcpSafeEnv, workspace as default cwd, process-tree owner) and wiresterminal/create|output|wait_for_exit|kill|release. Commands are parsed/normalized,cwd/envoverrides on terminal creation are rejected, and terminals are not created after the query aborts. Manager disposal runs on both the error path (early) and infinally.fs/read_text_file/fs/write_text_fileroute through the workspace-confined safe-fs layer with a 4 MiB read cap, line/limit ranges, realpath-aware boundary checks, and abort-aware writes. Workspace-less sessions register no host callbacks and fall back toprocess.cwd()for the agent cwd.session/request_permissionfrom the agent is auto-allowed (firstallow_*option) — the same trust model the Claude SDK path runs under. The AskUserQuestion-based ACP approval gate that was on dev is removed; the unified approval/permission UX for both providers is deferred design work tracked in ACP security hardening (approval gating + command redaction) — built, intentionally not shipping yet #2782.Deliberately not here
Extracted from #2711.
Verification
bun test tests/unit/lib/acp/acp-query-runner.test.ts— 54/54 (includes real-subprocess terminal test at a 20s budget)tsc --build --noEmit, knip, pinned-biome format — cleantests/unit/lib/acp/sandbox run matches the pre-existing environmental baseline (AcpClient/mock-server noise) except the real-subprocess allowlist test, which passes in isolation and hangs only in the sandbox's polluted full-dir run; CI shard boundaries differ and ran this suite green on the source branch