fix(acp-integration/agent): clear pendingConfirmation when tool result arrives for pending tool

- Track pendingConfirmationCallId in AgentToolInvocation to properly clear stale prompts
- Clear pendingConfirmation when TOOL_RESULT arrives for the pending tool (IDE diff-tab path)
- Clear pendingConfirmation via onConfirm callback (terminal UI path)
- Ensure pendingConfirmation is NOT cleared when TOOL_RESULT is for a different tool
- Prefer filePath over fileName for diff content path in Session and SubAgentTracker
- Add comprehensive tests for IDE diff-tab and terminal UI confirmation flows

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This commit is contained in:
yiliang114 2026-03-23 23:49:22 +08:00
parent 38caa0b218
commit ee1f98f4ff
5 changed files with 414 additions and 12 deletions

View file

@ -623,7 +623,7 @@ export class Session implements SessionContext {
if (confirmationDetails.type === 'edit') { if (confirmationDetails.type === 'edit') {
content.push({ content.push({
type: 'diff', type: 'diff',
path: confirmationDetails.fileName, path: confirmationDetails.filePath || confirmationDetails.fileName,
oldText: confirmationDetails.originalContent, oldText: confirmationDetails.originalContent,
newText: confirmationDetails.newContent, newText: confirmationDetails.newContent,
}); });

View file

@ -531,6 +531,86 @@ describe('SubAgentTracker', () => {
expect(respondSpy).toHaveBeenCalledWith(ToolConfirmationOutcome.Cancel); 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<ToolEditConfirmationDetails, 'onConfirm'>,
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', () => { describe('permission options', () => {

View file

@ -226,12 +226,13 @@ export class SubAgentTracker {
const editDetails = event.confirmationDetails as unknown as { const editDetails = event.confirmationDetails as unknown as {
type: 'edit'; type: 'edit';
fileName: string; fileName: string;
filePath: string;
originalContent: string | null; originalContent: string | null;
newContent: string; newContent: string;
}; };
content.push({ content.push({
type: 'diff', type: 'diff',
path: editDetails.fileName, path: editDetails.filePath || editDetails.fileName,
oldText: editDetails.originalContent ?? '', oldText: editDetails.originalContent ?? '',
newText: editDetails.newContent, newText: editDetails.newContent,
}); });

View file

@ -8,6 +8,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import { AgentTool, type AgentParams } from './agent.js'; import { AgentTool, type AgentParams } from './agent.js';
import type { PartListUnion } from '@google/genai'; import type { PartListUnion } from '@google/genai';
import type { ToolResultDisplay, AgentResultDisplay } from './tools.js'; import type { ToolResultDisplay, AgentResultDisplay } from './tools.js';
import { ToolConfirmationOutcome } from './tools.js';
import type { Config } from '../config/config.js'; import type { Config } from '../config/config.js';
import { SubagentManager } from '../subagents/subagent-manager.js'; import { SubagentManager } from '../subagents/subagent-manager.js';
import type { SubagentConfig } from '../subagents/types.js'; import type { SubagentConfig } from '../subagents/types.js';
@ -16,22 +17,34 @@ import {
type AgentHeadless, type AgentHeadless,
ContextState, ContextState,
} from '../agents/runtime/agent-headless.js'; } 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 { partToString } from '../utils/partUtils.js';
import type { HookSystem } from '../hooks/hookSystem.js'; import type { HookSystem } from '../hooks/hookSystem.js';
import { PermissionMode } from '../hooks/types.js'; import { PermissionMode } from '../hooks/types.js';
// Type for accessing protected methods in tests // Type for accessing protected methods in tests
type AgentToolWithProtectedMethods = AgentTool & { type AgentToolInvocation = {
createInvocation: (params: AgentParams) => {
execute: ( execute: (
signal?: AbortSignal, signal?: AbortSignal,
liveOutputCallback?: (chunk: string) => void, updateOutput?: (output: ToolResultDisplay) => void,
) => Promise<{ ) => Promise<{
llmContent: PartListUnion; llmContent: PartListUnion;
returnDisplay: ToolResultDisplay; returnDisplay: ToolResultDisplay;
}>; }>;
getDescription: () => string; getDescription: () => string;
}; eventEmitter: AgentEventEmitter;
};
type AgentToolWithProtectedMethods = AgentTool & {
createInvocation: (params: AgentParams) => AgentToolInvocation;
}; };
// Mock dependencies // Mock dependencies
@ -998,4 +1011,295 @@ describe('AgentTool', () => {
expect(startAgentId).toMatch(/^file-search-\d+$/); 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<void>)
| 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);
});
});
}); });

View file

@ -308,6 +308,8 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
private setupEventListeners( private setupEventListeners(
updateOutput?: (output: ToolResultDisplay) => void, updateOutput?: (output: ToolResultDisplay) => void,
): void { ): void {
let pendingConfirmationCallId: string | undefined;
this.eventEmitter.on(AgentEventType.START, () => { this.eventEmitter.on(AgentEventType.START, () => {
this.updateDisplay({ status: 'running' }, updateOutput); this.updateDisplay({ status: 'running' }, updateOutput);
}); });
@ -344,9 +346,22 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
responseParts: event.responseParts, 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( this.updateDisplay(
{ {
toolCalls: [...this.currentToolCalls!], toolCalls: [...this.currentToolCalls!],
...clearPending,
}, },
updateOutput, updateOutput,
); );
@ -398,6 +413,7 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
} }
// Bridge scheduler confirmation details to UI inline prompt // Bridge scheduler confirmation details to UI inline prompt
pendingConfirmationCallId = event.callId;
const details: ToolCallConfirmationDetails = { const details: ToolCallConfirmationDetails = {
...(event.confirmationDetails as Omit< ...(event.confirmationDetails as Omit<
ToolCallConfirmationDetails, ToolCallConfirmationDetails,
@ -409,6 +425,7 @@ class AgentToolInvocation extends BaseToolInvocation<AgentParams, ToolResult> {
) => { ) => {
// Clear the inline prompt immediately // Clear the inline prompt immediately
// and optimistically mark the tool as executing for proceed outcomes. // and optimistically mark the tool as executing for proceed outcomes.
pendingConfirmationCallId = undefined;
const proceedOutcomes = new Set<ToolConfirmationOutcome>([ const proceedOutcomes = new Set<ToolConfirmationOutcome>([
ToolConfirmationOutcome.ProceedOnce, ToolConfirmationOutcome.ProceedOnce,
ToolConfirmationOutcome.ProceedAlways, ToolConfirmationOutcome.ProceedAlways,