diff --git a/packages/channels/base/src/AcpBridge.test.ts b/packages/channels/base/src/AcpBridge.test.ts index 280f51f22f3..a1953d2439e 100644 --- a/packages/channels/base/src/AcpBridge.test.ts +++ b/packages/channels/base/src/AcpBridge.test.ts @@ -1,5 +1,10 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { ACP_EVENT_LOOP_STALL_RESTART_MS, AcpBridge } from './AcpBridge.js'; +import type { RequestPermissionResponse } from '@agentclientprotocol/sdk'; +import { + ACP_EVENT_LOOP_STALL_RESTART_MS, + ACP_PERMISSION_RESPONSE_TIMEOUT_MS, + AcpBridge, +} from './AcpBridge.js'; import { CHANNEL_LOOP_MCP_SERVER_NAME } from './ChannelLoopTools.js'; import type { ChannelLoopToolHandler } from './ChannelAgentBridge.js'; @@ -44,6 +49,13 @@ const child = vi.hoisted(() => { return { instances: [] as MockChild[], + clients: [] as Array<{ + requestPermission: (params: unknown) => Promise; + }>, + connections: [] as Array<{ + initialize: ReturnType; + cancel: ReturnType; + }>, MockChild, spawn: vi.fn(() => { const instance = new MockChild(); @@ -65,9 +77,16 @@ vi.mock('node:stream', () => ({ vi.mock('@agentclientprotocol/sdk', () => ({ PROTOCOL_VERSION: 1, ndJsonStream: vi.fn(() => ({})), - ClientSideConnection: vi.fn().mockImplementation(() => ({ - initialize: vi.fn().mockResolvedValue(undefined), - })), + ClientSideConnection: vi.fn().mockImplementation((createClient) => { + const client = createClient(); + const connection = { + initialize: vi.fn().mockResolvedValue(undefined), + cancel: vi.fn().mockResolvedValue(undefined), + }; + child.clients.push(client); + child.connections.push(connection); + return connection; + }), })); type TestableAcpBridge = AcpBridge & { @@ -91,6 +110,8 @@ type TestableAcpBridge = AcpBridge & { describe('AcpBridge', () => { beforeEach(() => { child.instances.length = 0; + child.clients.length = 0; + child.connections.length = 0; child.spawn.mockClear(); }); @@ -350,4 +371,287 @@ describe('AcpBridge', () => { expect(proc.kill).not.toHaveBeenCalled(); }); + + it('relays ACP permission requests instead of auto-approving them', async () => { + const bridge = new AcpBridge({ + cliEntryPath: '/tmp/qwen', + cwd: '/tmp', + }); + const permissionRequest = vi.fn(); + const permissionResolved = vi.fn(); + bridge.on('permissionRequest', permissionRequest); + bridge.on('permissionResolved', permissionResolved); + + await bridge.start(); + const request = { + sessionId: 'session-1', + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [ + { optionId: 'proceed_once', name: 'Allow' }, + { optionId: 'cancel', name: 'Deny' }, + ], + }; + + const pending = child.clients[0]!.requestPermission(request); + await Promise.resolve(); + + expect(permissionRequest).toHaveBeenCalledTimes(1); + const event = permissionRequest.mock.calls[0]![0]; + expect(event).toMatchObject({ + sessionId: 'session-1', + request, + }); + expect(event.requestId).toMatch(/^acp-permission-/); + + const response = { outcome: { outcome: 'selected', optionId: 'cancel' } }; + await expect( + ( + bridge as unknown as TestableAcpBridge & { + respondToPermission( + requestId: string, + response: typeof response, + ): Promise; + } + ).respondToPermission(event.requestId, response), + ).resolves.toBe(true); + await expect(pending).resolves.toEqual(response); + expect(permissionResolved).toHaveBeenCalledWith({ + requestId: event.requestId, + outcome: response.outcome, + }); + await expect( + ( + bridge as unknown as TestableAcpBridge & { + respondToPermission( + requestId: string, + response: typeof response, + ): Promise; + } + ).respondToPermission(event.requestId, response), + ).resolves.toBe(false); + }); + + it('allows permission request listeners to respond synchronously', async () => { + const bridge = new AcpBridge({ + cliEntryPath: '/tmp/qwen', + cwd: '/tmp', + }); + const response: RequestPermissionResponse = { + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }; + bridge.on('permissionRequest', (event) => { + void bridge.respondToPermission(event.requestId, response); + }); + + await bridge.start(); + const pending = child.clients[0]!.requestPermission({ + sessionId: 'session-1', + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [{ optionId: 'proceed_once', name: 'Allow' }], + }); + + await expect(pending).resolves.toEqual(response); + }); + + it('falls back to the tool call id for permission requests without a session id', async () => { + const bridge = new AcpBridge({ + cliEntryPath: '/tmp/qwen', + cwd: '/tmp', + }); + const permissionRequest = vi.fn(); + bridge.on('permissionRequest', permissionRequest); + + await bridge.start(); + const pending = child.clients[0]!.requestPermission({ + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [{ optionId: 'cancel', name: 'Deny' }], + }); + await Promise.resolve(); + + const event = permissionRequest.mock.calls[0]![0]; + expect(event.sessionId).toBe('tool-1'); + await bridge.respondToPermission(event.requestId, { + outcome: { outcome: 'cancelled' }, + }); + await expect(pending).resolves.toEqual({ + outcome: { outcome: 'cancelled' }, + }); + }); + + it('resolves matching pending permissions as cancelled when a session is cancelled', async () => { + const bridge = new AcpBridge({ + cliEntryPath: '/tmp/qwen', + cwd: '/tmp', + }); + const permissionRequest = vi.fn(); + const permissionResolved = vi.fn(); + bridge.on('permissionRequest', permissionRequest); + bridge.on('permissionResolved', permissionResolved); + + await bridge.start(); + const first = child.clients[0]!.requestPermission({ + sessionId: 'session-1', + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [{ optionId: 'cancel', name: 'Deny' }], + }); + const second = child.clients[0]!.requestPermission({ + sessionId: 'session-2', + toolCall: { + toolCallId: 'tool-2', + kind: 'shell', + title: 'Run command', + }, + options: [{ optionId: 'cancel', name: 'Deny' }], + }); + await Promise.resolve(); + + const firstEvent = permissionRequest.mock.calls[0]![0]; + const secondEvent = permissionRequest.mock.calls[1]![0]; + await bridge.cancelSession('session-1'); + + expect(child.connections[0]!.cancel).toHaveBeenCalledWith({ + sessionId: 'session-1', + }); + await expect(first).resolves.toEqual({ + outcome: { outcome: 'cancelled' }, + }); + expect(permissionResolved).toHaveBeenCalledWith({ + requestId: firstEvent.requestId, + outcome: { outcome: 'cancelled' }, + }); + expect(permissionResolved).not.toHaveBeenCalledWith({ + requestId: secondEvent.requestId, + outcome: { outcome: 'cancelled' }, + }); + + const response: RequestPermissionResponse = { + outcome: { outcome: 'selected', optionId: 'cancel' }, + }; + await expect( + bridge.respondToPermission(secondEvent.requestId, response), + ).resolves.toBe(true); + await expect(second).resolves.toEqual(response); + }); + + it('resolves pending permissions as cancelled after the response timeout', async () => { + const bridge = new AcpBridge({ + cliEntryPath: '/tmp/qwen', + cwd: '/tmp', + }); + const permissionRequest = vi.fn(); + const permissionResolved = vi.fn(); + const stderr = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + bridge.on('permissionRequest', permissionRequest); + bridge.on('permissionResolved', permissionResolved); + + await bridge.start(); + + vi.useFakeTimers(); + try { + const pending = child.clients[0]!.requestPermission({ + sessionId: 'session-1', + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [{ optionId: 'cancel', name: 'Deny' }], + }); + await Promise.resolve(); + const event = permissionRequest.mock.calls[0]![0]; + + await vi.advanceTimersByTimeAsync(ACP_PERMISSION_RESPONSE_TIMEOUT_MS); + + await expect(pending).resolves.toEqual({ + outcome: { outcome: 'cancelled' }, + }); + expect(permissionResolved).toHaveBeenCalledWith({ + requestId: event.requestId, + outcome: { outcome: 'cancelled' }, + }); + expect(stderr.mock.calls.join('')).toContain( + `[AcpBridge] permission request ${event.requestId} timed out after ${ACP_PERMISSION_RESPONSE_TIMEOUT_MS}ms (session=session-1)`, + ); + } finally { + stderr.mockRestore(); + vi.useRealTimers(); + } + }); + + it('resolves pending permissions as cancelled when the ACP child exits', async () => { + const bridge = new AcpBridge({ + cliEntryPath: '/tmp/qwen', + cwd: '/tmp', + }); + const permissionRequest = vi.fn(); + const permissionResolved = vi.fn(); + bridge.on('permissionRequest', permissionRequest); + bridge.on('permissionResolved', permissionResolved); + + await bridge.start(); + const pending = child.clients[0]!.requestPermission({ + sessionId: 'session-1', + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [{ optionId: 'cancel', name: 'Deny' }], + }); + await Promise.resolve(); + const event = permissionRequest.mock.calls[0]![0]; + + child.instances[0]!.emit('exit', 1, null); + + await expect(pending).resolves.toEqual({ + outcome: { outcome: 'cancelled' }, + }); + expect(permissionResolved).toHaveBeenCalledWith({ + requestId: event.requestId, + outcome: { outcome: 'cancelled' }, + }); + }); + + it('resolves pending permissions as cancelled on stop', async () => { + const bridge = new AcpBridge({ + cliEntryPath: '/tmp/qwen', + cwd: '/tmp', + }); + + await bridge.start(); + const pending = child.clients[0]!.requestPermission({ + sessionId: 'session-1', + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [{ optionId: 'cancel', name: 'Deny' }], + }); + await Promise.resolve(); + + bridge.stop(); + + await expect(pending).resolves.toEqual({ + outcome: { outcome: 'cancelled' }, + }); + }); }); diff --git a/packages/channels/base/src/AcpBridge.ts b/packages/channels/base/src/AcpBridge.ts index 1271c4df1a7..dfe108fa35d 100644 --- a/packages/channels/base/src/AcpBridge.ts +++ b/packages/channels/base/src/AcpBridge.ts @@ -1,5 +1,6 @@ import { spawn } from 'node:child_process'; import type { ChildProcess } from 'node:child_process'; +import { randomUUID } from 'node:crypto'; import { Readable, Writable } from 'node:stream'; import { EventEmitter } from 'node:events'; import { @@ -39,6 +40,7 @@ export interface AcpBridgeOptions { } export const ACP_EVENT_LOOP_STALL_RESTART_MS = 5 * 60 * 1000; +export const ACP_PERMISSION_RESPONSE_TIMEOUT_MS = 5 * 60 * 1000; const ACP_EVENT_LOOP_STALL_RE = /^\[perf\] acp agent event loop stall: max=(\d+(?:\.\d+)?)ms/m; @@ -77,6 +79,14 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { private readonly channelLoopToolHandlers: ChannelLoopToolHandler[] = []; private channelLoopMcpRegistered = false; private channelLoopMcpRegistration: Promise | null = null; + private readonly pendingPermissions = new Map< + string, + { + sessionId: string; + resolve: (response: RequestPermissionResponse) => void; + timeout: ReturnType; + } + >(); constructor(options: AcpBridgeOptions) { super(); @@ -109,7 +119,7 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { this.child.stderr?.on('data', (data: Buffer) => { const msg = data.toString().trim(); if (msg) { - process.stderr.write(`[AcpBridge] ${msg}\n`); + process.stderr.write(`[AcpBridge] ${sanitizeLogText(msg, 4096)}\n`); this.maybeKillOnEventLoopStall(msg); } }); @@ -120,6 +130,7 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { ); // Do not emit sessionDied here: a full ACP process exit is handled by // channel start crash recovery, which reloads the persisted sessions. + this.resolvePendingPermissions(); this.connection = null; this.child = null; this.emit('disconnected', code, signal); @@ -147,15 +158,7 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { requestPermission: async ( params: RequestPermissionRequest, - ): Promise => { - // Auto-approve for now; Phase 5 will add interactive approval - const options = Array.isArray(params.options) ? params.options : []; - const optionId = - options.find((o) => o.optionId === 'proceed_once')?.optionId || - options[0]?.optionId || - 'proceed_once'; - return { outcome: { outcome: 'selected', optionId } }; - }, + ): Promise => this.requestPermission(params), extMethod: async ( method: string, @@ -245,10 +248,33 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { async cancelSession(sessionId: string): Promise { const conn = this.ensureConnection(); - await conn.cancel({ sessionId }); + try { + await conn.cancel({ sessionId }); + } finally { + this.resolvePendingPermissions(sessionId); + } + } + + async respondToPermission( + requestId: string, + response: RequestPermissionResponse, + ): Promise { + const pending = this.pendingPermissions.get(requestId); + if (!pending) { + return false; + } + clearTimeout(pending.timeout); + this.pendingPermissions.delete(requestId); + pending.resolve(response); + this.emit('permissionResolved', { + requestId, + outcome: response.outcome, + }); + return true; } stop(): void { + this.resolvePendingPermissions(); if (this.child) { this.child.kill(); this.child = null; @@ -319,6 +345,62 @@ export class AcpBridge extends EventEmitter implements ChannelAgentBridge { return this.connection; } + private requestPermission( + request: RequestPermissionRequest, + ): Promise { + const requestId = `acp-permission-${randomUUID()}`; + const sessionId = + typeof request.sessionId === 'string' && request.sessionId.length > 0 + ? request.sessionId + : request.toolCall.toolCallId; + + return new Promise((resolve) => { + const timeout = setTimeout(() => { + const pending = this.pendingPermissions.get(requestId); + if (!pending) { + return; + } + process.stderr.write( + `[AcpBridge] permission request ${sanitizeLogText(requestId, 128)} timed out after ${ACP_PERMISSION_RESPONSE_TIMEOUT_MS}ms (session=${sanitizeLogText(pending.sessionId, 128)})\n`, + ); + this.pendingPermissions.delete(requestId); + const response: RequestPermissionResponse = { + outcome: { outcome: 'cancelled' }, + }; + pending.resolve(response); + this.emit('permissionResolved', { + requestId, + outcome: response.outcome, + }); + }, ACP_PERMISSION_RESPONSE_TIMEOUT_MS); + timeout.unref?.(); + this.pendingPermissions.set(requestId, { sessionId, resolve, timeout }); + this.emit('permissionRequest', { + requestId, + sessionId, + request, + }); + }); + } + + private resolvePendingPermissions(sessionId?: string): void { + const response: RequestPermissionResponse = { + outcome: { outcome: 'cancelled' }, + }; + for (const [requestId, pending] of this.pendingPermissions) { + if (sessionId !== undefined && pending.sessionId !== sessionId) { + continue; + } + clearTimeout(pending.timeout); + this.pendingPermissions.delete(requestId); + pending.resolve(response); + this.emit('permissionResolved', { + requestId, + outcome: response.outcome, + }); + } + } + private maybeKillOnEventLoopStall(stderr: string): void { const match = ACP_EVENT_LOOP_STALL_RE.exec(stderr); if (!match) return; diff --git a/packages/channels/base/src/ChannelAgentBridge.ts b/packages/channels/base/src/ChannelAgentBridge.ts index a4da0249d2d..1adb7a90ee4 100644 --- a/packages/channels/base/src/ChannelAgentBridge.ts +++ b/packages/channels/base/src/ChannelAgentBridge.ts @@ -1,3 +1,8 @@ +import type { + RequestPermissionRequest, + RequestPermissionResponse, +} from '@agentclientprotocol/sdk'; + export interface AvailableCommand { name: string; description: string; @@ -47,10 +52,23 @@ export interface SessionDiedEvent { reason?: string; } +export interface PermissionRequestEvent { + requestId: string; + sessionId: string; + request: RequestPermissionRequest; +} + +export interface PermissionResolvedEvent { + requestId: string; + outcome?: RequestPermissionResponse['outcome']; +} + interface ChannelAgentBridgeEventMap { sessionDied: [SessionDiedEvent]; textChunk: [sessionId: string, chunk: string]; toolCall: [ToolCallEvent]; + permissionRequest: [PermissionRequestEvent]; + permissionResolved: [PermissionResolvedEvent]; } export interface BridgeSessionInfo { @@ -78,6 +96,10 @@ export interface ChannelAgentBridge { options?: { imageBase64?: string; imageMimeType?: string }, ): Promise; cancelSession(sessionId: string): Promise; + respondToPermission?( + requestId: string, + response: RequestPermissionResponse, + ): Promise; shellCommand?( sessionId: string, command: string, diff --git a/packages/channels/base/src/ChannelBase.test.ts b/packages/channels/base/src/ChannelBase.test.ts index 3802327d0fb..bfb67090247 100644 --- a/packages/channels/base/src/ChannelBase.test.ts +++ b/packages/channels/base/src/ChannelBase.test.ts @@ -21,8 +21,10 @@ import type { ChannelLoop, ChannelLoopInput } from './ChannelLoopStore.js'; class TestChannel extends ChannelBase { sent: Array<{ chatId: string; text: string }> = []; proactive: Array<{ chatId: string; text: string }> = []; + proactiveTargets: SessionTarget[] = []; proactiveSupported = false; proactiveTargetSupported: boolean | undefined; + sendMessageError?: Error; connected = false; toolCalls: Array<{ chatId: string; event: unknown }> = []; taskEvents: ChannelTaskLifecycleEvent[] = []; @@ -54,6 +56,9 @@ class TestChannel extends ChannelBase { this.connected = true; } async sendMessage(chatId: string, text: string) { + if (this.sendMessageError) { + throw this.sendMessageError; + } this.sent.push({ chatId, text }); } disconnect() { @@ -79,10 +84,11 @@ class TestChannel extends ChannelBase { } protected override async pushProactive( - target: { chatId: string }, + target: SessionTarget, text: string, ): Promise { this.proactive.push({ chatId: target.chatId, text }); + this.proactiveTargets.push(target); } enableCancelCommand(): void { @@ -176,6 +182,7 @@ function createBridge(): ChannelAgentBridge { isConnected: true, availableCommands: [], setBridge: vi.fn(), + respondToPermission: vi.fn().mockResolvedValue(true), registerChannelLoopToolHandler: vi.fn((handler: ChannelLoopToolHandler) => { channelLoopToolHandler = handler; }), @@ -296,6 +303,666 @@ describe('ChannelBase', () => { }); }); + describe('permission relay', () => { + function respondToPermissionMock(): ReturnType { + return ( + bridge as unknown as { + respondToPermission: ReturnType; + } + ).respondToPermission; + } + + function emitPermission( + sessionId: string, + requestId: string, + options = [ + { + optionId: 'proceed_always_project', + kind: 'allow_always', + name: 'Always Allow in project', + }, + { optionId: 'proceed_once', kind: 'allow_once', name: 'Allow once' }, + { optionId: 'cancel', kind: 'reject_once', name: 'Deny' }, + ], + ): void { + (bridge as unknown as EventEmitter).emit('permissionRequest', { + requestId, + sessionId, + request: { + toolCall: { + toolCallId: `tool-${requestId}`, + kind: 'shell', + title: `Run ${requestId}`, + rawInput: { command: 'echo secret-token' }, + }, + options, + }, + }); + } + + async function startSession( + ch: TestChannel, + env: Partial = {}, + ): Promise { + await ch.handleInbound(envelope({ text: 'run tests', ...env })); + const results = (bridge.newSession as ReturnType).mock + .results; + return results[results.length - 1]!.value as string; + } + + it('sends permission requests to the owning chat and approves with /approve', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1'); + + expect(ch.sent.at(-1)?.chatId).toBe('chat1'); + expect(ch.sent.at(-1)?.text).toContain( + 'Permission required to run a tool', + ); + expect(ch.sent.at(-1)?.text).toContain('Command:'); + expect(ch.sent.at(-1)?.text).toContain('Run req-1'); + expect(ch.sent.at(-1)?.text).toContain('/approve allow once'); + expect(ch.sent.at(-1)?.text).toContain('/approve-always always allow'); + expect(ch.sent.at(-1)?.text).toContain('/deny deny'); + expect(ch.sent.at(-1)?.text).not.toContain('Request: req-1'); + expect(ch.sent.at(-1)?.text).not.toContain('proceed_once'); + expect(ch.sent.at(-1)?.text).not.toContain('secret-token'); + + await ch.handleInbound(envelope({ text: '/approve' })); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }); + expect(ch.sent.at(-1)?.text).toBe('Permission approved.'); + }); + + it('cancels bridge permissions when the target no longer belongs to the channel', async () => { + const router = { + getTarget: vi.fn(() => ({ + channelName: 'other-channel', + chatId: 'chat1', + })), + setBridge: vi.fn(), + }; + const ch = createChannel({}, { router } as unknown as ChannelBaseOptions); + + await ch.dispatchPermissionRequest({ + requestId: 'req-stale', + sessionId: 'session-1', + request: { + toolCall: { + toolCallId: 'tool-req-stale', + kind: 'shell', + title: 'Run req-stale', + }, + options: [], + }, + }); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-stale', { + outcome: { outcome: 'cancelled' }, + }); + expect(ch.sent).toEqual([]); + }); + + it('requires an explicit request id when multiple permissions are pending', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1'); + emitPermission(sessionId, 'req-2'); + + await ch.handleInbound(envelope({ text: '/approve' })); + + expect(ch.sent.at(-1)?.text).toContain( + 'Multiple permission requests are pending', + ); + expect(ch.sent.at(-1)?.text).toContain('req-1'); + expect(ch.sent.at(-1)?.text).toContain('req-2'); + expect(ch.sent.at(-1)?.text).toContain('req-1: Run req-1'); + expect(ch.sent.at(-1)?.text).toContain('req-2: Run req-2'); + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(respondToPermissionMock()).toHaveBeenCalledTimes(1); + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }); + }); + + it('does not fall back to another pending request when an explicit id is wrong', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1'); + emitPermission(sessionId, 'req-2'); + + await ch.handleInbound(envelope({ text: '/approve missing-request' })); + + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + }); + + it('does not answer permission requests from another chat', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch, { chatId: 'chat2' }); + emitPermission(sessionId, 'req-chat2'); + + await ch.handleInbound( + envelope({ chatId: 'chat1', text: '/approve req-chat2' }), + ); + + expect(ch.sent.at(-1)?.chatId).toBe('chat1'); + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + }); + + it('does not answer permission requests from another thread', async () => { + const ch = createChannel({ + groupPolicy: 'open', + sessionScope: 'thread', + }); + const sessionId = await startSession(ch, { + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'alice', + threadId: 'thread-1', + }); + emitPermission(sessionId, 'req-thread-1'); + + await ch.handleInbound( + envelope({ + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'alice', + text: '/approve req-thread-1', + threadId: 'thread-2', + }), + ); + + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + + await ch.handleInbound( + envelope({ + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'alice', + text: '/approve req-thread-1', + threadId: 'thread-1', + }), + ); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-thread-1', { + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }); + }); + + it('delivers threaded permission requests through proactive targets when supported', async () => { + const ch = createChannel({ + groupPolicy: 'open', + sessionScope: 'thread', + }); + ch.proactiveSupported = true; + ch.proactiveTargetSupported = true; + const sessionId = await startSession(ch, { + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'alice', + threadId: 'thread-1', + }); + + emitPermission(sessionId, 'req-thread-1'); + + expect(ch.proactiveTargets.at(-1)).toMatchObject({ + chatId: 'group1', + senderId: 'alice', + threadId: 'thread-1', + }); + expect(ch.proactive.at(-1)?.text).toContain( + 'Permission required to run a tool', + ); + expect(ch.sent.at(-1)?.text).not.toContain( + 'Permission required to run a tool', + ); + }); + + it('routes single-scope permission requests to the active chat', async () => { + const ch = createChannel({ sessionScope: 'single' }); + await startSession(ch, { senderId: 'alice', chatId: 'alice-dm' }); + + let resolveBob!: (value: string) => void; + const bobPrompt = new Promise((resolve) => { + resolveBob = resolve; + }); + (bridge.prompt as ReturnType).mockImplementationOnce( + () => bobPrompt, + ); + + const bobTurn = ch.handleInbound( + envelope({ + senderId: 'bob', + senderName: 'Bob', + chatId: 'bob-dm', + text: 'needs permission', + }), + ); + await vi.waitFor(() => expect(bridge.prompt).toHaveBeenCalledTimes(2)); + const sessionId = (bridge.prompt as ReturnType).mock + .calls[1]![0] as string; + + emitPermission(sessionId, 'req-bob'); + + await vi.waitFor(() => + expect(ch.sent.at(-1)?.text).toContain( + 'Permission required to run a tool', + ), + ); + expect(ch.sent.at(-1)?.chatId).toBe('bob-dm'); + + resolveBob('agent response'); + await bobTurn; + }); + + it('does not let another group member approve user-scoped permissions', async () => { + const ch = createChannel({ + groupPolicy: 'open', + sessionScope: 'user', + }); + const sessionId = await startSession(ch, { + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'alice', + threadId: 'thread-1', + }); + emitPermission(sessionId, 'req-alice'); + + await ch.handleInbound( + envelope({ + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'bob', + text: '/approve req-alice', + threadId: 'thread-1', + }), + ); + + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + + await ch.handleInbound( + envelope({ + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'alice', + text: '/approve req-alice', + threadId: 'thread-1', + }), + ); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-alice', { + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }); + }); + + it('matches the current sender before reporting ambiguous permissions', async () => { + const ch = createChannel({ + groupPolicy: 'open', + sessionScope: 'user', + }); + const group = { + chatId: 'group1', + isGroup: true, + isMentioned: true, + threadId: 'thread-1', + }; + const aliceSessionId = await startSession(ch, { + ...group, + senderId: 'alice', + }); + emitPermission(aliceSessionId, 'req-alice'); + const bobSessionId = await startSession(ch, { + ...group, + senderId: 'bob', + }); + emitPermission(bobSessionId, 'req-bob'); + + await ch.handleInbound( + envelope({ + ...group, + senderId: 'alice', + text: '/approve', + }), + ); + + expect(ch.sent.at(-1)?.text).toBe('Permission approved.'); + expect(respondToPermissionMock()).toHaveBeenCalledTimes(1); + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-alice', { + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }); + }); + + it('gates shared-session permission responses to authorized senders', async () => { + const ch = createChannel({ + allowedUsers: ['boss'], + groupPolicy: 'open', + sessionScope: 'thread', + }); + const sessionId = await startSession(ch, { + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'boss', + threadId: 'thread-1', + }); + emitPermission(sessionId, 'req-1'); + + await ch.handleInbound( + envelope({ + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'rando', + text: '/approve req-1', + threadId: 'thread-1', + }), + ); + + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + expect(ch.sent.at(-1)?.text).toContain('Only authorized members'); + + await ch.handleInbound( + envelope({ + chatId: 'group1', + isGroup: true, + isMentioned: true, + senderId: 'boss', + text: '/approve req-1', + threadId: 'thread-1', + }), + ); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }); + }); + + it('uses ACP option kinds for approval and denial', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1', [ + { optionId: 'always', kind: 'allow_always', name: 'Allow always' }, + { optionId: 'once', kind: 'allow_once', name: 'Allow once' }, + { optionId: 'never', kind: 'reject_always', name: 'Deny always' }, + { optionId: 'reject', kind: 'reject_once', name: 'Deny once' }, + ]); + + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'selected', optionId: 'once' }, + }); + + emitPermission(sessionId, 'req-2', [ + { optionId: 'reject', kind: 'reject_once', name: 'Deny once' }, + ]); + + await ch.handleInbound(envelope({ text: '/deny req-2' })); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-2', { + outcome: { outcome: 'selected', optionId: 'reject' }, + }); + expect(ch.sent.at(-1)?.text).toBe('Permission denied.'); + + emitPermission(sessionId, 'req-3', [ + { optionId: 'always', kind: 'allow_always', name: 'Allow always' }, + { optionId: 'never', kind: 'reject_always', name: 'Deny always' }, + ]); + + await ch.handleInbound(envelope({ text: '/deny req-3' })); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-2', { + outcome: { outcome: 'selected', optionId: 'reject' }, + }); + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-3', { + outcome: { outcome: 'cancelled' }, + }); + expect(ch.sent.at(-1)?.text).toBe('Permission denied.'); + }); + + it('reports permission requests that lack requested approval options', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1', [ + { optionId: 'reject', kind: 'reject_once', name: 'Deny once' }, + ]); + + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(ch.sent.at(-1)?.text).toBe( + 'This permission request has no approvable option.', + ); + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + + await ch.handleInbound(envelope({ text: '/approve-always req-1' })); + + expect(ch.sent.at(-1)?.text).toBe( + 'This permission request has no always-allow option.', + ); + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + }); + + it('clears pending permission requests when response dispatch fails', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1'); + respondToPermissionMock().mockRejectedValueOnce(new Error('send failed')); + + await ch.handleInbound(envelope({ text: '/approve req-1' })); + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + }); + + it('supports explicit approve-always for persistent permission grants', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1', [ + { + optionId: 'proceed_always_user', + kind: 'allow_always', + name: 'Always Allow for user', + }, + { + optionId: 'proceed_always_project', + kind: 'allow_always', + name: 'Always Allow in project', + }, + { optionId: 'once', kind: 'allow_once', name: 'Allow once' }, + ]); + + expect(ch.sent.at(-1)?.text).toContain( + '/approve-always always allow for this project', + ); + + await ch.handleInbound(envelope({ text: '/approve-always req-1' })); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'selected', optionId: 'proceed_always_project' }, + }); + expect(ch.sent.at(-1)?.text).toBe('Permission approved always.'); + }); + + it('falls back to user-scope approve-always when project scope is unavailable', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1', [ + { + optionId: 'proceed_always_user', + kind: 'allow_always', + name: 'Always Allow for user', + }, + { optionId: 'once', kind: 'allow_once', name: 'Allow once' }, + ]); + + expect(ch.sent.at(-1)?.text).toContain( + '/approve-always always allow for this user', + ); + + await ch.handleInbound(envelope({ text: '/approve-always req-1' })); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'selected', optionId: 'proceed_always_user' }, + }); + }); + + it('does not infer approve-always scope from noncanonical option ids', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1', [ + { + optionId: 'sandbox_bypass_project', + kind: 'allow_always', + name: 'Always allow sandbox bypass', + }, + { + optionId: 'proceed_always_user', + kind: 'allow_always', + name: 'Always Allow for user', + }, + { optionId: 'once', kind: 'allow_once', name: 'Allow once' }, + ]); + + expect(ch.sent.at(-1)?.text).toContain( + '/approve-always always allow for this user', + ); + + await ch.handleInbound(envelope({ text: '/approve-always req-1' })); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'selected', optionId: 'proceed_always_user' }, + }); + }); + + it('allows approve-always without a request id when one request is pending', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1', [ + { optionId: 'always', kind: 'allow_always', name: 'Allow always' }, + { optionId: 'once', kind: 'allow_once', name: 'Allow once' }, + ]); + + await ch.handleInbound(envelope({ text: '/approve-always' })); + + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'selected', optionId: 'always' }, + }); + }); + + it('clears pending permission requests when the session is cleared', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1'); + + await ch.handleInbound(envelope({ text: '/clear' })); + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + }); + + it('clears pending permission requests when the session dies', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1'); + + (bridge as unknown as EventEmitter).emit('sessionDied', { + sessionId, + }); + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(respondToPermissionMock()).not.toHaveBeenCalled(); + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + }); + + it('reports when the bridge cannot answer permission requests', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1'); + delete (bridge as unknown as { respondToPermission?: unknown }) + .respondToPermission; + + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(ch.sent.at(-1)?.text).toBe( + 'Permission relay is not available for this session.', + ); + }); + + it('cancels the permission request when the relay message cannot be sent', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + ch.sendMessageError = new Error('send failed'); + + emitPermission(sessionId, 'req-1'); + await vi.waitFor(() => + expect(respondToPermissionMock()).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'cancelled' }, + }), + ); + ch.sendMessageError = undefined; + + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + }); + + it('clears pending permission requests when the bridge is replaced', async () => { + const ch = createChannel(); + const sessionId = await startSession(ch); + emitPermission(sessionId, 'req-1'); + const oldRespondToPermission = respondToPermissionMock(); + const newBridge = createBridge(); + + ch.setBridge(newBridge); + await ch.handleInbound(envelope({ text: '/approve req-1' })); + + expect(ch.sent.at(-1)?.text).toBe( + 'No pending permission request with that id for this chat.', + ); + expect(oldRespondToPermission).not.toHaveBeenCalled(); + expect( + ( + newBridge as unknown as { + respondToPermission: ReturnType; + } + ).respondToPermission, + ).not.toHaveBeenCalled(); + }); + }); + describe('group history backfill', () => { it('does not record unmentioned group messages when groupHistoryLimit is absent', async () => { const ch = createChannel({ @@ -835,6 +1502,7 @@ describe('ChannelBase', () => { expect(ch.sent).toHaveLength(1); expect(ch.sent[0]!.text).toContain('/help'); expect(ch.sent[0]!.text).toContain('/clear'); + expect(ch.sent[0]!.text).toContain('/approve-always [request-id]'); expect(ch.sent[0]!.text).not.toContain('/cancel'); expect(bridge.prompt).not.toHaveBeenCalled(); }); diff --git a/packages/channels/base/src/ChannelBase.ts b/packages/channels/base/src/ChannelBase.ts index 0dc98395954..390ac2c1066 100644 --- a/packages/channels/base/src/ChannelBase.ts +++ b/packages/channels/base/src/ChannelBase.ts @@ -35,6 +35,8 @@ import type { ChannelAgentBridge, ChannelLoopToolCreateInput, ChannelLoopToolResult, + PermissionRequestEvent, + PermissionResolvedEvent, SessionDiedEvent, ToolCallEvent, } from './ChannelAgentBridge.js'; @@ -102,6 +104,17 @@ export interface ChannelLoopPromptOptions { /** Handler for a slash command. Return true if handled, false to forward to agent. */ type CommandHandler = (envelope: Envelope, args: string) => Promise; +type PendingPermission = { + requestId: string; + sessionId: string; + target: SessionTarget; + request: PermissionRequestEvent['request']; +}; +type PermissionOption = PermissionRequestEvent['request']['options'][number]; +type PendingPermissionLookup = + | { kind: 'found'; pending: PendingPermission } + | { kind: 'none'; explicit: boolean } + | { kind: 'ambiguous'; requestIds: string[] }; type CollectBufferEntry = { text: string; envelope: Envelope }; type ActivePrompt = { cancelled: boolean; @@ -119,6 +132,8 @@ type ActivePrompt = { /** The originating turn's chat/message, so a clear-time eviction can run this * turn's own onPromptEnd (its finally may settle long after — or never). */ chatId: string; + threadId?: string; + isGroup?: boolean; messageId?: string; senderId?: string; senderName?: string; @@ -215,6 +230,22 @@ export abstract class ChannelBase { ): void => { this.onSessionDied(event.sessionId); }; + private readonly bridgePermissionRequestListener = ( + event: PermissionRequestEvent, + ): void => { + void this.dispatchPermissionRequest(event).catch((err: unknown) => { + process.stderr.write( + `[${this.name}] permission relay failed for request ${sanitizeLogText(event.requestId, 128)}: ${this.lifecycleError(err)}\n`, + ); + }); + }; + private readonly bridgePermissionResolvedListener = ( + event: PermissionResolvedEvent, + ): void => { + this.dispatchPermissionResolved(event); + }; + private readonly pendingPermissions = new Map(); + private readonly pendingPermissionsByChat = new Map(); private readonly channelLoopToolHandler = { canHandle: (sessionId: string) => this.router.getTarget(sessionId)?.channelName === this.name, @@ -251,6 +282,91 @@ export abstract class ChannelBase { this.onToolCall(chatId, event); } + async dispatchPermissionRequest( + event: PermissionRequestEvent, + ): Promise { + const target = this.permissionTargetForEvent(event); + if (!target) { + try { + await this.bridge.respondToPermission?.(event.requestId, { + outcome: { outcome: 'cancelled' }, + }); + } catch (respondErr) { + process.stderr.write( + `[${this.name}] permission cancellation failed for request ${sanitizeLogText(event.requestId, 128)}: ${this.lifecycleError(respondErr)}\n`, + ); + } + return; + } + this.removePendingPermission(event.requestId); + const pending: PendingPermission = { + requestId: event.requestId, + sessionId: event.sessionId, + target, + request: event.request, + }; + this.pendingPermissions.set(event.requestId, pending); + const chatKey = this.permissionChatKey(target); + const requestIds = this.pendingPermissionsByChat.get(chatKey) ?? []; + requestIds.push(event.requestId); + this.pendingPermissionsByChat.set(chatKey, requestIds); + try { + const text = this.formatPermissionRequest(pending); + if ( + target.threadId !== undefined && + this.supportsProactiveSend() && + this.supportsProactiveTarget(target) + ) { + await this.pushProactive(target, text); + } else { + await this.sendMessage(target.chatId, text); + } + } catch (err) { + this.removePendingPermission(event.requestId); + try { + await this.bridge.respondToPermission?.(event.requestId, { + outcome: { outcome: 'cancelled' }, + }); + } catch (respondErr) { + process.stderr.write( + `[${this.name}] permission cancellation failed for request ${sanitizeLogText(event.requestId, 128)}: ${this.lifecycleError(respondErr)}\n`, + ); + } + throw err; + } + } + + private permissionTargetForEvent( + event: PermissionRequestEvent, + ): SessionTarget | undefined { + const routeTarget = this.router.getTarget(event.sessionId); + if (!routeTarget || routeTarget.channelName !== this.name) { + return undefined; + } + const active = this.activePrompts.get(event.sessionId); + if (!active) { + return routeTarget; + } + const target: SessionTarget = { + channelName: routeTarget.channelName, + senderId: active.senderId ?? routeTarget.senderId, + chatId: active.chatId, + }; + if (active.threadId !== undefined) { + target.threadId = active.threadId; + } + if (active.isGroup !== undefined) { + target.isGroup = active.isGroup; + } else if (routeTarget.isGroup !== undefined) { + target.isGroup = routeTarget.isGroup; + } + return target; + } + + dispatchPermissionResolved(event: PermissionResolvedEvent): void { + this.removePendingPermission(event.requestId); + } + constructor( name: string, config: ChannelConfig, @@ -461,6 +577,7 @@ export abstract class ChannelBase { if (this.registerBridgeEvents) { this.detachBridgeEvents(this.bridge); } + this.clearPendingPermissions(); this.router.setBridge(bridge); this.bridge = bridge; if (this.loopController) { @@ -597,6 +714,8 @@ export abstract class ChannelBase { done, resolve: doneResolve, chatId: job.target.chatId, + threadId: job.target.threadId, + isGroup: job.target.isGroup, messageId: job.id, senderId: job.target.senderId, senderName: job.createdBy, @@ -979,16 +1098,21 @@ export abstract class ChannelBase { onSessionDied(sessionId: string): void { this.router.removeSessionId(sessionId); this.instructedSessions.delete(sessionId); + this.removePendingPermissionsForSession(sessionId); } private attachBridgeEvents(bridge: ChannelAgentBridge): void { bridge.on('toolCall', this.bridgeToolCallListener); bridge.on('sessionDied', this.bridgeSessionDiedListener); + bridge.on('permissionRequest', this.bridgePermissionRequestListener); + bridge.on('permissionResolved', this.bridgePermissionResolvedListener); } private detachBridgeEvents(bridge: ChannelAgentBridge): void { bridge.off('toolCall', this.bridgeToolCallListener); bridge.off('sessionDied', this.bridgeSessionDiedListener); + bridge.off('permissionRequest', this.bridgePermissionRequestListener); + bridge.off('permissionResolved', this.bridgePermissionResolvedListener); } /** @@ -1109,6 +1233,303 @@ export abstract class ChannelBase { }); } + private permissionChatKey( + target: Pick, + ) { + return `${target.chatId}\0${target.threadId ?? ''}`; + } + + private pendingPermissionIdsForChatKey(chatKey: string): string[] { + const requestIds = this.pendingPermissionsByChat.get(chatKey); + if (!requestIds) { + return []; + } + const live = requestIds.filter((id) => this.pendingPermissions.has(id)); + if (live.length === 0) { + this.pendingPermissionsByChat.delete(chatKey); + } else if (live.length !== requestIds.length) { + this.pendingPermissionsByChat.set(chatKey, live); + } + return live; + } + + private removePendingPermission(requestId: string): void { + const pending = this.pendingPermissions.get(requestId); + if (!pending) { + return; + } + this.pendingPermissions.delete(requestId); + const chatKey = this.permissionChatKey(pending.target); + const requestIds = this.pendingPermissionsByChat.get(chatKey); + if (!requestIds) { + return; + } + const remaining = requestIds.filter((id) => id !== requestId); + if (remaining.length === 0) { + this.pendingPermissionsByChat.delete(chatKey); + } else { + this.pendingPermissionsByChat.set(chatKey, remaining); + } + } + + private removePendingPermissionsForSession(sessionId: string): void { + const requestIds = Array.from(this.pendingPermissions) + .filter(([, pending]) => pending.sessionId === sessionId) + .map(([requestId]) => requestId); + for (const requestId of requestIds) { + this.removePendingPermission(requestId); + } + } + + private clearPendingPermissions(): void { + this.pendingPermissions.clear(); + this.pendingPermissionsByChat.clear(); + } + + private pendingPermissionForEnvelope( + envelope: Envelope, + args: string, + ): PendingPermissionLookup { + const trimmed = args.trim(); + if (trimmed) { + const explicit = this.pendingPermissions.get(trimmed); + if ( + explicit && + this.canEnvelopeAnswerPendingPermission(envelope, explicit) + ) { + return { kind: 'found', pending: explicit }; + } + return { kind: 'none', explicit: true }; + } + const requestIds = this.pendingPermissionIdsForChatKey( + this.permissionChatKey(envelope), + ); + if (requestIds.length === 0) { + return { kind: 'none', explicit: false }; + } + const matching = requestIds + .map((id) => this.pendingPermissions.get(id)) + .filter( + (pending): pending is PendingPermission => + pending !== undefined && + this.canEnvelopeAnswerPendingPermission(envelope, pending), + ); + if (matching.length === 0) { + return { kind: 'none', explicit: false }; + } + if (matching.length > 1) { + return { + kind: 'ambiguous', + requestIds: matching.map((pending) => pending.requestId), + }; + } + return { kind: 'found', pending: matching[0]! }; + } + + private canEnvelopeAnswerPendingPermission( + envelope: Envelope, + pending: PendingPermission, + ): boolean { + return ( + pending.target.chatId === envelope.chatId && + pending.target.threadId === envelope.threadId && + (this.isSharedSessionTarget(pending.target) || + pending.target.senderId === envelope.senderId) + ); + } + + private formatPermissionRequest(pending: PendingPermission): string { + const { toolCall } = pending.request; + const title = sanitizeQuotedText(toolCall.title || 'Tool use', 160); + const alwaysOption = this.approvalAlwaysOption(pending); + const replies = [ + '/approve allow once', + ...(alwaysOption ? [`/approve-always ${alwaysOption.label}`] : []), + '/deny deny', + ]; + const lines = [ + 'Permission required to run a tool', + '', + 'Command:', + title, + '', + 'Reply with:', + ...replies, + ]; + return lines.join('\n'); + } + + private approvalOptionId(pending: PendingPermission): string | undefined { + const options = pending.request.options; + return ( + options.find((option) => option.kind === 'allow_once')?.optionId ?? + options.find( + (option) => + option.optionId === 'proceed_once' && + (option as { kind?: string }).kind === undefined, + )?.optionId + ); + } + + private approvalAlwaysOption( + pending: PendingPermission, + ): { optionId: string; label: string } | undefined { + const options = pending.request.options.filter( + (option) => option.kind === 'allow_always', + ); + const option = + this.findScopedAlwaysOption(options, 'project') ?? + this.findScopedAlwaysOption(options, 'user') ?? + options[0]; + if (!option) { + return undefined; + } + return { + optionId: option.optionId, + label: this.approvalAlwaysLabel(option), + }; + } + + private findScopedAlwaysOption( + options: PermissionOption[], + scope: 'project' | 'user', + ): PermissionOption | undefined { + return options.find( + (option) => option.optionId === `proceed_always_${scope}`, + ); + } + + private approvalAlwaysLabel(option: PermissionOption): string { + if (option.optionId === 'proceed_always_project') { + return 'always allow for this project'; + } + if (option.optionId === 'proceed_always_user') { + return 'always allow for this user'; + } + return 'always allow'; + } + + private denialResponse(pending: PendingPermission): { + outcome: + | { outcome: 'selected'; optionId: string } + | { outcome: 'cancelled' }; + } { + const option = + pending.request.options.find( + (candidate) => candidate.kind === 'reject_once', + ) ?? + pending.request.options.find( + (candidate) => + candidate.optionId === 'cancel' && + (candidate as { kind?: string }).kind === undefined, + ); + if (option) { + return { outcome: { outcome: 'selected', optionId: option.optionId } }; + } + return { outcome: { outcome: 'cancelled' } }; + } + + private async handlePermissionResponseCommand( + envelope: Envelope, + args: string, + decision: 'approve' | 'approve-always' | 'deny', + ): Promise { + if (!this.isAuthorizedForSharedSession(envelope)) { + await this.sendMessage( + envelope.chatId, + 'Only authorized members can answer permission requests in this shared session.', + ); + return true; + } + const lookup = this.pendingPermissionForEnvelope(envelope, args); + if (lookup.kind === 'ambiguous') { + const requestList = lookup.requestIds + .slice(0, 6) + .map((id) => { + const pending = this.pendingPermissions.get(id); + const title = pending + ? `: ${sanitizeQuotedText(pending.request.toolCall.title || 'Tool use', 160)}` + : ''; + return `- ${sanitizeQuotedText(id, 128)}${title}`; + }) + .join('\n'); + await this.sendMessage( + envelope.chatId, + `Multiple permission requests are pending for this chat. Reply with /${decision} .\n${requestList}`, + ); + return true; + } + if (lookup.kind === 'none') { + await this.sendMessage( + envelope.chatId, + lookup.explicit + ? 'No pending permission request with that id for this chat.' + : 'No pending permission request for this chat.', + ); + return true; + } + if (!this.bridge.respondToPermission) { + await this.sendMessage( + envelope.chatId, + 'Permission relay is not available for this session.', + ); + return true; + } + + const { pending } = lookup; + const response = (() => { + if (decision === 'deny') { + return this.denialResponse(pending); + } + const optionId = + decision === 'approve' + ? this.approvalOptionId(pending) + : this.approvalAlwaysOption(pending)?.optionId; + return optionId + ? { outcome: { outcome: 'selected' as const, optionId } } + : undefined; + })(); + if (!response) { + await this.sendMessage( + envelope.chatId, + decision === 'approve-always' + ? 'This permission request has no always-allow option.' + : 'This permission request has no approvable option.', + ); + return true; + } + + let accepted: boolean; + try { + accepted = await this.bridge.respondToPermission( + pending.requestId, + response, + ); + } catch (err) { + this.removePendingPermission(pending.requestId); + process.stderr.write( + `[${this.name}] permission response failed for request ${sanitizeLogText(pending.requestId, 128)}: ${this.lifecycleError(err)}\n`, + ); + await this.sendMessage( + envelope.chatId, + 'Failed to answer the permission request.', + ); + return true; + } + this.removePendingPermission(pending.requestId); + await this.sendMessage( + envelope.chatId, + accepted + ? decision === 'approve' + ? 'Permission approved.' + : decision === 'approve-always' + ? 'Permission approved always.' + : 'Permission denied.' + : 'Permission request is no longer pending.', + ); + return true; + } + /** Register shared slash commands. Called from constructor. */ private registerSharedCommands(): void { const doClear = async (envelope: Envelope): Promise => { @@ -1140,6 +1561,7 @@ export abstract class ChannelBase { id, (this.sessionGenerations.get(id) ?? 0) + 1, ); + this.removePendingPermissionsForSession(id); // Cancel an in-flight turn (and drop its buffered follow-ups) before // purging, so a running prompt can't deliver a stale response into — // or resurrect via collect-drain — the just-cleared session. @@ -1256,6 +1678,15 @@ export abstract class ChannelBase { this.registerCommand('clear', clearHandler); this.registerCommand('reset', clearHandler); this.registerCommand('new', clearHandler); + this.registerCommand('approve', (envelope, args) => + this.handlePermissionResponseCommand(envelope, args, 'approve'), + ); + this.registerCommand('approve-always', (envelope, args) => + this.handlePermissionResponseCommand(envelope, args, 'approve-always'), + ); + this.registerCommand('deny', (envelope, args) => + this.handlePermissionResponseCommand(envelope, args, 'deny'), + ); this.registerCommand('remember-channel', async (envelope, args) => { const text = args.trim(); @@ -1364,6 +1795,9 @@ export abstract class ChannelBase { : '/clear — Clear your session (aliases: /reset, /new)', '/who — Show current session & workspace', '/status — Show session info', + '/approve [request-id] — Approve a pending permission request', + '/approve-always [request-id] — Always approve a pending permission request', + '/deny [request-id] — Deny a pending permission request', ]; // Platform-specific commands (registered by adapters, not shared ones) @@ -1372,6 +1806,9 @@ export abstract class ChannelBase { 'clear', 'reset', 'new', + 'approve', + 'approve-always', + 'deny', 'remember-channel', 'channel-memory', 'forget-channel', @@ -2977,6 +3414,8 @@ export abstract class ChannelBase { done, resolve: doneResolve, chatId: envelope.chatId, + threadId: envelope.threadId, + isGroup: envelope.isGroup, messageId: envelope.messageId, senderId: envelope.senderId, senderName: envelope.senderName, diff --git a/packages/channels/base/src/index.ts b/packages/channels/base/src/index.ts index 9dadbcb0d6a..5ac1e037d06 100644 --- a/packages/channels/base/src/index.ts +++ b/packages/channels/base/src/index.ts @@ -7,6 +7,8 @@ export type { ChannelLoopToolCreateInput, ChannelLoopToolHandler, ChannelLoopToolResult, + PermissionRequestEvent, + PermissionResolvedEvent, SessionDiedEvent, ToolCallEvent, } from './ChannelAgentBridge.js'; diff --git a/packages/cli/src/commands/channel/daemon-worker.test.ts b/packages/cli/src/commands/channel/daemon-worker.test.ts index 0cefc9dffc9..f424840988c 100644 --- a/packages/cli/src/commands/channel/daemon-worker.test.ts +++ b/packages/cli/src/commands/channel/daemon-worker.test.ts @@ -9,6 +9,7 @@ const mockReadChannelMemory = vi.hoisted(() => vi.fn()); const mockAppendChannelMemory = vi.hoisted(() => vi.fn()); const mockClearChannelMemory = vi.hoisted(() => vi.fn()); const mockRegisterToolCallDispatch = vi.hoisted(() => vi.fn()); +const mockRegisterPermissionRelay = vi.hoisted(() => vi.fn()); const mockRegisterSessionCleanup = vi.hoisted(() => vi.fn()); const mockSessionsPath = vi.hoisted(() => vi.fn(() => '/tmp/sessions.json')); const mockLoadSettings = vi.hoisted(() => @@ -74,6 +75,7 @@ const mockBridgeNewSession = vi.hoisted(() => vi.fn()); const mockBridgeLoadSession = vi.hoisted(() => vi.fn()); const mockBridgePrompt = vi.hoisted(() => vi.fn()); const mockBridgeCancelSession = vi.hoisted(() => vi.fn()); +const mockBridgeRespondToPermission = vi.hoisted(() => vi.fn()); const mockBridgeShellCommand = vi.hoisted(() => vi.fn()); const mockBridgeGetAvailableCommands = vi.hoisted(() => vi.fn(() => [])); const mockDaemonChannelBridge = vi.hoisted(() => @@ -88,6 +90,7 @@ const mockDaemonChannelBridge = vi.hoisted(() => loadSession: mockBridgeLoadSession, prompt: mockBridgePrompt, cancelSession: mockBridgeCancelSession, + respondToPermission: mockBridgeRespondToPermission, shellCommand: mockBridgeShellCommand, start: mockBridgeStart, stop: mockBridgeStop, @@ -137,6 +140,7 @@ vi.mock('./runtime.js', () => ({ loadChannelsConfig: mockLoadChannelsConfig, loadChannelsFromExtensions: mockLoadChannelsFromExtensions, parseConfiguredChannels: mockParseConfiguredChannels, + registerPermissionRelay: mockRegisterPermissionRelay, registerSessionCleanup: mockRegisterSessionCleanup, registerToolCallDispatch: mockRegisterToolCallDispatch, selectFirstModel: mockSelectFirstModel, @@ -411,6 +415,48 @@ describe('createDaemonChannelBridgeFacade', () => { expect(listSessions).toHaveBeenCalled(); }); + it('forwards permission responses when present on bridge', async () => { + const respondToPermission = vi.fn().mockResolvedValue(true); + const bridge = { + availableCommands: [], + on: mockBridgeOn, + off: mockBridgeOff, + newSession: mockBridgeNewSession, + loadSession: mockBridgeLoadSession, + prompt: mockBridgePrompt, + cancelSession: mockBridgeCancelSession, + respondToPermission, + }; + + const facade = createDaemonChannelBridgeFacade(bridge, { + exposeShellCommand: false, + }); + + const response = { outcome: { outcome: 'cancelled' as const } }; + await expect(facade.respondToPermission?.('req-1', response)).resolves.toBe( + true, + ); + expect(respondToPermission).toHaveBeenCalledWith('req-1', response); + }); + + it('omits permission responses when absent on bridge', () => { + const bridge = { + availableCommands: [], + on: mockBridgeOn, + off: mockBridgeOff, + newSession: mockBridgeNewSession, + loadSession: mockBridgeLoadSession, + prompt: mockBridgePrompt, + cancelSession: mockBridgeCancelSession, + }; + + const facade = createDaemonChannelBridgeFacade(bridge, { + exposeShellCommand: false, + }); + + expect('respondToPermission' in facade).toBe(false); + }); + it('omits listSessions when absent on bridge', () => { const bridge = { availableCommands: [], @@ -502,6 +548,11 @@ describe('runChannelDaemonWorker', () => { }), }), ); + expect(mockRegisterPermissionRelay).toHaveBeenCalledWith( + bridgeFacade, + mockSessionRouter.mock.results[0]!.value, + expect.any(Map), + ); expect(mockResolveProxyUrl).toHaveBeenCalledWith( undefined, 'http://settings-proxy:8080', diff --git a/packages/cli/src/commands/channel/daemon-worker.ts b/packages/cli/src/commands/channel/daemon-worker.ts index ec34b9fbf2a..00867fc0c03 100644 --- a/packages/cli/src/commands/channel/daemon-worker.ts +++ b/packages/cli/src/commands/channel/daemon-worker.ts @@ -36,6 +36,7 @@ import { loadChannelsConfig, loadChannelsFromExtensions, parseConfiguredChannels, + registerPermissionRelay, registerSessionCleanup, registerToolCallDispatch, selectFirstModel, @@ -157,6 +158,10 @@ export function createDaemonChannelBridgeFacade( cancelSession: bridge.cancelSession.bind(bridge), }; + if (bridge.respondToPermission) { + facade.respondToPermission = bridge.respondToPermission.bind(bridge); + } + if (bridge.getAvailableCommands) { facade.getAvailableCommands = bridge.getAvailableCommands.bind(bridge); } @@ -358,6 +363,7 @@ export async function runChannelDaemonWorker( ); } registerToolCallDispatch(bridgeFacade, createdRouter, channels); + registerPermissionRelay(bridgeFacade, createdRouter, channels); registerSessionCleanup(bridgeFacade, createdRouter, channels); for (const [name, channel] of channels) { diff --git a/packages/cli/src/commands/channel/runtime.test.ts b/packages/cli/src/commands/channel/runtime.test.ts index 3df5bdc78fa..271b2abab18 100644 --- a/packages/cli/src/commands/channel/runtime.test.ts +++ b/packages/cli/src/commands/channel/runtime.test.ts @@ -1,5 +1,6 @@ +import { EventEmitter } from 'node:events'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { parseConfiguredChannels } from './runtime.js'; +import { parseConfiguredChannels, registerPermissionRelay } from './runtime.js'; vi.mock('@qwen-code/qwen-code-core', () => ({ Storage: { getGlobalQwenDir: () => '/tmp/qwen' }, @@ -99,3 +100,181 @@ describe('parseConfiguredChannels', () => { expect(parsed[0]?.config.token).toBe('token-from-env'); }); }); + +describe('registerPermissionRelay', () => { + function createBridge() { + const emitter = new EventEmitter(); + return Object.assign(emitter, { + availableCommands: [], + newSession: vi.fn(), + loadSession: vi.fn(), + prompt: vi.fn(), + cancelSession: vi.fn(), + respondToPermission: vi.fn().mockResolvedValue(true), + }); + } + + it('cancels permission requests when no route exists', async () => { + const stderr = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + const bridge = createBridge(); + const router = { getTarget: vi.fn() }; + + try { + registerPermissionRelay(bridge, router as never, new Map()); + bridge.emit('permissionRequest', { + requestId: 'req-1', + sessionId: 'missing-session', + request: { + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [], + }, + }); + + await vi.waitFor(() => + expect(bridge.respondToPermission).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'cancelled' }, + }), + ); + expect(stderr.mock.calls.join('')).toContain( + 'No route for session missing-session; cancelling permission req-1', + ); + } finally { + stderr.mockRestore(); + } + }); + + it('does not crash cancelling permission requests without a responder', () => { + const stderr = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + const bridge = createBridge(); + delete (bridge as { respondToPermission?: unknown }).respondToPermission; + const router = { getTarget: vi.fn() }; + + try { + registerPermissionRelay(bridge, router as never, new Map()); + + expect(() => + bridge.emit('permissionRequest', { + requestId: 'req-1', + sessionId: 'missing-session', + request: { + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [], + }, + }), + ).not.toThrow(); + expect(stderr.mock.calls.join('')).toContain( + 'No route for session missing-session; cancelling permission req-1', + ); + } finally { + stderr.mockRestore(); + } + }); + + it('cancels permission requests when channel dispatch fails', async () => { + const bridge = createBridge(); + const router = { + getTarget: vi.fn(() => ({ channelName: 'telegram', chatId: 'chat1' })), + }; + const channel = { + dispatchPermissionRequest: vi + .fn() + .mockRejectedValue(new Error('send failed')), + dispatchPermissionResolved: vi.fn(), + }; + + registerPermissionRelay( + bridge, + router as never, + new Map([['telegram', channel as never]]), + ); + bridge.emit('permissionRequest', { + requestId: 'req-1', + sessionId: 'session-1', + request: { + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [], + }, + }); + + await vi.waitFor(() => + expect(bridge.respondToPermission).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'cancelled' }, + }), + ); + }); + + it('logs before cancelling permission requests with no channel', async () => { + const stderr = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + const bridge = createBridge(); + const router = { + getTarget: vi.fn(() => ({ channelName: 'telegram', chatId: 'chat1' })), + }; + + try { + registerPermissionRelay(bridge, router as never, new Map()); + bridge.emit('permissionRequest', { + requestId: 'req-1', + sessionId: 'session-1', + request: { + toolCall: { + toolCallId: 'tool-1', + kind: 'shell', + title: 'Run command', + }, + options: [], + }, + }); + + await vi.waitFor(() => + expect(bridge.respondToPermission).toHaveBeenCalledWith('req-1', { + outcome: { outcome: 'cancelled' }, + }), + ); + expect(stderr.mock.calls.join('')).toContain( + 'No channel "telegram" for session session-1; cancelling permission req-1', + ); + } finally { + stderr.mockRestore(); + } + }); + + it('broadcasts resolved permission requests to channels', () => { + const bridge = createBridge(); + const channel = { + dispatchPermissionResolved: vi.fn(), + }; + + registerPermissionRelay( + bridge, + { getTarget: vi.fn() } as never, + new Map([['telegram', channel as never]]), + ); + bridge.emit('permissionResolved', { + requestId: 'req-1', + outcome: { outcome: 'cancelled' }, + }); + + expect(channel.dispatchPermissionResolved).toHaveBeenCalledWith({ + requestId: 'req-1', + outcome: { outcome: 'cancelled' }, + }); + }); +}); diff --git a/packages/cli/src/commands/channel/runtime.ts b/packages/cli/src/commands/channel/runtime.ts index cf7cc6f1135..70a8799fa88 100644 --- a/packages/cli/src/commands/channel/runtime.ts +++ b/packages/cli/src/commands/channel/runtime.ts @@ -7,6 +7,8 @@ import type { ChannelBase, ChannelBaseOptions, ChannelPlugin, + PermissionRequestEvent, + PermissionResolvedEvent, ToolCallEvent, } from '@qwen-code/channel-base'; import { sanitizeLogText } from '@qwen-code/channel-base'; @@ -164,6 +166,59 @@ export function registerToolCallDispatch( }); } +function cancelPermissionRequest( + bridge: ChannelAgentBridge, + requestId: string, +): void { + if (!bridge.respondToPermission) { + return; + } + void bridge + .respondToPermission(requestId, { outcome: { outcome: 'cancelled' } }) + .catch((err: unknown) => { + writeStderrLine( + `[Channel] Permission cancellation failed for ${sanitizeLogText(requestId, 128)}: ${err instanceof Error ? sanitizeLogText(err.message, 512) : sanitizeLogText(String(err), 512)}`, + ); + }); +} + +export function registerPermissionRelay( + bridge: ChannelAgentBridge, + router: SessionRouter, + channels: Map, +): void { + bridge.on('permissionRequest', (event: PermissionRequestEvent) => { + const target = router.getTarget(event.sessionId); + if (!target) { + writeStderrLine( + `[Channel] No route for session ${sanitizeLogText(event.sessionId, 128)}; cancelling permission ${sanitizeLogText(event.requestId, 128)}`, + ); + cancelPermissionRequest(bridge, event.requestId); + return; + } + const channel = channels.get(target.channelName); + if (!channel) { + writeStderrLine( + `[Channel] No channel "${sanitizeLogText(target.channelName, 64)}" for session ${sanitizeLogText(event.sessionId, 128)}; cancelling permission ${sanitizeLogText(event.requestId, 128)}`, + ); + cancelPermissionRequest(bridge, event.requestId); + return; + } + channel.dispatchPermissionRequest(event).catch((err: unknown) => { + writeStderrLine( + `[Channel] Permission relay failed for ${sanitizeLogText(event.requestId, 128)}: ${err instanceof Error ? sanitizeLogText(err.message, 512) : sanitizeLogText(String(err), 512)}`, + ); + cancelPermissionRequest(bridge, event.requestId); + }); + }); + + bridge.on('permissionResolved', (event: PermissionResolvedEvent) => { + for (const channel of channels.values()) { + channel.dispatchPermissionResolved(event); + } + }); +} + export function registerSessionCleanup( bridge: ChannelAgentBridge, router: SessionRouter, diff --git a/packages/cli/src/commands/channel/start.ts b/packages/cli/src/commands/channel/start.ts index badc8d549d6..95b4bd6b470 100644 --- a/packages/cli/src/commands/channel/start.ts +++ b/packages/cli/src/commands/channel/start.ts @@ -32,6 +32,7 @@ import { loadChannelsConfig, loadChannelsFromExtensions, parseConfiguredChannels, + registerPermissionRelay, registerSessionCleanup, registerToolCallDispatch, selectFirstModel, @@ -223,6 +224,7 @@ async function startSingle( }) : undefined; registerToolCallDispatch(bridge, router, channels); + registerPermissionRelay(bridge, router, channels); registerSessionCleanup(bridge, router, channels); try { @@ -276,6 +278,7 @@ async function startSingle( channel.disconnect(); await channel.connect(); registerToolCallDispatch(bridge, router, channels); + registerPermissionRelay(bridge, router, channels); registerSessionCleanup(bridge, router, channels); attachDisconnectHandler(bridge); @@ -384,6 +387,7 @@ async function startAll( ); } registerToolCallDispatch(bridge, router, channels); + registerPermissionRelay(bridge, router, channels); registerSessionCleanup(bridge, router, channels); // Connect all channels @@ -482,6 +486,7 @@ async function startAll( process.exit(1); } registerToolCallDispatch(bridge, router, channels); + registerPermissionRelay(bridge, router, channels); registerSessionCleanup(bridge, router, channels); attachDisconnectHandler(bridge);