fix(daemon): stop auto-approving every ACP permission request - #6135
fix(daemon): stop auto-approving every ACP permission request#6135btagritech wants to merge 5 commits into
Conversation
choosePermissionOutcome() picked an "allow" option the instant a session/request_permission arrived, with no human involved and no deny path in its own logic (it only ever returns an allow_* optionId or null). ACP has no session-scoped permission kind, so Hermes' "allow for this session" option (optionId: allow_session) is sent with kind: "allow_always" -- the same kind as the real "allow always" option. choosePermissionOutcome's first-match .find() on kind picked whichever came first and returned it, silently approving. This affects every ACP agent (Hermes, Kimi, Kilo, Kiro, Vibe, Devin) and every kind of gated action those agents escalate through ACP, not just one plugin's rule -- including Hermes' own pre-existing dangerous-shell-command gate (rm -rf, sudo, force-push, etc). Replace the instant auto-approve with a real pending-decision flow. replyPermission now emits a permission_request agent event over the run's SSE stream (title, description, and only the choices Hermes actually offered for this request) and waits up to 55s for a matching POST /api/runs/:id/permission call. No answer within the timeout is denied, never approved, matching the fail-closed philosophy of Hermes' own dangerous-command gate. Requires threading a runId through AttachAcpSessionOptions so the pending request can be correlated with the answering API call; a request with no runId available denies immediately rather than hanging until timeout. Verified live against a real Hermes ACP session (an MCP tool gated by a plugin's pre_tool_call "approve" directive): a gated call correctly produced the SSE prompt with the exact tool name and arguments, a real POST .../permission relay resolved it and the tool executed for real, and two separate unanswered requests correctly denied on timeout rather than defaulting to allow. Known gap, not addressed here: no apps/web UI renders the new permission_request event yet, so a user driving the actual browser UI sees the run pause for up to 55s with no visible prompt before it reports blocked. Fail-closed, but not yet good UX -- a real frontend card is separate follow-up work.
|
Thanks @btagritech — good catch on a safety gate that was effectively auto-allowing dangerous ACP tool calls. I’m routing this for reviewer/product/QA triage now.
|
|
@btagritech I'm holding off on generating review comments for #6135 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
|
🧪 Queued for QA validation — this PR changes a live agent-run permission path, so it should get a manual QA pass before merge. Nothing needed from you right now; we’ll update here once it’s validated. Thanks for the contribution! 🙏 |
0cd288b made every ACP session/request_permission wait up to 55s for a human answer over SSE, but apps/web has no UI yet that can render or answer that prompt, so every routine tool call (write_file, read_file, execute_code, ...) stalled and failed closed on every ACP run. Scope the pending-decision/approval flow down to Hostinger MCP tool calls (mcp__hostinger__*) -- the one tool family that can actually delete production infrastructure (VMs, firewalls, domains, mailboxes) today. Every other ACP tool call goes back to the pre-fix instant auto-approve via a restored chooseAutoApprovedOptionId(), identical to the old choosePermissionOutcome() this replaced. ACP has no dedicated tool-name field, so isHostingerAcpToolCall()/extractAcpToolName() recover it from the mcp__<server>__<tool> token Hermes embeds in toolCall.title/rawInput.description -- confirmed against real captured run logs from tonight's live testing. Adds regression coverage in acp.test.ts: non-Hostinger auto-approves with no permission_request event, a Hostinger call still raises the prompt and resolves via resolvePendingAcpPermission, and an unanswered Hostinger call still denies (never auto-allows) on timeout. Full universal gating is still the eventual goal once apps/web has a real approval UI; see acp-permission-scoping-handoff.md.
|
@btagritech I'm holding off on generating review comments for #6135 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
apps/web had no surface for the permission_request SSE event added in 0cd288b -- a gated ACP tool call would pause the run for up to 55s with nothing visible before failing closed. Adds a real approval card: - packages/contracts: permission_request/permission_resolved added to DaemonAgentPayload (sse/chat.ts) and PersistedAgentEvent (api/chat.ts). - apps/daemon/src/server.ts: daemonAgentPayloadToPersistedAgentEvent gains matching branches so the request/resolution survive reload and history replay, not just the live stream. - apps/web/src/providers/daemon.ts: translateAgentEvent handles both event kinds; new answerRunPermission(runId, choice) posts to POST /api/runs/:id/permission, distinguishing 404 (run gone) from 409 (already answered/timed out) so a stale click shows the right notice. - apps/web/src/components/AssistantMessage.tsx: new PermissionRequestCard (modeled on SkillPluginCandidateCard) renders the offered choices as buttons, a soft countdown toward the daemon's 55s timeout, and the resolved outcome once answered -- wired into the existing Block/ buildBlocks machinery the same way plugin-candidate is. - i18n: permission.* keys added to types.ts and all 19 locale files (English text in every locale for now -- translation is a separate follow-up, not blocking the feature). - apps/daemon/src/cli.ts: od run permission <runId> --choice <c> [--json], mirroring od run cancel, so the CLI surface isn't left behind (root AGENTS.md's UI/CLI dual-track rule). - apps/daemon/tests/permission-persistence.test.ts: covers the new server.ts persist-mapper branches, mirroring the existing tool_loop persistence test's shape and rationale. pnpm typecheck and pnpm guard (60/60) clean. New test file passes (5/5), and apps/daemon/tests/acp.test.ts (45/45, covering the route this card calls) is unaffected.
|
@btagritech I'm holding off on generating review comments for #6135 because this pull request has merge conflicts right now. Please resolve the conflicts with main and push the updated branch. Once that's done, request or wait for the review to run again and I'll take another look. 🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos. |
Isolating the ACP edit-approval stall (real edits denied with "Edit approval denied by ACP client; file was not modified" after a real run, no permission_request/tool_call ever recorded). Logs the extracted tool name, chosen optionId, and exact timestamp when sendRpcResult is called, tagged [ACP-DEBUG]. Two isolated reproduction attempts came back clean (~3-4ms round trip both times), so the leading hypothesis is conversation-scale-dependent (150+ turns, 300k+ tokens) rather than a routing bug. Notes in .tmp/DEBUG-NOTES-acp-permission-stall.md (gitignored, not in this commit). Intentionally left in place, not a leftover to clean up yet.
|
Thanks @xiaoche-hub — I checked the current UI diff and agree these permission-card states are the key remaining design input. @btagritech, could you add either a short recording or labelled screenshots from the current head covering the pending state, one approved outcome, one denied outcome, and the timeout state? Once those visuals are in the thread, design should have what it needs to finish this pass. |
Investigation writeup for a design conversation, not a fix. Traces a real symptom (repeated "Edit approval denied by ACP client" failures) through to its actual root cause: composeChatUserRequestForAgent() sends the full, unbounded conversation transcript on every turn for 21 of ~24 registered agents (only claude/codebuddy/pi opt out via session resume). In the traced conversation this reached 1,159,779 input tokens -- over Gemini 2.5 Flash's real context window -- with the ACP permission round-trip itself proven correct twice via live reproduction. Documents the existing advisory-only mitigation (buildPriorRunContextWarning, 200k token threshold) and the existing full-reset mechanism (starting a new conversation), and lists open questions without recommending a specific fix.
Why
I hit this myself while wiring a real external integration through Open Design's ACP backend: a human-approval gate for destructive Hostinger API calls, driven through Hermes as the ACP agent. Hermes already has a
pre_tool_callhook that flags a tool call for approval and routes it through whatever human-approval channel is available — when driven through Open Design, that channel is ACP'ssession/request_permission. Testing that gate live, every gated call was silently approved with no pause and no way to deny, even though Hermes' own plugin had explicitly asked for approval.The root cause is in
apps/daemon/src/acp.ts, not on the Hermes side:choosePermissionOutcome()picked anallow_*option the instant asession/request_permissionrequest arrived, with no human involved and no deny path anywhere in its own logic — it only ever returns an allow option ornull. Concretely, ACP has no session-scoped permissionkind, so Hermes' "allow for this session" option (optionId: allow_session) is sent withkind: "allow_always"— the samekindas the real "allow always" option.choosePermissionOutcome's first-match.find()onkindpicked whichever came first and returned it, silently approving. This isn't scoped to my one plugin's rule — it affects every ACP agent (Hermes, Kimi, Kilo, Kiro, Vibe, Devin) and every kind of gated action any of them escalate through ACP, including Hermes' own pre-existing dangerous-shell-command gate (rm -rf,sudo, force-push, etc). A safety gate that always says yes is worse than no gate, since it looks like protection while providing none.What users will see
GET /api/runs/:id/events) emits a newpermission_requestagentevent (title, description, and the specific choices the agent offered) when this happens, and apermission_resolvedevent once it's answered.POST /api/runs/:id/permissionwith{ "choice": string }, answers a pending request for that run.apps/webdoes not yet render a UI card forpermission_request, so a user driving the browser UI today will see the run pause for up to 55s with no visible prompt before it reports blocked/denied. That's a real UX gap, but it's strictly safer than the previous behavior (instant, invisible auto-approval), and a dedicated UI card is natural, separate follow-up work — flagged here rather than bundled into this fix.Surface area
apps/weborapps/desktop(including Electron menu bar)odsubcommand or flag, newtools-dev/tools-pack/tools-prflag, or newOD_*env var/api/*endpoint, new SSE event, or changed shape inpackages/contractsskills/,design-systems/,design-templates/, orcraft/, or change to the skills protocolTRANSLATIONS.mdfor the locale workflow)package.json(dependenciesordevDependencies); workspace-packagepackage.jsonfiles are out of scope. Include a paragraph on what we get vs. what bytes we ship (seeCONTRIBUTING.md→ Code style)POST /api/runs/:id/permissionand thepermission_request/permission_resolvedSSE events are new surface, not yet reflected inpackages/contracts— happy to add typed contract entries for them if maintainers want that in this PR rather than a fast-follow.Screenshots
N/A — no
apps/web/apps/desktopUI surface touched. The observable change is on the daemon's HTTP/SSE API (see "API / contract" above and "Bug fix verification" below for how I actually watched it happen).Bug fix verification
session/request_permissionround-trip end-to-end throughattachAcpSession, which is more scope than I could responsibly take on tonight on top of the fix itself — I didn't want to let an imperfect test gate a real safety fix, and I'd rather be upfront about the gap than fake coverage. I'd welcome guidance on the right harness for this (possibly themocks/ACP-family fixtures already in this repo) and can follow up with one.od run start, produced the newpermission_requestSSE event with the exact tool name/arguments Hermes sent; a realPOST /api/runs/:id/permissioncall resolved it and the tool executed for real; and two separate unanswered requests correctly denied on the 55s timeout instead of defaulting to allow, which is exactly the failure mode this PR closes.Validation
pnpm typecheck— cleanpnpm guard— clean (60/60)pnpm --filter @open-design/daemon test— compared againstmainin two fresh, isolatedgit worktreecheckouts (this commit vs. its immediate parent) to avoid both node_modules cross-contamination and machine load skewing results. Baseline: 23 failed test files / 152 failed tests, all pre-existing (missing Composio/Codex/Cursor/vela credentials and CLIs in this sandbox, unrelated to ACP). This commit: 24 failed files / 150 failed tests. The one-file difference,tests/routes/handoff.test.ts, is abeforeAllstartServerhook timeout (10s) that reproduces identically on both commits when that file is run in isolation — a pre-existing timing flake, not a regression from this change. No ACP- or permission-related test ever appears in either failure list.