Skip redundant send_to_terminal confirmation when replying to askQuestions carousel (#309598)

Author: meganrogge

src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts

@@ -153,12 +153,14 @@ export class SendToTerminalTool extends Disposable implements IToolImpl {
 		const chatSessionResource = context.chatSessionResource;
 		const isSessionAutoApproved = chatSessionResource && isSessionAutoApproveLevel(chatSessionResource, this._configurationService, this._chatWidgetService, this._chatService);
 
-		// send_to_terminal always requires confirmation in default approvals mode.
-		// Unlike run_in_terminal, the text sent here may be arbitrary input to a
-		// waiting prompt (e.g. a name, password, or confirmation) rather than a
-		// shell command, so the command-line auto-approve analyzer cannot reliably
-		// determine safety.
-		const shouldShowConfirmation = !isSessionAutoApproved || context.forceConfirmationReason !== undefined;
+		// send_to_terminal normally requires confirmation in default approvals mode
+		// because the text may be arbitrary input (passwords, confirmations, etc.)
+		// that the command-line auto-approve analyzer cannot assess. However, when
+		// the text being sent was just collected via askQuestions for the same
+		// terminal, the user already explicitly provided the answer so a second
+		// confirmation is redundant.
+		const isAnsweringQuestion = questionText !== undefined;
+		const shouldShowConfirmation = (!isSessionAutoApproved && !isAnsweringQuestion) || context.forceConfirmationReason !== undefined;
 		const confirmationMessages = shouldShowConfirmation ? {
 			title: localize('send.confirm.title', "Send to Terminal"),
 			message: confirmationMessage,
@@ -212,13 +214,15 @@ export class SendToTerminalTool extends Disposable implements IToolImpl {
 
 	/**
 	 * Searches the current session's responses for the most recent question
-	 * carousel associated with the target terminal, then matches the command
-	 * text being sent against the carousel's submitted answers to return the
-	 * specific question that this send_to_terminal call is answering.
+	 * carousel associated with the target terminal, then uses positional
+	 * matching to return the specific question that this send_to_terminal
+	 * call is answering.
 	 *
 	 * When a carousel contains multiple questions, the model calls
-	 * send_to_terminal once per answer. This method correlates each call to
-	 * the right question by matching the sent text against answer values.
+	 * send_to_terminal once per answer in order. This method counts prior
+	 * send_to_terminal invocations since the carousel to determine the
+	 * current question index, then verifies the command matches the answer
+	 * at that position.
 	 */
 	private _getQuestionContextForTerminal(chatSessionResource: URI | undefined, args: ISendToTerminalInputParams): string | undefined {
 		if (!chatSessionResource) {
@@ -245,40 +249,58 @@ export class SendToTerminalTool extends Disposable implements IToolImpl {
 				continue;
 			}
 			const parts = response.response.value;
+
+			// First, find the carousel for this terminal (searching backwards)
+			let carouselIndex = -1;
+			let carousel: IChatQuestionCarousel | undefined;
 			for (let j = parts.length - 1; j >= 0; j--) {
 				const part = parts[j];
 				if (part.kind === 'questionCarousel') {
-					const carousel = part as IChatQuestionCarousel;
-					if (!carousel.terminalId || carousel.questions.length === 0) {
+					const candidate = part as IChatQuestionCarousel;
+					if (!candidate.terminalId || candidate.questions.length === 0) {
 						continue;
 					}
-					// Match by execution UUID or by resolving the carousel's UUID to an instance ID
-					const matchesById = !!args.id && carousel.terminalId === args.id;
+					const matchesById = !!args.id && candidate.terminalId === args.id;
 					const matchesByInstanceId = args.terminalId !== undefined &&
-						RunInTerminalTool.getExecution(carousel.terminalId)?.instance.instanceId === args.terminalId;
-					if (!matchesById && !matchesByInstanceId) {
-						continue;
+						RunInTerminalTool.getExecution(candidate.terminalId)?.instance.instanceId === args.terminalId;
+					if (matchesById || matchesByInstanceId) {
+						carouselIndex = j;
+						carousel = candidate;
+						break;
 					}
+				}
+			}
 
-					// If there's only one question, return it directly
-					if (carousel.questions.length === 1) {
-						return this._getQuestionText(carousel.questions[0]);
-					}
+			if (!carousel || carouselIndex === -1) {
+				continue;
+			}
 
-					// Multiple questions: match the command text against submitted answers
-					if (carousel.data) {
-						for (const question of carousel.questions) {
-							const answer = carousel.data[question.id];
-							if (this._answerMatchesCommand(answer, commandText)) {
-								return this._getQuestionText(question);
-							}
-						}
-					}
+			// Count send_to_terminal tool invocations after the carousel to
+			// determine which question this call corresponds to (positional).
+			let sendCount = 0;
+			for (let j = carouselIndex + 1; j < parts.length; j++) {
+				if (parts[j].kind === 'toolInvocation' && (parts[j] as { toolId?: string }).toolId === TerminalToolId.SendToTerminal) {
+					sendCount++;
+				}
+			}
+
+			const questionIndex = sendCount;
+			if (questionIndex >= carousel.questions.length) {
+				return undefined;
+			}
 
-					// Fallback: return the first question's text
-					return this._getQuestionText(carousel.questions[0]);
+			const question = carousel.questions[questionIndex];
+
+			// Verify the command matches the answer at this position so that
+			// unrelated send_to_terminal calls don't skip confirmation.
+			if (carousel.data) {
+				const answer = carousel.data[question.id];
+				if (this._answerMatchesCommand(answer, commandText)) {
+					return this._getQuestionText(question);
 				}
 			}
+
+			return undefined;
 		}
 		return undefined;
 	}

src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts

@@ -16,6 +16,7 @@ import { ITerminalChatService, type ITerminalInstance } from '../../../../termin
 import { workbenchInstantiationService } from '../../../../../test/browser/workbenchTestServices.js';
 import type { TestInstantiationService } from '../../../../../../platform/instantiation/test/common/instantiationServiceMock.js';
 import { IChatService } from '../../../../chat/common/chatService/chatService.js';
+import { URI } from '../../../../../../base/common/uri.js';
 
 suite('SendToTerminalTool', () => {
 	const store = ensureNoDisposablesAreLeakedInTestSuite();
@@ -134,10 +135,11 @@ suite('SendToTerminalTool', () => {
 		assert.strictEqual(mockExecution.sentTexts[0].shouldExecute, true);
 	});
 
-	function createPreparationContext(id: string, command: string): IToolInvocationPreparationContext {
+	function createPreparationContext(id: string, command: string, chatSessionResource?: URI): IToolInvocationPreparationContext {
 		return {
 			parameters: { id, command },
 			toolCallId: 'test-call',
+			chatSessionResource,
 		} as unknown as IToolInvocationPreparationContext;
 	}
 
@@ -177,4 +179,176 @@ suite('SendToTerminalTool', () => {
 		const message = prepared.invocationMessage as IMarkdownString;
 		assert.ok(!message.value.includes('\n'), 'newlines should be collapsed to spaces');
 	});
+
+	test('prepareToolInvocation skips confirmation when answering a question carousel', async () => {
+		const sessionResource = URI.parse('chat-session://test-session');
+		const mockSession = {
+			getRequests: () => [{
+				response: {
+					response: {
+						value: [{
+							kind: 'questionCarousel' as const,
+							terminalId: KNOWN_TERMINAL_ID,
+							questions: [{ id: 'q1', title: 'package name?', message: 'package name?' }],
+							data: { q1: 'my-package' },
+						}]
+					}
+				}
+			}],
+		};
+		instantiationService.stub(IChatService, 'getSession', () => mockSession);
+		tool = store.add(instantiationService.createInstance(SendToTerminalTool));
+
+		const prepared = await tool.prepareToolInvocation(
+			createPreparationContext(KNOWN_TERMINAL_ID, 'my-package', sessionResource),
+			CancellationToken.None,
+		);
+
+		assert.ok(prepared);
+		assert.strictEqual(prepared.confirmationMessages, undefined, 'should skip confirmation when the command matches a carousel answer');
+	});
+
+	test('prepareToolInvocation does not skip confirmation when the command does not match a carousel answer', async () => {
+		const sessionResource = URI.parse('chat-session://test-session');
+		const mockSession = {
+			getRequests: () => [{
+				response: {
+					response: {
+						value: [{
+							kind: 'questionCarousel' as const,
+							terminalId: KNOWN_TERMINAL_ID,
+							questions: [{ id: 'q1', title: 'package name?', message: 'package name?' }],
+							data: { q1: 'my-package' },
+						}]
+					}
+				}
+			}],
+		};
+		instantiationService.stub(IChatService, 'getSession', () => mockSession);
+		tool = store.add(instantiationService.createInstance(SendToTerminalTool));
+
+		const prepared = await tool.prepareToolInvocation(
+			createPreparationContext(KNOWN_TERMINAL_ID, 'different-package', sessionResource),
+			CancellationToken.None,
+		);
+
+		assert.ok(prepared);
+		assert.ok(prepared.confirmationMessages, 'should require confirmation when the command does not match a carousel answer');
+	});
+
+	test('prepareToolInvocation skips confirmation only for exact matches in multi-question carousels', async () => {
+		const sessionResource = URI.parse('chat-session://test-session');
+		const carousel = {
+			kind: 'questionCarousel' as const,
+			terminalId: KNOWN_TERMINAL_ID,
+			questions: [
+				{ id: 'q1', title: 'package name?', message: 'package name?' },
+				{ id: 'q2', title: 'entry point?', message: 'entry point?' }
+			],
+			data: { q1: 'my-package', q2: 'src/index.ts' },
+		};
+		// Simulate one prior send_to_terminal invocation after the carousel
+		// so that positional matching targets question[1] (entry point)
+		const priorSendInvocation = {
+			kind: 'toolInvocation' as const,
+			toolId: 'send_to_terminal',
+		};
+		const mockSession = {
+			getRequests: () => [{
+				response: {
+					response: {
+						value: [carousel, priorSendInvocation]
+					}
+				}
+			}],
+		};
+		instantiationService.stub(IChatService, 'getSession', () => mockSession);
+		tool = store.add(instantiationService.createInstance(SendToTerminalTool));
+
+		const exactMatchPrepared = await tool.prepareToolInvocation(
+			createPreparationContext(KNOWN_TERMINAL_ID, 'src/index.ts', sessionResource),
+			CancellationToken.None,
+		);
+
+		assert.ok(exactMatchPrepared);
+		assert.strictEqual(exactMatchPrepared.confirmationMessages, undefined, 'should skip confirmation when the command exactly matches a carousel answer');
+
+		const mismatchedPrepared = await tool.prepareToolInvocation(
+			createPreparationContext(KNOWN_TERMINAL_ID, 'src/index.js', sessionResource),
+			CancellationToken.None,
+		);
+
+		assert.ok(mismatchedPrepared);
+		assert.ok(mismatchedPrepared.confirmationMessages, 'should require confirmation when the command does not exactly match any carousel answer');
+	});
+
+	test('prepareToolInvocation uses positional matching for identical answers (all defaults)', async () => {
+		const sessionResource = URI.parse('chat-session://test-session');
+		const carousel = {
+			kind: 'questionCarousel' as const,
+			terminalId: KNOWN_TERMINAL_ID,
+			questions: [
+				{ id: 'q1', title: 'package name?', message: 'package name?' },
+				{ id: 'q2', title: 'version?', message: 'version?' },
+				{ id: 'q3', title: 'description?', message: 'description?' },
+			],
+			data: { q1: '', q2: '', q3: '' },
+		};
+
+		// First call: no prior send_to_terminal → positional index 0 → "package name?"
+		const mockSession0 = {
+			getRequests: () => [{
+				response: { response: { value: [carousel] } }
+			}],
+		};
+		instantiationService.stub(IChatService, 'getSession', () => mockSession0);
+		tool = store.add(instantiationService.createInstance(SendToTerminalTool));
+
+		const first = await tool.prepareToolInvocation(
+			createPreparationContext(KNOWN_TERMINAL_ID, '', sessionResource),
+			CancellationToken.None,
+		);
+		assert.ok(first);
+		assert.strictEqual(first.confirmationMessages, undefined);
+		const firstMsg = first.pastTenseMessage as IMarkdownString;
+		assert.ok(firstMsg.value.includes('package'), 'first call should show package name question');
+
+		// Second call: one prior send_to_terminal → positional index 1 → "version?"
+		const priorSend1 = { kind: 'toolInvocation' as const, toolId: 'send_to_terminal' };
+		const mockSession1 = {
+			getRequests: () => [{
+				response: { response: { value: [carousel, priorSend1] } }
+			}],
+		};
+		instantiationService.stub(IChatService, 'getSession', () => mockSession1);
+		tool = store.add(instantiationService.createInstance(SendToTerminalTool));
+
+		const second = await tool.prepareToolInvocation(
+			createPreparationContext(KNOWN_TERMINAL_ID, '', sessionResource),
+			CancellationToken.None,
+		);
+		assert.ok(second);
+		assert.strictEqual(second.confirmationMessages, undefined);
+		const secondMsg = second.pastTenseMessage as IMarkdownString;
+		assert.ok(secondMsg.value.includes('version'), 'second call should show version question');
+
+		// Third call: two prior send_to_terminal → positional index 2 → "description?"
+		const priorSend2 = { kind: 'toolInvocation' as const, toolId: 'send_to_terminal' };
+		const mockSession2 = {
+			getRequests: () => [{
+				response: { response: { value: [carousel, priorSend1, priorSend2] } }
+			}],
+		};
+		instantiationService.stub(IChatService, 'getSession', () => mockSession2);
+		tool = store.add(instantiationService.createInstance(SendToTerminalTool));
+
+		const third = await tool.prepareToolInvocation(
+			createPreparationContext(KNOWN_TERMINAL_ID, '', sessionResource),
+			CancellationToken.None,
+		);
+		assert.ok(third);
+		assert.strictEqual(third.confirmationMessages, undefined);
+		const thirdMsg = third.pastTenseMessage as IMarkdownString;
+		assert.ok(thirdMsg.value.includes('description'), 'third call should show description question');
+	});
 });