diff --git a/packages/cli/src/acp-integration/session/Session.ts b/packages/cli/src/acp-integration/session/Session.ts index 45b837569..f9e47cdae 100644 --- a/packages/cli/src/acp-integration/session/Session.ts +++ b/packages/cli/src/acp-integration/session/Session.ts @@ -623,7 +623,7 @@ export class Session implements SessionContext { if (confirmationDetails.type === 'edit') { content.push({ type: 'diff', - path: confirmationDetails.fileName, + path: confirmationDetails.filePath || confirmationDetails.fileName, oldText: confirmationDetails.originalContent, newText: confirmationDetails.newContent, }); diff --git a/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts b/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts index 0be126ff4..4c4025c82 100644 --- a/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts +++ b/packages/cli/src/acp-integration/session/SubAgentTracker.test.ts @@ -531,6 +531,86 @@ describe('SubAgentTracker', () => { expect(respondSpy).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); }); }); + + it('should use filePath over fileName for diff content path', async () => { + tracker.setup(eventEmitter, abortController.signal); + + const respondSpy = vi.fn().mockResolvedValue(undefined); + const event = createApprovalEvent({ + name: 'edit_file', + callId: 'call-path-test', + description: 'Editing file', + confirmationDetails: createEditConfirmation({ + fileName: 'test.ts', + filePath: '/workspace/src/test.ts', + originalContent: 'old content', + newContent: 'new content', + }), + respond: respondSpy, + }); + + eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); + + await vi.waitFor(() => { + expect(requestPermissionSpy).toHaveBeenCalled(); + }); + + expect(requestPermissionSpy).toHaveBeenCalledWith( + expect.objectContaining({ + toolCall: expect.objectContaining({ + content: [ + { + type: 'diff', + path: '/workspace/src/test.ts', + oldText: 'old content', + newText: 'new content', + }, + ], + }), + }), + ); + }); + + it('should fall back to fileName when filePath is not available', async () => { + tracker.setup(eventEmitter, abortController.signal); + + const respondSpy = vi.fn().mockResolvedValue(undefined); + const event = createApprovalEvent({ + name: 'edit_file', + callId: 'call-fallback-test', + description: 'Editing file', + confirmationDetails: { + type: 'edit' as const, + title: 'Edit file', + fileName: 'fallback.ts', + fileDiff: '', + originalContent: 'old', + newContent: 'new', + } as Omit, + respond: respondSpy, + }); + + eventEmitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, event); + + await vi.waitFor(() => { + expect(requestPermissionSpy).toHaveBeenCalled(); + }); + + expect(requestPermissionSpy).toHaveBeenCalledWith( + expect.objectContaining({ + toolCall: expect.objectContaining({ + content: [ + { + type: 'diff', + path: 'fallback.ts', + oldText: 'old', + newText: 'new', + }, + ], + }), + }), + ); + }); }); describe('permission options', () => { diff --git a/packages/cli/src/acp-integration/session/SubAgentTracker.ts b/packages/cli/src/acp-integration/session/SubAgentTracker.ts index 5536390bc..7a904e9e6 100644 --- a/packages/cli/src/acp-integration/session/SubAgentTracker.ts +++ b/packages/cli/src/acp-integration/session/SubAgentTracker.ts @@ -226,12 +226,13 @@ export class SubAgentTracker { const editDetails = event.confirmationDetails as unknown as { type: 'edit'; fileName: string; + filePath: string; originalContent: string | null; newContent: string; }; content.push({ type: 'diff', - path: editDetails.fileName, + path: editDetails.filePath || editDetails.fileName, oldText: editDetails.originalContent ?? '', newText: editDetails.newContent, }); diff --git a/packages/core/src/tools/agent.test.ts b/packages/core/src/tools/agent.test.ts index aae2f6373..2894fe335 100644 --- a/packages/core/src/tools/agent.test.ts +++ b/packages/core/src/tools/agent.test.ts @@ -8,6 +8,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { AgentTool, type AgentParams } from './agent.js'; import type { PartListUnion } from '@google/genai'; import type { ToolResultDisplay, AgentResultDisplay } from './tools.js'; +import { ToolConfirmationOutcome } from './tools.js'; import type { Config } from '../config/config.js'; import { SubagentManager } from '../subagents/subagent-manager.js'; import type { SubagentConfig } from '../subagents/types.js'; @@ -16,22 +17,34 @@ import { type AgentHeadless, ContextState, } from '../agents/runtime/agent-headless.js'; +import { + AgentEventType, +} from '../agents/runtime/agent-events.js'; +import type { + AgentToolCallEvent, + AgentToolResultEvent, + AgentApprovalRequestEvent, + + AgentEventEmitter} from '../agents/runtime/agent-events.js'; import { partToString } from '../utils/partUtils.js'; import type { HookSystem } from '../hooks/hookSystem.js'; import { PermissionMode } from '../hooks/types.js'; // Type for accessing protected methods in tests +type AgentToolInvocation = { + execute: ( + signal?: AbortSignal, + updateOutput?: (output: ToolResultDisplay) => void, + ) => Promise<{ + llmContent: PartListUnion; + returnDisplay: ToolResultDisplay; + }>; + getDescription: () => string; + eventEmitter: AgentEventEmitter; +}; + type AgentToolWithProtectedMethods = AgentTool & { - createInvocation: (params: AgentParams) => { - execute: ( - signal?: AbortSignal, - liveOutputCallback?: (chunk: string) => void, - ) => Promise<{ - llmContent: PartListUnion; - returnDisplay: ToolResultDisplay; - }>; - getDescription: () => string; - }; + createInvocation: (params: AgentParams) => AgentToolInvocation; }; // Mock dependencies @@ -998,4 +1011,295 @@ describe('AgentTool', () => { expect(startAgentId).toMatch(/^file-search-\d+$/); }); }); + + describe('IDE diff-tab confirmation clears pendingConfirmation', () => { + let mockAgent: AgentHeadless; + let mockContextState: ContextState; + + // We capture the eventEmitter from the invocation so we can simulate + // events during subagent execution. + let capturedInvocation: AgentToolInvocation; + + beforeEach(() => { + mockContextState = { + set: vi.fn(), + } as unknown as ContextState; + + MockedContextState.mockImplementation(() => mockContextState); + + vi.mocked(mockSubagentManager.loadSubagent).mockResolvedValue( + mockSubagents[0], + ); + }); + + function createInvocationWithEventDrivenAgent( + emitDuringExecute: (emitter: AgentEventEmitter) => void, + ) { + // Create a mock agent whose execute() emits events on the invocation's + // eventEmitter, simulating a real subagent lifecycle. + mockAgent = { + execute: vi.fn(), + result: 'Done', + terminateMode: AgentTerminateMode.GOAL, + getFinalText: vi.fn().mockReturnValue('Done'), + formatCompactResult: vi.fn().mockReturnValue('✅ Success'), + getExecutionSummary: vi.fn().mockReturnValue({ + rounds: 1, + totalDurationMs: 100, + totalToolCalls: 1, + successfulToolCalls: 1, + failedToolCalls: 0, + successRate: 100, + inputTokens: 10, + outputTokens: 5, + totalTokens: 15, + toolUsage: [], + }), + getStatistics: vi.fn().mockReturnValue({ + rounds: 1, + totalDurationMs: 100, + totalToolCalls: 1, + successfulToolCalls: 1, + failedToolCalls: 0, + }), + getTerminateMode: vi.fn().mockReturnValue(AgentTerminateMode.GOAL), + } as unknown as AgentHeadless; + + vi.mocked(mockAgent.execute).mockImplementation(async () => { + emitDuringExecute(capturedInvocation.eventEmitter); + }); + + vi.mocked(mockSubagentManager.createAgentHeadless).mockResolvedValue( + mockAgent, + ); + + const params: AgentParams = { + description: 'Edit files', + prompt: 'Fix the bug', + subagent_type: 'file-search', + }; + + capturedInvocation = ( + agentTool as AgentToolWithProtectedMethods + ).createInvocation(params); + + return capturedInvocation; + } + + it('should clear pendingConfirmation when TOOL_RESULT arrives for the pending tool (IDE accept path)', async () => { + // Track whether pendingConfirmation was set then cleared, using + // snapshots that safely handle function properties (structuredClone + // can't serialize functions). + const snapshots: Array<{ + hasPendingConfirmation: boolean; + toolStatuses: Array<{ callId: string; status: string }>; + }> = []; + + const invocation = createInvocationWithEventDrivenAgent((emitter) => { + emitter.emit(AgentEventType.TOOL_CALL, { + subagentId: 'sub-1', + round: 1, + callId: 'call-edit-1', + name: 'edit_file', + args: { path: '/test.ts' }, + description: 'Editing test.ts', + timestamp: Date.now(), + } satisfies AgentToolCallEvent); + + // Tool needs approval → pendingConfirmation is set + emitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, { + subagentId: 'sub-1', + round: 1, + callId: 'call-edit-1', + name: 'edit_file', + description: 'Editing test.ts', + timestamp: Date.now(), + confirmationDetails: { + type: 'edit' as const, + title: 'Edit file', + fileName: 'test.ts', + filePath: '/test.ts', + fileDiff: '', + originalContent: 'old', + newContent: 'new', + }, + respond: vi.fn(), + } as unknown as AgentApprovalRequestEvent); + + // IDE diff-tab accepted → TOOL_RESULT arrives without onConfirm + emitter.emit(AgentEventType.TOOL_RESULT, { + subagentId: 'sub-1', + round: 1, + callId: 'call-edit-1', + name: 'edit_file', + success: true, + timestamp: Date.now(), + } satisfies AgentToolResultEvent); + }); + + await invocation.execute(undefined, (output) => { + const display = output as AgentResultDisplay; + snapshots.push({ + hasPendingConfirmation: display.pendingConfirmation !== undefined, + toolStatuses: (display.toolCalls ?? []).map((tc) => ({ + callId: tc.callId, + status: tc.status, + })), + }); + }); + + // Should have at least one snapshot with pendingConfirmation set + const hasApproval = snapshots.some((s) => s.hasPendingConfirmation); + expect(hasApproval).toBe(true); + + // The final snapshot after TOOL_RESULT should have cleared it + const resultSnapshot = snapshots.find( + (s) => + !s.hasPendingConfirmation && + s.toolStatuses.some( + (tc) => tc.callId === 'call-edit-1' && tc.status === 'success', + ), + ); + expect(resultSnapshot).toBeDefined(); + }); + + it('should NOT clear pendingConfirmation when TOOL_RESULT is for a different tool', async () => { + const snapshots: Array<{ + hasPendingConfirmation: boolean; + toolStatuses: Array<{ callId: string; status: string }>; + }> = []; + + const invocation = createInvocationWithEventDrivenAgent((emitter) => { + // Tool A starts + emitter.emit(AgentEventType.TOOL_CALL, { + subagentId: 'sub-1', + round: 1, + callId: 'call-read-1', + name: 'read_file', + args: {}, + description: 'Reading', + timestamp: Date.now(), + } satisfies AgentToolCallEvent); + + // Tool B starts + emitter.emit(AgentEventType.TOOL_CALL, { + subagentId: 'sub-1', + round: 1, + callId: 'call-edit-1', + name: 'edit_file', + args: {}, + description: 'Editing', + timestamp: Date.now(), + } satisfies AgentToolCallEvent); + + // Tool B needs approval + emitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, { + subagentId: 'sub-1', + round: 1, + callId: 'call-edit-1', + name: 'edit_file', + description: 'Editing', + timestamp: Date.now(), + confirmationDetails: { + type: 'edit' as const, + title: 'Edit', + fileName: 'test.ts', + filePath: '/test.ts', + fileDiff: '', + originalContent: '', + newContent: 'new', + }, + respond: vi.fn(), + } as unknown as AgentApprovalRequestEvent); + + // Tool A finishes (different callId) + emitter.emit(AgentEventType.TOOL_RESULT, { + subagentId: 'sub-1', + round: 1, + callId: 'call-read-1', + name: 'read_file', + success: true, + timestamp: Date.now(), + } satisfies AgentToolResultEvent); + }); + + await invocation.execute(undefined, (output) => { + const display = output as AgentResultDisplay; + snapshots.push({ + hasPendingConfirmation: display.pendingConfirmation !== undefined, + toolStatuses: (display.toolCalls ?? []).map((tc) => ({ + callId: tc.callId, + status: tc.status, + })), + }); + }); + + // The snapshot for read_file's TOOL_RESULT should still have + // pendingConfirmation because the result was for a different tool. + const readResultSnapshot = snapshots.find((s) => + s.toolStatuses.some( + (tc) => tc.callId === 'call-read-1' && tc.status === 'success', + ), + ); + expect(readResultSnapshot).toBeDefined(); + expect(readResultSnapshot!.hasPendingConfirmation).toBe(true); + }); + + it('should clear pendingConfirmation via onConfirm callback (terminal UI path)', async () => { + let capturedOnConfirm: + | ((outcome: ToolConfirmationOutcome) => Promise) + | undefined; + const snapshots: Array<{ hasPendingConfirmation: boolean }> = []; + + const invocation = createInvocationWithEventDrivenAgent((emitter) => { + emitter.emit(AgentEventType.TOOL_CALL, { + subagentId: 'sub-1', + round: 1, + callId: 'call-edit-1', + name: 'edit_file', + args: {}, + description: 'Editing', + timestamp: Date.now(), + } satisfies AgentToolCallEvent); + + emitter.emit(AgentEventType.TOOL_WAITING_APPROVAL, { + subagentId: 'sub-1', + round: 1, + callId: 'call-edit-1', + name: 'edit_file', + description: 'Editing', + timestamp: Date.now(), + confirmationDetails: { + type: 'edit' as const, + title: 'Edit', + fileName: 'test.ts', + filePath: '/test.ts', + fileDiff: '', + originalContent: '', + newContent: 'new', + }, + respond: vi.fn(), + } as unknown as AgentApprovalRequestEvent); + }); + + await invocation.execute(undefined, (output) => { + const display = output as AgentResultDisplay; + snapshots.push({ + hasPendingConfirmation: display.pendingConfirmation !== undefined, + }); + if (display.pendingConfirmation?.onConfirm) { + capturedOnConfirm = display.pendingConfirmation.onConfirm; + } + }); + + expect(capturedOnConfirm).toBeDefined(); + + // Call onConfirm as if the user pressed "accept" in the terminal UI + snapshots.length = 0; + await capturedOnConfirm!(ToolConfirmationOutcome.ProceedOnce); + + // The onConfirm callback should have cleared pendingConfirmation + expect(snapshots.some((s) => !s.hasPendingConfirmation)).toBe(true); + }); + }); }); diff --git a/packages/core/src/tools/agent.ts b/packages/core/src/tools/agent.ts index 1b0c1c924..1f80ca37a 100644 --- a/packages/core/src/tools/agent.ts +++ b/packages/core/src/tools/agent.ts @@ -308,6 +308,8 @@ class AgentToolInvocation extends BaseToolInvocation { private setupEventListeners( updateOutput?: (output: ToolResultDisplay) => void, ): void { + let pendingConfirmationCallId: string | undefined; + this.eventEmitter.on(AgentEventType.START, () => { this.updateDisplay({ status: 'running' }, updateOutput); }); @@ -344,9 +346,22 @@ class AgentToolInvocation extends BaseToolInvocation { responseParts: event.responseParts, }; + // When a tool result arrives for the tool that had a pending + // confirmation, clear the stale prompt. This handles the case where + // the IDE diff-tab accept resolved the tool via CoreToolScheduler's + // ideConfirmation.then path, which bypasses the UI's onConfirm wrapper. + const clearPending = + pendingConfirmationCallId === event.callId + ? { pendingConfirmation: undefined } + : {}; + if (pendingConfirmationCallId === event.callId) { + pendingConfirmationCallId = undefined; + } + this.updateDisplay( { toolCalls: [...this.currentToolCalls!], + ...clearPending, }, updateOutput, ); @@ -398,6 +413,7 @@ class AgentToolInvocation extends BaseToolInvocation { } // Bridge scheduler confirmation details to UI inline prompt + pendingConfirmationCallId = event.callId; const details: ToolCallConfirmationDetails = { ...(event.confirmationDetails as Omit< ToolCallConfirmationDetails, @@ -409,6 +425,7 @@ class AgentToolInvocation extends BaseToolInvocation { ) => { // Clear the inline prompt immediately // and optimistically mark the tool as executing for proceed outcomes. + pendingConfirmationCallId = undefined; const proceedOutcomes = new Set([ ToolConfirmationOutcome.ProceedOnce, ToolConfirmationOutcome.ProceedAlways,