fix(acp): route sub-agent tool approvals to the client - #1080
Conversation
A tool call made inside a dispatched sub-agent goes through the global approval slot in tool-approval-queue. Only the Ink TUI installs a handler there, so under ACP the slot fell back to denying, and the sub-agent got "Tool execution was denied by the user." without the client ever being asked. A client that gates writes saw and decided every top-level call and nothing a sub-agent did, so delegated work could only write by bypassing approval. Install a handler for the turn that forwards these to the same session/request_permission channel the top-level calls use. The call is announced first, because a permission request naming a tool call the client has not seen is rejected as invalid params, and the title carries the sub-agent name so the client can tell the two sources apart. A denied or cancelled decision marks the announced call failed. The terminal status after an approval is not emitted, because the sub-agent layer does not report its tool results back to the ACP conversation. That gap predates this change. Closes Nano-Collective#1019
will-lamerton
left a comment
There was a problem hiding this comment.
Thanks for this, the diagnosis is right and the fix is in the right layer. The announce-before-request ordering you asked about is correct: requestToolPermission embeds a ToolCallUpdate keyed on toolCall.id, and the top-level path announces first for the same reason. I applied the branch locally onto current main: spec passes (46), tsc --noEmit clean, format clean.
One thing I'd like changed before merge, plus a few follow-ups.
Blocking: the handler is installed globally and never torn down.
setGlobalToolApprovalHandler is called at the top of runAcpConversation with no matching teardown, and the slot is a module singleton with last-writer-wins semantics. AcpAgent keeps a sessions Map and the overlap guard is session.turnActive, which is per session, so two sessions can have turns in flight at once. If session A dispatches a sub-agent and session B starts a prompt before A's sub-agent asks for approval, A's approval runs through B's closure: the tool_call is emitted with session B's id and requestPermission names a toolCallId B's client has never seen. That is exactly the invalid-params rejection your comment warns about, and it also races the wrong abortController, so cancelling B cancels A's pending approval. Separately, after the turn returns the closure stays live holding a finished turn's session, conn, and aborted controller.
Simplest fix: have GlobalHandlerSlot.set return a disposer (backwards compatible for the existing TUI caller) and wrap the loop in try { ... } finally { restore() }. That confines the closure to the turn; keying the slot by session id, or threading an approval channel through SubagentExecutor, would also close the concurrent-session case.
Worth folding in: a transport failure kills the whole sub-agent run.
requestToolPermission does not catch, and in subagent-executor.ts:724 the await signalToolApproval(...) sits outside the surrounding try, so a rejected requestPermission (closed connection, or the invalid-params case above) propagates and aborts the sub-agent instead of denying one tool. A try/catch around the handler body returning false keeps the safe-fallback posture.
Follow-ups, happy for these to be separate:
- Sub-agent approvals ignore the ACP session's mode and config
alwaysAllow.needsApprovalForToolpasses only{mode: this.currentMode()}, and the ACP executor is constructed insource/plain/initialize.ts:96with the defaultparentMode: 'normal'and no mode resolver. So ayoloorauto-acceptsession still prompts inside a sub-agent while the same tool at top level does not. Pre-existing, but this PR is what makes it visible, so it's worth a line in the changeset. - On the in_progress gap: note it cuts both ways. Sub-agent tools that don't need approval are never announced at all, so a client gets cards only for the approval-gated subset and each of those spins forever. If threading results out is separate work, consider emitting
completedright after approval rather thanin_progress, so nothing is left permanently spinning. - The tests call
signalToolApprovalafterrunAcpConversationhas returned, which only works because of the leak above. They can't catch a regression once teardown is added, and they don't cover the real in-turn case. Driving the signal from inside the turn (e.g. a mock tool handler that calls it) would test the actual ordering and let you assert the session id on the emitted update. - Minor: sub-agent
toolCallIds come from the sub-agent's own model and share an id space with top-level calls, so a collision would merge two cards client-side. Prefixing the announced id would rule it out. - Needs a rebase: the auto-compact work landed in
acp-conversation.tsand new tests landed at the end of the spec since you branched.
The handler was installed on a module singleton with no teardown. Two sessions can have turns in flight at once, since the overlap guard is per session, so a second session's closure could answer the first session's approvals: the tool_call went out with the wrong session id, the permission request named an id that client had never seen, and cancelling one turn cancelled the other's pending approval. A finished turn's session, conn and aborted controller also stayed reachable. GlobalHandlerSlot.set now returns a disposer that restores the previous handler, and only if nobody replaced it meanwhile. runAcpConversation installs the handler, runs the turn, and restores in a finally. The existing TUI caller ignores the return value and is unaffected. Catch inside the handler and deny. signalToolApproval is awaited outside the sub-agent executor's own try, so a rejected requestPermission would abort the whole sub-agent run instead of refusing one tool. Emit completed after an approval rather than in_progress. The sub-agent layer reports no result back here, so the card would otherwise spin forever. Prefix the announced id. Sub-agent tool ids come from the sub-agent's own model and share an id space with top-level calls, so a collision would merge two cards client-side. The tests now signal from inside the turn, through the awaited chat call, rather than after runAcpConversation returned - which only passed because of the leak this fixes. Reported by @will-lamerton in review of Nano-Collective#1080.
|
All taken, plus the rebase. Pushed in d024911. Blocking: handler scopeYou are right, and the concurrent-session case is the worse half. Went with your disposer suggestion.
That also let me split the handler out as Transport failureFolded in. The handler body is wrapped and returns Follow-ups
Mode and The tests. This was the sharpest catch. They passed only because of the leak, and they would have gone green against a broken teardown. They now signal from inside the turn, through the awaited Id collision. Took it. Announced ids are prefixed RebaseMerged current Semgrep still not installed here. |
The changeset shipped with #1080 said "the handler is scoped to the turn that installs it, so concurrent sessions cannot answer each other's approvals". They still can. turnActive is per session (acp-session.ts, guarded in acp-agent.ts), so two sessions can be mid-turn at once, and the approval slot is a process-wide singleton with last-writer-wins semantics. For the overlap the later installer answers the earlier session's approvals against the wrong session id and abort controller - the exact mis-routing the disposer was added to address. What the disposer does fix is the leak past the turn: a finished turn's session, connection and aborted controller no longer stay reachable, and the restore is LIFO-correct, so routing rights hand back when the later turn ends. Say only that, in both the release note and the comment, and name what closing the window would take: keying the slot by session id, or threading an approval channel through SubagentExecutor. Also record the other half-truth. An approved sub-agent call is marked completed at approval time rather than when it runs, because the sub-agent layer does not report results back, so a client sees completed for a tool that may still fail. That was a deliberate trade against leaving the card spinning forever, but it was undocumented. Restore the cancelled-permission test dropped when the sub-agent tests were rewritten to signal from inside the turn. Nothing covered the permission === 'cancelled' branch, so the distinct "Cancelled by user" output was free to regress into the deny path; both messages are now asserted.
Description
A tool call made inside a dispatched sub-agent goes through
signalToolApprovalinsource/utils/tool-approval-queue.ts. That slot is created with a safe fallback of() => false, and the only code that ever installs a handler isuseGlobalHandlerQueues.tsx, the Ink TUI. Under--acpnothing installs one, so every sub-agent tool needing approval was denied without the client seeing asession/request_permission.The visible effect is the one in the issue: a client that gates writes decides every top-level call and nothing a sub-agent does, and delegated work can only write by bypassing approval entirely.
runAcpConversationnow installs a handler for the turn that forwards these to the same channel top-level calls already use.Two details worth calling out
The call is announced before the request.
acp-question.tsdocuments why, and the top-level path does the same thing with the comment "We reuse this call's id (just announced) so the permission request targets a known tool call". A permission request naming a tool call the client has not seen is rejected as invalid params, and sub-agent calls are never announced otherwise. So the handler emits atool_callupdate first, then requests permission against that id.The title carries the sub-agent name, as the issue asks, so a client can tell a sub-agent's call apart from a top-level one.
What this does not do
A denied or cancelled decision marks the announced call
failed. After an approval it is markedin_progressand no terminal status follows, because the sub-agent layer does not report its tool results back to the ACP conversation. That gap predates this change and closing it would mean threading results out ofsubagent-executor, which felt like a separate piece of work rather than something to fold in here. Flagging it because announcing the call is what makes it visible: before this change there was no card at all.Happy to follow up on that if you want it in the same release.
Type of Change
Tests
Three cases in
source/acp/acp-conversation.spec.ts, all failing onmain:They run a turn, which is what installs the handler, then call
signalToolApprovalexactly assubagent-executor.tsdoes. They assert the request reaches the connection, that itstoolCallIdmatches a call announced earlier in the same turn, that the title names the sub-agent, and that deny and cancel both come back false with afailedupdate.Note on the deny and cancel cases:
t.false(approved)alone would pass without this change, since the fallback already denies. Each of those tests also asserts the permission request happened and the call was marked failed, which is the part that needs the handler.Commands run
Not run: Semgrep, not installed here. I also have not driven a real ACP client, so this is verified against the connection mock rather than against Zed. The announce-before-request ordering is the part most worth a second opinion.
Platform: macOS, Node 24, pnpm 11.