From 8edf410112d589980cfda8c15b69a37f908b8449 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Mon, 13 Apr 2026 15:33:19 -0400 Subject: [PATCH 1/3] fix #309589 --- .../browser/tools/sendToTerminalTool.ts | 14 ++++---- .../test/browser/sendToTerminalTool.test.ts | 32 ++++++++++++++++++- 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts index 2d82a43c624dd..cbc826ad12c8c 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts +++ b/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, diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts index 01898d97c77a8..0de8d910df2c7 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts +++ b/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,32 @@ 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'); + }); }); From 9976ed6d86588ca76207431385d6712ca48443b3 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Mon, 13 Apr 2026 15:51:01 -0400 Subject: [PATCH 2/3] safer fix --- .../browser/tools/sendToTerminalTool.ts | 13 ++-- .../test/browser/sendToTerminalTool.test.ts | 67 +++++++++++++++++++ 2 files changed, 71 insertions(+), 9 deletions(-) diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts index cbc826ad12c8c..f6ace5a557128 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts @@ -262,12 +262,10 @@ export class SendToTerminalTool extends Disposable implements IToolImpl { continue; } - // If there's only one question, return it directly - if (carousel.questions.length === 1) { - return this._getQuestionText(carousel.questions[0]); - } - - // Multiple questions: match the command text against submitted answers + // Match the command text against submitted answers; + // only return a question when the command exactly matches + // an answer so that unrelated send_to_terminal calls do + // not accidentally skip confirmation. if (carousel.data) { for (const question of carousel.questions) { const answer = carousel.data[question.id]; @@ -276,9 +274,6 @@ export class SendToTerminalTool extends Disposable implements IToolImpl { } } } - - // Fallback: return the first question's text - return this._getQuestionText(carousel.questions[0]); } } } diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts index 0de8d910df2c7..f2c8b226cf43c 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts @@ -207,4 +207,71 @@ suite('SendToTerminalTool', () => { 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 mockSession = { + getRequests: () => [{ + response: { + response: { + value: [{ + 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' }, + }] + } + } + }], + }; + 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'); + }); }); From 154a0fbcf3c3f380efebe7184cef6c80b22b0930 Mon Sep 17 00:00:00 2001 From: meganrogge Date: Mon, 13 Apr 2026 16:05:43 -0400 Subject: [PATCH 3/3] improve reliability --- .../browser/tools/sendToTerminalTool.ts | 73 +++++++++----- .../test/browser/sendToTerminalTool.test.ts | 95 +++++++++++++++++-- 2 files changed, 135 insertions(+), 33 deletions(-) diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts index f6ace5a557128..99a9e6c634443 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/sendToTerminalTool.ts @@ -214,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) { @@ -247,35 +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; } + } + } - // Match the command text against submitted answers; - // only return a question when the command exactly matches - // an answer so that unrelated send_to_terminal calls do - // not accidentally skip confirmation. - if (carousel.data) { - for (const question of carousel.questions) { - const answer = carousel.data[question.id]; - if (this._answerMatchesCommand(answer, commandText)) { - return this._getQuestionText(question); - } - } - } + if (!carousel || carouselIndex === -1) { + continue; + } + + // 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; + } + + 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; } diff --git a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts index f2c8b226cf43c..93dd7ccfa2bc4 100644 --- a/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts +++ b/src/vs/workbench/contrib/terminalContrib/chatAgentTools/test/browser/sendToTerminalTool.test.ts @@ -238,19 +238,26 @@ suite('SendToTerminalTool', () => { 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: [{ - 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' }, - }] + value: [carousel, priorSendInvocation] } } }], @@ -274,4 +281,74 @@ suite('SendToTerminalTool', () => { 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'); + }); });