fix(cli): scope the ACP permission queue to the session (#11802)

* fix(cli): scope the ACP permission queue to the session

The permission queue was keyed on the AgentSideConnection, but a daemon
child owns exactly one connection that every session on it multiplexes
onto. One unanswered question in an idle session therefore head-of-line
blocks every sibling session, and with the mediator timeout disabled by
default the wait is both unbounded and invisible: the blocked request is
never published, so hosts report no pending interaction at all.

Keep the queue -- a single session does have concurrent permission
producers -- and narrow its unit of serialization to the session, which
is what the bridge already models with its per-session promptQueue.

* fix(cli): Remove the unused unqueued Session.requestPermission

SubAgentTracker receives #requestPermissionQueued from the session that
constructs it, and its own default goes straight to the connection, so no
caller ever reached this method. Its docstring named a caller that uses the
queued path, which left an unqueued route onto the shared connection sitting
beside the queue that is meant to own the boundary.

* test(cli): Pin the abandoned-while-queued permission request to the floor

A request whose caller aborts while it is still queued behind its session's
head is dropped instead of published, because an RPC that reaches the client
after its caller is gone becomes a prompt nobody can answer or cancel. That
short-circuit predates the queue-scoping change and no test covered it, so a
mutation that removes it leaves the suite green.

Add the missing case: hold one request unanswered, queue a second, abort the
queued one, then release the head and assert the client was contacted exactly
once. A third request from the same session then proves dropping the abandoned
one did not wedge the queue.

* test(cli): Pin each session's permission settlement and share the approval fixture

The cross-session test asserted only where the request went. It now also
asserts that each session settles its own approval against its own run
registry, so a queue that mis-scoped the receive half as well as the send
half cannot pass. Verified discriminating: forcing the run id to a constant
fails the test.

Four of these tests had each copied the same approval fixture verbatim; one
of them was added here. Hoisted into a single helper, assertions unmoved.

---------

Co-authored-by: gary.gq <[email protected]>
This commit is contained in:
ChiGaoandgary.gq authored and GitHub committed 2026-09-15 06:13:16 +00:00
1 parent 1fb7595c36
commit e4a6ccd449
2 files changed
+147 -93

No files matched your search

@@ -1853,6 +1853,22 @@ describe('Session', () => {
});
});
const makeWorkflowApproval = (approvalId: string): core.WorkflowApproval => ({
approvalId,
subagentId: `agent-${approvalId}`,
callId: `call-${approvalId}`,
name: 'run_shell_command',
description: 'Run command',
confirmationDetails: {
type: 'exec',
title: 'Run command',
command: 'echo safe',
rootCommand: 'echo',
hideAlwaysAllow: true,
},
at: 1,
});
it('bridges workflow approvals through ACP permission requests', async () => {
mockToolRegistry.getTool.mockReturnValue({
displayName: 'Shell',
@@ -1951,31 +1967,15 @@ describe('Session', () => {
);
const callback = mockWorkflowRunRegistry.setApprovalRequestCallback.mock
.calls[0]?.[0] as core.WorkflowApprovalRequestCallback;
const makeApproval = (approvalId: string): core.WorkflowApproval => ({
approvalId,
subagentId: `agent-${approvalId}`,
callId: `call-${approvalId}`,
name: 'run_shell_command',
description: 'Run command',
confirmationDetails: {
type: 'exec',
title: 'Run command',
command: 'echo safe',
rootCommand: 'echo',
hideAlwaysAllow: true,
},
at: 1,
});
const first = callback(
{ runId: 'wf_concurrent' } as core.WorkflowTask,
makeApproval('wfap_first'),
makeWorkflowApproval('wfap_first'),
{ command: 'echo first' },
new AbortController().signal,
);
const second = callback(
{ runId: 'wf_concurrent' } as core.WorkflowTask,
makeApproval('wfap_second'),
makeWorkflowApproval('wfap_second'),
{ command: 'echo second' },
new AbortController().signal,
);
@@ -2014,23 +2014,26 @@ describe('Session', () => {
);
});
it('serializes permissions across sessions sharing one ACP connection', async () => {
let resolveFirst:
it('does not serialize permissions across sessions sharing one ACP connection', async () => {
let settleFirst:
| ((response: RequestPermissionResponse) => void)
| undefined;
let settleSecond:
| ((response: RequestPermissionResponse) => void)
| undefined;
vi.mocked(mockClient.requestPermission)
.mockImplementationOnce(
() =>
new Promise<RequestPermissionResponse>((resolve) => {
resolveFirst = resolve;
settleFirst = resolve;
}),
)
.mockResolvedValueOnce({
outcome: {
outcome: 'selected',
optionId: core.ToolConfirmationOutcome.ProceedOnce,
},
});
.mockImplementationOnce(
() =>
new Promise<RequestPermissionResponse>((resolve) => {
settleSecond = resolve;
}),
);
const secondWorkflowRunRegistry = {
...mockWorkflowRunRegistry,
setApprovalRequestCallback: vi.fn(),
@@ -2052,55 +2055,65 @@ describe('Session', () => {
.mock.calls[0]?.[0] as core.WorkflowApprovalRequestCallback;
const secondCallback = secondWorkflowRunRegistry.setApprovalRequestCallback
.mock.calls[0]?.[0] as core.WorkflowApprovalRequestCallback;
const makeApproval = (): core.WorkflowApproval => ({
approvalId: 'wfap_1',
subagentId: 'agent-1',
callId: 'call-1',
name: 'run_shell_command',
description: 'Run command',
confirmationDetails: {
type: 'exec',
title: 'Run command',
command: 'echo safe',
rootCommand: 'echo',
hideAlwaysAllow: true,
},
at: 1,
});
const first = firstCallback(
{ runId: 'wf_first' } as core.WorkflowTask,
makeApproval(),
makeWorkflowApproval('wfap_first'),
{ command: 'echo first' },
new AbortController().signal,
);
const second = secondCallback(
{ runId: 'wf_second' } as core.WorkflowTask,
makeApproval(),
makeWorkflowApproval('wfap_second'),
{ command: 'echo second' },
new AbortController().signal,
);
// Neither request is answered and neither signal aborts, which is the
// shape of an idle session nobody is watching: a sibling session sharing
// the connection must still reach the client.
await vi.waitFor(() => {
expect(mockClient.requestPermission).toHaveBeenCalledOnce();
expect(mockClient.requestPermission).toHaveBeenCalledTimes(2);
});
resolveFirst?.({
outcome: {
outcome: 'selected',
optionId: core.ToolConfirmationOutcome.ProceedOnce,
},
});
await Promise.all([first, second]);
expect(mockClient.requestPermission).toHaveBeenCalledTimes(2);
expect(
vi
.mocked(mockClient.requestPermission)
.mock.calls.map(([request]) => request.toolCall.toolCallId),
).toEqual([
'workflow:test-session-id:wf_first:wfap_1',
'workflow:second-session-id:wf_second:wfap_1',
'workflow:test-session-id:wf_first:wfap_first',
'workflow:second-session-id:wf_second:wfap_second',
]);
settleFirst?.({ outcome: { outcome: 'cancelled' } });
settleSecond?.({ outcome: { outcome: 'cancelled' } });
await Promise.allSettled([first, second]);
// The send half above only shows where the request went. Each session must
// also settle its own approval against its own registry.
expect(mockWorkflowRunRegistry.resolvePendingApproval).toHaveBeenCalledWith(
'wf_first',
'wfap_first',
core.ToolConfirmationOutcome.Cancel,
undefined,
);
expect(
secondWorkflowRunRegistry.resolvePendingApproval,
).toHaveBeenCalledWith(
'wf_second',
'wfap_second',
core.ToolConfirmationOutcome.Cancel,
undefined,
);
expect(
mockWorkflowRunRegistry.resolvePendingApproval.mock.calls.map(
([runId]) => runId,
),
).toEqual(['wf_first']);
expect(
secondWorkflowRunRegistry.resolvePendingApproval.mock.calls.map(
([runId]) => runId,
),
).toEqual(['wf_second']);
secondSession.dispose();
});
@@ -2124,25 +2137,9 @@ describe('Session', () => {
const callback = mockWorkflowRunRegistry.setApprovalRequestCallback.mock
.calls[0]?.[0] as core.WorkflowApprovalRequestCallback;
const firstAbort = new AbortController();
const makeApproval = (approvalId: string): core.WorkflowApproval => ({
approvalId,
subagentId: `agent-${approvalId}`,
callId: `call-${approvalId}`,
name: 'run_shell_command',
description: 'Run command',
confirmationDetails: {
type: 'exec',
title: 'Run command',
command: 'echo safe',
rootCommand: 'echo',
hideAlwaysAllow: true,
},
at: 1,
});
const first = callback(
{ runId: 'wf_abort_queue' } as core.WorkflowTask,
makeApproval('wfap_aborted'),
makeWorkflowApproval('wfap_aborted'),
{ command: 'echo first' },
firstAbort.signal,
);
@@ -2151,7 +2148,7 @@ describe('Session', () => {
});
const second = callback(
{ runId: 'wf_abort_queue' } as core.WorkflowTask,
makeApproval('wfap_after_abort'),
makeWorkflowApproval('wfap_after_abort'),
{ command: 'echo second' },
new AbortController().signal,
);
@@ -2168,6 +2165,71 @@ describe('Session', () => {
settleFirstTransport?.({ outcome: { outcome: 'cancelled' } });
});
it('drops a queued permission request that was abandoned before the head settled', async () => {
let settleFirst:
| ((response: RequestPermissionResponse) => void)
| undefined;
vi.mocked(mockClient.requestPermission)
.mockImplementationOnce(
() =>
new Promise<RequestPermissionResponse>((resolve) => {
settleFirst = resolve;
}),
)
.mockResolvedValueOnce({
outcome: {
outcome: 'selected',
optionId: core.ToolConfirmationOutcome.ProceedOnce,
},
});
const callback = mockWorkflowRunRegistry.setApprovalRequestCallback.mock
.calls[0]?.[0] as core.WorkflowApprovalRequestCallback;
const first = callback(
{ runId: 'wf_queued_abort' } as core.WorkflowTask,
makeWorkflowApproval('wfap_head'),
{ command: 'echo first' },
new AbortController().signal,
);
await vi.waitFor(() => {
expect(mockClient.requestPermission).toHaveBeenCalledOnce();
});
const queuedAbort = new AbortController();
const queued = callback(
{ runId: 'wf_queued_abort' } as core.WorkflowTask,
makeWorkflowApproval('wfap_queued'),
{ command: 'echo second' },
queuedAbort.signal,
);
expect(mockClient.requestPermission).toHaveBeenCalledOnce();
queuedAbort.abort();
await queued;
settleFirst?.({ outcome: { outcome: 'cancelled' } });
await first;
// The head releasing must not resurrect the abandoned request. Published
// after its caller is gone, it would sit on the host as a prompt nobody
// can answer and no longer cancels.
expect(mockClient.requestPermission).toHaveBeenCalledOnce();
expect(
vi
.mocked(mockWorkflowRunRegistry.resolvePendingApproval)
.mock.calls.map(([, approvalId]) => approvalId),
).toEqual(['wfap_queued', 'wfap_head']);
// Dropping it must not wedge the session queue either.
await callback(
{ runId: 'wf_queued_abort' } as core.WorkflowTask,
makeWorkflowApproval('wfap_after'),
{ command: 'echo third' },
new AbortController().signal,
);
expect(mockClient.requestPermission).toHaveBeenCalledTimes(2);
expect(
vi.mocked(mockClient.requestPermission).mock.calls[1]?.[0].toolCall
.toolCallId,
).toBe('workflow:test-session-id:wf_queued_abort:wfap_after');
});
it('fails a workflow approval closed when the ACP request times out', async () => {
vi.mocked(mockClient.requestPermission).mockRejectedValue(
new Error('Permission request timed out'),
@@ -441,10 +441,6 @@ import {
} from './repeated-tool-failure-guard.js';
const debugLogger = createDebugLogger('SESSION');
const permissionRequestTails = new WeakMap<
AgentSideConnection,
Promise<void>
>();
const MAX_RETAINED_SESSION_ROUTE_COUNTS = 8;
const USER_CANCEL_ABORT_REASON = 'qwen:user-cancel';
const NEW_PROMPT_ABORT_REASON = 'qwen:new-prompt';
@@ -2258,6 +2254,14 @@ export class Session implements SessionContext {
readonly sessionId: string;
private sessionReasoningSelection?: ReasoningSelection;
private readonly restoredHistoryGaps?: HistoryGap[];
/**
* Tail of this session's permission queue. The serialization unit is the
* session, not the `AgentSideConnection`: one daemon child owns a single
* connection that every session on it multiplexes onto, so a connection-wide
* queue makes unrelated sessions inherit the wait behind a question nobody
* is going to answer. Mirrors `promptQueue`, which is per session bridge-side.
*/
private permissionRequestTail: Promise<void> = Promise.resolve();
constructor(
id: string,
@@ -11271,18 +11275,6 @@ export class Session implements SessionContext {
await this.sendUpdate(update);
}
/**
* Requests permission from the client for a tool call.
* Used by SubAgentTracker for sub-agent approval requests.
*/
async requestPermission(
params: RequestPermissionRequest,
): Promise<RequestPermissionResponse> {
return this.client.requestPermission(
this.#withBackgroundPermission(params),
);
}
#withBackgroundPermission(
params: RequestPermissionRequest,
): RequestPermissionRequest {
@@ -11300,7 +11292,7 @@ export class Session implements SessionContext {
signal: AbortSignal,
): Promise<RequestPermissionResponse> {
params = this.#withBackgroundPermission(params);
const prior = permissionRequestTails.get(this.client) ?? Promise.resolve();
const prior = this.permissionRequestTail;
const transportRequest = prior.then(() =>
signal.aborted
? requestPermissionWithAbort(this.client, params, signal)
@@ -11322,7 +11314,7 @@ export class Session implements SessionContext {
]).finally(() => {
if (abortListener) signal.removeEventListener('abort', abortListener);
});
permissionRequestTails.set(this.client, tail);
this.permissionRequestTail = tail;
return requestPermissionWithAbort(
{ requestPermission: () => transportRequest },
params,