diff --git a/ui/desktop/src/acp/__tests__/chatSessionStore.test.ts b/ui/desktop/src/acp/__tests__/chatSessionStore.test.ts index e66ef999f..464598c76 100644 --- a/ui/desktop/src/acp/__tests__/chatSessionStore.test.ts +++ b/ui/desktop/src/acp/__tests__/chatSessionStore.test.ts @@ -12,6 +12,7 @@ import { useAcpChatSessionSnapshot, } from '../chatSessionStore'; import type { AcpElicitationRequest } from '../elicitationRequests'; +import type { AcpPermissionRequest } from '../permissionRequestTypes'; function message(id: string, text: string): Message { return { @@ -39,8 +40,8 @@ function session(id: string, conversation: Message[] = []): Session { } as Session; } -function permissionRequest(sessionId: string, toolCallId = 'tool-1'): RequestPermissionRequest { - return { +function permissionRequest(sessionId: string, toolCallId = 'tool-1'): AcpPermissionRequest { + const request: RequestPermissionRequest = { sessionId, options: [{ optionId: 'allow-once', name: 'Allow once', kind: 'allow_once' }], toolCall: { @@ -62,6 +63,7 @@ function permissionRequest(sessionId: string, toolCallId = 'tool-1'): RequestPer }, }, }; + return { generation: `generation-${toolCallId}`, request }; } function elicitationRequest(sessionId: string): AcpElicitationRequest { @@ -559,6 +561,20 @@ describe('acpChatSessionStore', () => { }); }); + it('removes the current permission card when the request is cancelled', () => { + const currentSessionId = sessionId('session-1'); + const request = permissionRequest(currentSessionId, 'tool-1'); + acpChatSessionActions.applyPermissionRequest(request); + + const snapshot = acpChatSessionActions.cancelPermissionRequest( + currentSessionId, + 'tool-1', + request.generation + ); + + expect(snapshot?.messages).toEqual([]); + }); + it('resumes streaming only after the final pending user input request resolves', () => { const currentSessionId = sessionId('session-1'); diff --git a/ui/desktop/src/acp/__tests__/permissionRequests.test.ts b/ui/desktop/src/acp/__tests__/permissionRequests.test.ts index 839c71fc8..2de3bae37 100644 --- a/ui/desktop/src/acp/__tests__/permissionRequests.test.ts +++ b/ui/desktop/src/acp/__tests__/permissionRequests.test.ts @@ -11,6 +11,7 @@ vi.mock('../chatSessionStore', () => ({ acpPermissionUserInputRequestId: (toolCallId: string) => `permission:${toolCallId}`, acpChatSessionActions: { applyPermissionRequest: vi.fn(), + cancelPermissionRequest: vi.fn(), resolveUserInputRequest: vi.fn(), }, })); @@ -54,6 +55,11 @@ async function expectStillPending(promise: Promise): expect(settled).toBe(false); } +function appliedGeneration(callIndex = 0): string { + return vi.mocked(acpChatSessionActions.applyPermissionRequest).mock.calls[callIndex][0] + .generation; +} + describe('ACP permission requests', () => { beforeEach(() => { vi.clearAllMocks(); @@ -70,10 +76,11 @@ describe('ACP permission requests', () => { it('keeps permission requests pending until explicit resolve', async () => { const response = requestAcpPermission(permissionRequest('session-1', 'tool-1')); + const generation = appliedGeneration(); await expectStillPending(response); - expect(resolveAcpPermissionRequest('session-1', 'tool-1', 'allow_once')).toBe(true); + expect(resolveAcpPermissionRequest('session-1', 'tool-1', generation, 'allow_once')).toBe(true); expect(acpChatSessionActions.resolveUserInputRequest).toHaveBeenCalledWith( 'session-1', 'permission:tool-1' @@ -89,6 +96,7 @@ describe('ACP permission requests', () => { it('cancels only pending requests for the requested session', async () => { const sessionOneResponse = requestAcpPermission(permissionRequest('session-1', 'tool-1')); const sessionTwoResponse = requestAcpPermission(permissionRequest('session-2', 'tool-2')); + const sessionTwoGeneration = appliedGeneration(1); cancelAcpPermissionRequestsForSession('session-1'); @@ -98,8 +106,15 @@ describe('ACP permission requests', () => { }, }); await expectStillPending(sessionTwoResponse); + expect(acpChatSessionActions.cancelPermissionRequest).toHaveBeenCalledWith( + 'session-1', + 'tool-1', + appliedGeneration() + ); - expect(resolveAcpPermissionRequest('session-2', 'tool-2', 'deny_once')).toBe(true); + expect( + resolveAcpPermissionRequest('session-2', 'tool-2', sessionTwoGeneration, 'deny_once') + ).toBe(true); await expect(sessionTwoResponse).resolves.toEqual({ outcome: { outcome: 'selected', @@ -110,7 +125,9 @@ describe('ACP permission requests', () => { it('cancels an older duplicate request for the same session and tool call', async () => { const firstResponse = requestAcpPermission(permissionRequest('session-1', 'tool-1')); + const firstGeneration = appliedGeneration(); const secondResponse = requestAcpPermission(permissionRequest('session-1', 'tool-1')); + const secondGeneration = appliedGeneration(1); await expect(firstResponse).resolves.toEqual({ outcome: { @@ -118,8 +135,16 @@ describe('ACP permission requests', () => { }, }); await expectStillPending(secondResponse); + expect(firstGeneration).not.toBe(secondGeneration); - expect(resolveAcpPermissionRequest('session-1', 'tool-1', 'allow_once')).toBe(true); + expect(resolveAcpPermissionRequest('session-1', 'tool-1', firstGeneration, 'allow_once')).toBe( + false + ); + await expectStillPending(secondResponse); + + expect(resolveAcpPermissionRequest('session-1', 'tool-1', secondGeneration, 'allow_once')).toBe( + true + ); await expect(secondResponse).resolves.toEqual({ outcome: { outcome: 'selected', @@ -127,4 +152,29 @@ describe('ACP permission requests', () => { }, }); }); + + it('fails closed when a legacy card has no permission generation', async () => { + const response = requestAcpPermission(permissionRequest('session-1', 'tool-1')); + + expect(resolveAcpPermissionRequest('session-1', 'tool-1', undefined, 'allow_once')).toBe(false); + await expectStillPending(response); + }); + + it('keeps distinct tool call IDs independently resolvable', async () => { + const firstResponse = requestAcpPermission(permissionRequest('session-1', 'tool-1')); + const firstGeneration = appliedGeneration(); + const secondResponse = requestAcpPermission(permissionRequest('session-1', 'tool-2')); + const secondGeneration = appliedGeneration(1); + + expect(resolveAcpPermissionRequest('session-1', 'tool-1', firstGeneration, 'allow_once')).toBe( + true + ); + await expect(firstResponse).resolves.toMatchObject({ outcome: { outcome: 'selected' } }); + await expectStillPending(secondResponse); + + expect(resolveAcpPermissionRequest('session-1', 'tool-2', secondGeneration, 'deny_once')).toBe( + true + ); + await expect(secondResponse).resolves.toMatchObject({ outcome: { outcome: 'selected' } }); + }); }); diff --git a/ui/desktop/src/acp/__tests__/sessionNotificationAdapter.test.ts b/ui/desktop/src/acp/__tests__/sessionNotificationAdapter.test.ts index aef953b5f..af98c82dc 100644 --- a/ui/desktop/src/acp/__tests__/sessionNotificationAdapter.test.ts +++ b/ui/desktop/src/acp/__tests__/sessionNotificationAdapter.test.ts @@ -114,6 +114,24 @@ function firstContent(message: Message): Message['content'][number] { return content; } +function permissionRequest( + sessionId: string, + toolCallId: string, + title: string, + path: string +): RequestPermissionRequest { + return { + sessionId, + options: [{ optionId: 'allow', name: 'Allow', kind: 'allow_once' }], + toolCall: { + toolCallId, + title, + rawInput: { path }, + content: [{ type: 'content', content: { type: 'text', text: `Allow ${title}?` } }], + }, + }; +} + describe('createAcpSessionNotificationAdapter', () => { describe('apply', () => { describe('message chunks', () => { @@ -925,7 +943,10 @@ describe('createAcpSessionNotificationAdapter', () => { }, }; - const permissionStateChanges = adapter.applyPermissionRequest(request); + const permissionStateChanges = adapter.applyPermissionRequest({ + generation: 'permission-generation-1', + request, + }); const messages = expectOnlyMessagesChange(permissionStateChanges); expect(messages).toHaveLength(1); @@ -934,6 +955,7 @@ describe('createAcpSessionNotificationAdapter', () => { type: 'actionRequired', data: { actionType: 'toolConfirmation', + generation: 'permission-generation-1', id: 'tool-1', toolName: 'edit_file', arguments: { path: 'README.md' }, @@ -941,6 +963,43 @@ describe('createAcpSessionNotificationAdapter', () => { }, }); }); + + it('replaces reused tool call IDs with the current permission details', () => { + const adapter = createAcpSessionNotificationAdapter(); + const first = permissionRequest(SESSION_ID, 'tool-1', 'Read file', 'README.md'); + const second = permissionRequest(SESSION_ID, 'tool-1', 'Run command', 'secrets.txt'); + + adapter.applyPermissionRequest({ generation: 'generation-a', request: first }); + const messages = expectOnlyMessagesChange( + adapter.applyPermissionRequest({ generation: 'generation-b', request: second }) + ); + + expect(messages).toHaveLength(1); + expect(firstContent(messages[0])).toMatchObject({ + type: 'actionRequired', + data: { + actionType: 'toolConfirmation', + generation: 'generation-b', + id: 'tool-1', + toolName: 'Run command', + arguments: { path: 'secrets.txt' }, + prompt: 'Allow Run command?', + }, + }); + }); + + it('removes only the matching permission generation when cancelled', () => { + const adapter = createAcpSessionNotificationAdapter(); + const request = permissionRequest(SESSION_ID, 'tool-1', 'Read file', 'README.md'); + adapter.applyPermissionRequest({ generation: 'generation-a', request }); + + expect(adapter.cancelPermissionRequest('tool-1', 'generation-stale')).toEqual([]); + const messages = expectOnlyMessagesChange( + adapter.cancelPermissionRequest('tool-1', 'generation-a') + ); + + expect(messages).toEqual([]); + }); }); describe('session_info_update with queuedSteer', () => { diff --git a/ui/desktop/src/acp/adapter/permissions.ts b/ui/desktop/src/acp/adapter/permissions.ts index e99212038..a4794cad9 100644 --- a/ui/desktop/src/acp/adapter/permissions.ts +++ b/ui/desktop/src/acp/adapter/permissions.ts @@ -1,4 +1,5 @@ import type { RequestPermissionRequest } from '@agentclientprotocol/sdk'; +import type { AcpPermissionRequest } from '../permissionRequestTypes'; import { type AcpChatStateChange, type AdapterState, @@ -10,20 +11,11 @@ import { export function applyPermissionRequest( state: AdapterState, - request: RequestPermissionRequest + permissionRequest: AcpPermissionRequest ): AcpChatStateChange[] { + const { generation, request } = permissionRequest; const toolCallId = request.toolCall.toolCallId; - const existing = state.messages.some((message) => - message.content.some( - (content) => - content.type === 'actionRequired' && - content.data.actionType === 'toolConfirmation' && - content.data.id === toolCallId - ) - ); - if (existing) { - return messagesChange(state); - } + removePermissionRequestFromState(state, toolCallId); const identity = toolIdentity(request.toolCall); const prompt = permissionPrompt(request); @@ -37,6 +29,7 @@ export function applyPermissionRequest( type: 'actionRequired', data: { actionType: 'toolConfirmation', + generation, id: toolCallId, toolName: identity.toolName ?? request.toolCall.title ?? toolCallId, arguments: rawInputToArguments(request.toolCall.rawInput), @@ -50,6 +43,44 @@ export function applyPermissionRequest( return messagesChange(state); } +export function cancelPermissionRequest( + state: AdapterState, + toolCallId: string, + generation: string +): AcpChatStateChange[] { + return removePermissionRequestFromState(state, toolCallId, generation) + ? messagesChange(state) + : []; +} + +function removePermissionRequestFromState( + state: AdapterState, + toolCallId: string, + generation?: string +): boolean { + let changed = false; + + state.messages = state.messages.flatMap((message) => { + const content = message.content.filter((content) => { + const matches = + content.type === 'actionRequired' && + content.data.actionType === 'toolConfirmation' && + content.data.id === toolCallId && + (generation === undefined || content.data.generation === generation); + changed ||= matches; + return !matches; + }); + + if (content.length === message.content.length) { + return [message]; + } + + return content.length > 0 ? [{ ...message, content }] : []; + }); + + return changed; +} + function permissionPrompt(request: RequestPermissionRequest): string | undefined { for (const content of request.toolCall.content ?? []) { if (content.type === 'content' && content.content.type === 'text') { diff --git a/ui/desktop/src/acp/chatSessionStore.ts b/ui/desktop/src/acp/chatSessionStore.ts index 021b86691..f0b0e3964 100644 --- a/ui/desktop/src/acp/chatSessionStore.ts +++ b/ui/desktop/src/acp/chatSessionStore.ts @@ -1,6 +1,6 @@ import { useEffect, useState } from 'react'; import type { GooseSessionNotification_unstable } from '@aaif/goose-sdk'; -import type { RequestPermissionRequest, SessionNotification } from '@agentclientprotocol/sdk'; +import type { SessionNotification } from '@agentclientprotocol/sdk'; import type { TokenState } from '../types/chat'; import { ChatState } from '../types/chatState'; import type { Message, NotificationEvent } from '../types/message'; @@ -13,6 +13,7 @@ import { import type { ElicitationStatus } from './adapter/elicitations'; import { cloneMessage } from './adapter/shared'; import type { AcpElicitationRequest } from './elicitationRequests'; +import type { AcpPermissionRequest } from './permissionRequestTypes'; export interface AcpChatSessionSnapshot { session: Session | undefined; @@ -65,7 +66,12 @@ export interface AcpChatSessionActions { applyAcpGooseSessionNotification( notification: GooseSessionNotification_unstable ): AcpChatSessionSnapshot; - applyPermissionRequest(request: RequestPermissionRequest): AcpChatSessionSnapshot; + applyPermissionRequest(request: AcpPermissionRequest): AcpChatSessionSnapshot; + cancelPermissionRequest( + sessionId: string, + toolCallId: string, + generation: string + ): AcpChatSessionSnapshot | undefined; applyElicitationRequest(request: AcpElicitationRequest): AcpChatSessionSnapshot; setElicitationStatus( sessionId: string, @@ -483,14 +489,30 @@ function createAcpChatSessionStoreInternal(): AcpChatSessionStoreInternal { }; const applyPermissionRequest: AcpChatSessionActions['applyPermissionRequest'] = (request) => { - const entry = getOrCreateEntry(request.sessionId); + const entry = getOrCreateEntry(request.request.sessionId); const changes = entry.adapter.applyPermissionRequest(request); applyChatStateChanges(entry, changes); entry.pendingUserInputRequestIds.add( - acpPermissionUserInputRequestId(request.toolCall.toolCallId) + acpPermissionUserInputRequestId(request.request.toolCall.toolCallId) ); entry.chatState = ChatState.WaitingForUserInput; - return notify(request.sessionId, entry); + return notify(request.request.sessionId, entry); + }; + + const cancelPermissionRequest: AcpChatSessionActions['cancelPermissionRequest'] = ( + sessionId, + toolCallId, + generation + ) => { + const entry = sessionsById.get(sessionId); + if (!entry) { + return undefined; + } + + const changes = entry.adapter.cancelPermissionRequest(toolCallId, generation); + applyChatStateChanges(entry, changes); + entry.pendingUserInputRequestIds.delete(acpPermissionUserInputRequestId(toolCallId)); + return notify(sessionId, entry); }; const applyElicitationRequest: AcpChatSessionActions['applyElicitationRequest'] = (request) => { @@ -545,6 +567,7 @@ function createAcpChatSessionStoreInternal(): AcpChatSessionStoreInternal { applyAcpSessionNotification, applyAcpGooseSessionNotification, applyPermissionRequest, + cancelPermissionRequest, applyElicitationRequest, setElicitationStatus, }; @@ -601,6 +624,7 @@ function actionsFromStore(store: AcpChatSessionStoreInternal): AcpChatSessionAct applyAcpSessionNotification: store.applyAcpSessionNotification, applyAcpGooseSessionNotification: store.applyAcpGooseSessionNotification, applyPermissionRequest: store.applyPermissionRequest, + cancelPermissionRequest: store.cancelPermissionRequest, applyElicitationRequest: store.applyElicitationRequest, setElicitationStatus: store.setElicitationStatus, setSessionMetadata: store.setSessionMetadata, diff --git a/ui/desktop/src/acp/permissionRequestTypes.ts b/ui/desktop/src/acp/permissionRequestTypes.ts new file mode 100644 index 000000000..0ea89c347 --- /dev/null +++ b/ui/desktop/src/acp/permissionRequestTypes.ts @@ -0,0 +1,6 @@ +import type { RequestPermissionRequest } from '@agentclientprotocol/sdk'; + +export interface AcpPermissionRequest { + generation: string; + request: RequestPermissionRequest; +} diff --git a/ui/desktop/src/acp/permissionRequests.ts b/ui/desktop/src/acp/permissionRequests.ts index fbfc9a180..0f8e19050 100644 --- a/ui/desktop/src/acp/permissionRequests.ts +++ b/ui/desktop/src/acp/permissionRequests.ts @@ -1,9 +1,11 @@ import type { RequestPermissionRequest, RequestPermissionResponse } from '@agentclientprotocol/sdk'; import type { Permission } from '../types/permissions'; import { acpChatSessionActions, acpPermissionUserInputRequestId } from './chatSessionStore'; +import type { AcpPermissionRequest } from './permissionRequestTypes'; interface PendingPermissionRequest { request: RequestPermissionRequest; + generation: string; resolve: (response: RequestPermissionResponse) => void; } @@ -19,19 +21,24 @@ export async function requestAcpPermission( } return new Promise((resolve) => { - pendingRequests.set(key, { request, resolve }); - acpChatSessionActions.applyPermissionRequest(request); + const permissionRequest: AcpPermissionRequest = { + generation: globalThis.crypto.randomUUID(), + request, + }; + pendingRequests.set(key, { ...permissionRequest, resolve }); + acpChatSessionActions.applyPermissionRequest(permissionRequest); }); } export function resolveAcpPermissionRequest( sessionId: string, toolCallId: string, + generation: string | undefined, action: Permission ): boolean { const key = permissionRequestKey(sessionId, toolCallId); const pending = pendingRequests.get(key); - if (!pending) { + if (!pending || !generation || pending.generation !== generation) { return false; } @@ -48,6 +55,11 @@ export function cancelAcpPermissionRequestsForSession(sessionId: string): void { for (const [key, pending] of pendingRequests) { if (pending.request.sessionId === sessionId) { pendingRequests.delete(key); + acpChatSessionActions.cancelPermissionRequest( + sessionId, + pending.request.toolCall.toolCallId, + pending.generation + ); pending.resolve(cancelledPermissionResponse()); } } diff --git a/ui/desktop/src/acp/sessionNotificationAdapter.ts b/ui/desktop/src/acp/sessionNotificationAdapter.ts index ac34a6bfa..a9aca5f37 100644 --- a/ui/desktop/src/acp/sessionNotificationAdapter.ts +++ b/ui/desktop/src/acp/sessionNotificationAdapter.ts @@ -1,5 +1,5 @@ import type { GooseSessionNotification_unstable } from '@aaif/goose-sdk'; -import type { RequestPermissionRequest, SessionNotification } from '@agentclientprotocol/sdk'; +import type { SessionNotification } from '@agentclientprotocol/sdk'; import type { Message } from '../types/message'; import { applyElicitationRequest as applyElicitationRequestToState, @@ -8,7 +8,10 @@ import { } from './adapter/elicitations'; import { applyGooseSessionNotification } from './adapter/gooseSessionNotifications'; import { applyContentChunk, applyThoughtChunk } from './adapter/messages'; -import { applyPermissionRequest as applyPermissionRequestToState } from './adapter/permissions'; +import { + applyPermissionRequest as applyPermissionRequestToState, + cancelPermissionRequest as cancelPermissionRequestInState, +} from './adapter/permissions'; import { type AcpChatStateChange, type AdapterState, @@ -18,13 +21,15 @@ import { } from './adapter/shared'; import { applyToolCall, applyToolCallUpdate } from './adapter/tools'; import type { AcpElicitationRequest } from './elicitationRequests'; +import type { AcpPermissionRequest } from './permissionRequestTypes'; export type { AcpChatStateChange } from './adapter/shared'; export interface AcpSessionNotificationAdapter { apply(notification: SessionNotification): AcpChatStateChange[]; applyGoose(notification: GooseSessionNotification_unstable): AcpChatStateChange[]; - applyPermissionRequest(request: RequestPermissionRequest): AcpChatStateChange[]; + applyPermissionRequest(request: AcpPermissionRequest): AcpChatStateChange[]; + cancelPermissionRequest(toolCallId: string, generation: string): AcpChatStateChange[]; applyElicitationRequest(request: AcpElicitationRequest): AcpChatStateChange[]; applyElicitationStatus(elicitationId: string, status: ElicitationStatus): AcpChatStateChange[]; getMessages(): Message[]; @@ -50,6 +55,9 @@ export function createAcpSessionNotificationAdapter( applyPermissionRequest(request) { return applyPermissionRequestToState(state, request); }, + cancelPermissionRequest(toolCallId, generation) { + return cancelPermissionRequestInState(state, toolCallId, generation); + }, applyElicitationRequest(request) { return applyElicitationRequestToState(state, request); }, diff --git a/ui/desktop/src/components/ToolApprovalButtons.test.tsx b/ui/desktop/src/components/ToolApprovalButtons.test.tsx index 89b21314e..2bbd38fe3 100644 --- a/ui/desktop/src/components/ToolApprovalButtons.test.tsx +++ b/ui/desktop/src/components/ToolApprovalButtons.test.tsx @@ -26,6 +26,7 @@ describe('ToolApprovalButtons', () => { { expect(resolveAcpPermissionRequestMock).toHaveBeenCalledWith( 'session-1', 'tool-call-approved', + 'permission-generation-1', 'allow_once' ); expect(screen.getByText('developer__shell - Allowed once')).toBeInTheDocument(); @@ -60,9 +62,46 @@ describe('ToolApprovalButtons', () => { expect(resolveAcpPermissionRequestMock).toHaveBeenCalledWith( 'session-1', 'tool-call-rerun', + undefined, 'allow_once' ); expect(screen.getByText('This approval request is no longer active.')).toBeInTheDocument(); expect(screen.queryByText('developer__shell - Allowed once')).not.toBeInTheDocument(); }); + + it('resets the displayed decision for a new permission generation', async () => { + resolveAcpPermissionRequestMock.mockReturnValue(true); + const { rerender } = renderWithIntl( + + ); + + await userEvent.click(screen.getByRole('button', { name: 'Allow Once' })); + expect(screen.getByText('developer__shell - Allowed once')).toBeInTheDocument(); + + rerender( + + ); + await userEvent.click(await screen.findByRole('button', { name: 'Allow Once' })); + + expect(resolveAcpPermissionRequestMock).toHaveBeenLastCalledWith( + 'session-1', + 'tool-call-reused', + 'permission-generation-b', + 'allow_once' + ); + }); }); diff --git a/ui/desktop/src/components/ToolApprovalButtons.tsx b/ui/desktop/src/components/ToolApprovalButtons.tsx index 5dcfbce04..7f793fc4e 100644 --- a/ui/desktop/src/components/ToolApprovalButtons.tsx +++ b/ui/desktop/src/components/ToolApprovalButtons.tsx @@ -52,6 +52,7 @@ const globalApprovalState = new Map< >(); export interface ToolApprovalData { + generation?: string; id: string; toolName: string; prompt?: string; @@ -61,9 +62,10 @@ export interface ToolApprovalData { export default function ToolApprovalButtons({ data }: { data: ToolApprovalData }) { const intl = useIntl(); - const { id, toolName, prompt, sessionId, isClicked: initialIsClicked } = data; + const { generation, id, toolName, prompt, sessionId, isClicked: initialIsClicked } = data; + const approvalStateKey = generation ?? id; - const storedState = globalApprovalState.get(id); + const storedState = globalApprovalState.get(approvalStateKey); const [decision, setDecision] = useState(storedState?.decision ?? null); const [isClicked, setIsClicked] = useState(storedState?.isClicked ?? initialIsClicked ?? false); const [approvalError, setApprovalError] = useState(null); @@ -75,21 +77,24 @@ export default function ToolApprovalButtons({ data }: { data: ToolApprovalData } }; useEffect(() => { - const currentState = globalApprovalState.get(id); + const currentState = globalApprovalState.get(approvalStateKey); if (currentState) { setDecision(currentState.decision); setIsClicked(currentState.isClicked); + } else { + setDecision(null); + setIsClicked(initialIsClicked ?? false); } setApprovalError(null); - }, [id]); + }, [approvalStateKey, initialIsClicked]); useEffect(() => { - globalApprovalState.set(id, { decision, isClicked }); - }, [id, decision, isClicked]); + globalApprovalState.set(approvalStateKey, { decision, isClicked }); + }, [approvalStateKey, decision, isClicked]); const handleAction = async (action: Permission) => { try { - if (resolveAcpPermissionRequest(sessionId, id, action)) { + if (resolveAcpPermissionRequest(sessionId, id, generation, action)) { setResolvedDecision(action); } else { setApprovalError(intl.formatMessage(i18n.staleApprovalRequest)); diff --git a/ui/desktop/src/components/ToolCallConfirmation.test.tsx b/ui/desktop/src/components/ToolCallConfirmation.test.tsx index 0579fdb2d..cfd0fda49 100644 --- a/ui/desktop/src/components/ToolCallConfirmation.test.tsx +++ b/ui/desktop/src/components/ToolCallConfirmation.test.tsx @@ -5,7 +5,9 @@ import type { ActionRequired } from '../types/message'; import ToolCallConfirmation from './ToolCallConfirmation'; vi.mock('./ToolApprovalButtons', () => ({ - default: () =>
, + default: ({ data }: { data: { generation?: string } }) => ( +
+ ), })); const securityPrompt = 'This command sends a local file to a remote service.'; @@ -14,6 +16,7 @@ const actionRequiredContent = { type: 'actionRequired', data: { actionType: 'toolConfirmation', + generation: 'permission-generation-1', id: 'request-1', toolName: 'developer__shell', arguments: { @@ -36,7 +39,10 @@ describe('ToolCallConfirmation', () => { expect(screen.getByText('command')).toBeInTheDocument(); expect(screen.getByText(/upload \/home\/alice\/private\.txt/)).toBeInTheDocument(); - expect(screen.getByTestId('approval-buttons')).toBeInTheDocument(); + expect(screen.getByTestId('approval-buttons')).toHaveAttribute( + 'data-generation', + 'permission-generation-1' + ); }); it('shows the security prompt before approval', () => { diff --git a/ui/desktop/src/components/ToolCallConfirmation.tsx b/ui/desktop/src/components/ToolCallConfirmation.tsx index bd50f76e5..533a18373 100644 --- a/ui/desktop/src/components/ToolCallConfirmation.tsx +++ b/ui/desktop/src/components/ToolCallConfirmation.tsx @@ -36,7 +36,7 @@ export default function ToolConfirmation({ }: ToolConfirmationProps) { const intl = useIntl(); const data = actionRequiredContent.data as ToolConfirmationData; - const { id, toolName, arguments: toolArguments, prompt } = data; + const { generation, id, toolName, arguments: toolArguments, prompt } = data; const displayName = formatToolName(toolName); return ( @@ -50,7 +50,7 @@ export default function ToolConfirmation({ {prompt &&
{prompt}
} } />
diff --git a/ui/desktop/src/components/ToolCallWithResponse.test.tsx b/ui/desktop/src/components/ToolCallWithResponse.test.tsx index 42b7e786c..e168aad06 100644 --- a/ui/desktop/src/components/ToolCallWithResponse.test.tsx +++ b/ui/desktop/src/components/ToolCallWithResponse.test.tsx @@ -1,13 +1,21 @@ import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { IntlTestWrapper } from '../i18n/test-utils'; import type { + Message, NotificationEvent, ToolRequestMessageContent, ToolResponseMessageContent, } from '../types/message'; +import { getAnyToolConfirmationData } from '../types/message'; +import { resolveAcpPermissionRequest } from '../acp/permissionRequests'; import ToolCallWithResponse from './ToolCallWithResponse'; +vi.mock('../acp/permissionRequests', () => ({ + resolveAcpPermissionRequest: vi.fn(), +})); + const toolRequest: ToolRequestMessageContent = { type: 'toolRequest', id: 'tool-1', @@ -100,4 +108,44 @@ describe('ToolCallWithResponse live output', () => { expect(screen.queryByText(/starting/)).not.toBeInTheDocument(); expect(await screen.findByText('final result')).toBeInTheDocument(); }); + + it('passes the current ACP permission generation through inline approval', async () => { + const permissionMessage: Message = { + content: [ + { + type: 'actionRequired', + data: { + actionType: 'toolConfirmation', + arguments: { command: 'build' }, + generation: 'permission-generation-1', + id: 'tool-1', + toolName: 'developer__shell', + }, + }, + ], + created: 0, + metadata: { agentVisible: true, userVisible: true }, + role: 'assistant', + }; + + render( + , + { wrapper: IntlTestWrapper } + ); + + await userEvent.click(screen.getByRole('button', { name: 'Allow Once' })); + + expect(resolveAcpPermissionRequest).toHaveBeenCalledWith( + 'session-1', + 'tool-1', + 'permission-generation-1', + 'allow_once' + ); + }); }); diff --git a/ui/desktop/src/components/ToolCallWithResponse.tsx b/ui/desktop/src/components/ToolCallWithResponse.tsx index 7f9727a6a..ce2d22436 100644 --- a/ui/desktop/src/components/ToolCallWithResponse.tsx +++ b/ui/desktop/src/components/ToolCallWithResponse.tsx @@ -284,6 +284,7 @@ export default function ToolCallWithResponse({
; @@ -425,6 +427,7 @@ export function getAnyToolConfirmationData(message: Message): ToolConfirmationDa const actionRequired = getToolConfirmationContent(message); if (actionRequired && actionRequired.data.actionType === 'toolConfirmation') { return { + generation: actionRequired.data.generation, id: actionRequired.data.id, toolName: actionRequired.data.toolName, arguments: actionRequired.data.arguments,