fix(chat): Adding conditional check for Read and List tool execution (aws#7026)

laileni-aws · web-flow · commit 741c2c481bcf · 2025-04-14T17:49:35.000-07:00
## Problem - Reading Tool and List Directory tool requires users acceptance if IDE wants to perform operation outside of workspace. ## Solution - This problem is fixed as part of this PR ![image](https://github.com/user-attachments/assets/ad2811a6-7dca-4f21-b1f5-4c7809ec58e9) --- - Treat all work as PUBLIC. Private `feature/x` branches will not be squash-merged at release time. - Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). - License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
diff --git a/packages/core/package.nls.json b/packages/core/package.nls.json
@@ -459,6 +459,7 @@
     "AWS.amazonq.executeBash.reject": "Reject",
     "AWS.amazonq.chat.directive.pairProgrammingModeOn": "You are using **pair programming mode**: Q can now list files, preview code diffs and allow you to run shell commands.",
     "AWS.amazonq.chat.directive.pairProgrammingModeOff": "You turned off **pair programming mode**. Q will not include code diffs or run commands in the chat.",
+    "AWS.amazonq.chat.directive.permission.readAndList": "I need permission to read files and list directories outside the workspace.",
     "AWS.amazonq.chat.directive.runCommandToProceed": "Run the command to proceed.",
     "AWS.toolkit.lambda.walkthrough.quickpickTitle": "Application Builder Walkthrough",
     "AWS.toolkit.lambda.walkthrough.title": "Get started building your application",
diff --git a/packages/core/src/codewhispererChat/controllers/chat/messenger/messenger.ts b/packages/core/src/codewhispererChat/controllers/chat/messenger/messenger.ts
@@ -376,7 +376,11 @@ export class Messenger {
                                     changeList,
                                     explanation
                                 )
-                                await ToolUtils.queueDescription(tool, chatStream)
+                                await ToolUtils.queueDescription(
+                                    tool,
+                                    chatStream,
+                                    chatStream.validation.requiresAcceptance
+                                )
                                 if (session.messageIdToUpdate === undefined && tool.type === ToolType.FsRead) {
                                     // Store the first messageId in a chain of tool uses
                                     session.setMessageIdToUpdate(toolUse.toolUseId)
@@ -748,13 +752,17 @@ export class Messenger {
         }
 
         // Handle read tool and list directory messages
-        if (toolUse?.name === ToolType.FsRead || toolUse?.name === ToolType.ListDirectory) {
+        if (
+            (toolUse?.name === ToolType.FsRead || toolUse?.name === ToolType.ListDirectory) &&
+            !validation.requiresAcceptance
+        ) {
             return this.sendReadAndListDirToolMessage(toolUse, session, tabID, triggerID, messageIdToUpdate)
         }
 
         // Handle file write tool, execute bash tool and bash command output log messages
         const buttons: ChatItemButton[] = []
         let header: ChatItemHeader | undefined = undefined
+        let messageID: string = toolUse?.toolUseId ?? ''
         if (toolUse?.name === ToolType.ExecuteBash && message.startsWith('```shell')) {
             if (validation.requiresAcceptance) {
                 const buttons: ChatItemButton[] = [
@@ -825,6 +833,10 @@ export class Messenger {
             }
         } else if (toolUse?.name === ToolType.ListDirectory || toolUse?.name === ToolType.FsRead) {
             if (validation.requiresAcceptance) {
+                /** For Read and List Directory tools
+                 * requiredAcceptance = false, we use messageID = toolID and we keep on updating this messageID
+                 * requiredAcceptance = true, IDE sends messageID != toolID, some default value, as this overlaps with previous message. */
+                messageID = 'toolUse'
                 const buttons: ChatItemButton[] = [
                     {
                         id: 'confirm-tool-use',
@@ -840,7 +852,6 @@ export class Messenger {
                     },
                 ]
                 header = {
-                    body: 'shell',
                     buttons,
                 }
             }
@@ -859,7 +870,7 @@ export class Messenger {
                     followUpsHeader: undefined,
                     relatedSuggestions: undefined,
                     triggerID,
-                    messageID: toolUse?.toolUseId ?? '',
+                    messageID,
                     userIntent: undefined,
                     codeBlockLanguage: undefined,
                     contextList: undefined,
diff --git a/packages/core/src/codewhispererChat/tools/chatStream.ts b/packages/core/src/codewhispererChat/tools/chatStream.ts
@@ -29,7 +29,7 @@ export class ChatStream extends Writable {
         private readonly messageIdToUpdate: string | undefined,
         // emitEvent decides to show the streaming message or read/list directory tool message to the user.
         private readonly emitEvent: boolean,
-        private readonly validation: CommandValidation,
+        readonly validation: CommandValidation,
         private readonly isReadorList: boolean,
         private readonly changeList?: Change[],
         private readonly explanation?: string,
@@ -45,7 +45,13 @@ export class ChatStream extends Writable {
         if (this.explanation) {
             this.messenger.sendDirectiveMessage(tabID, triggerID, this.explanation)
         }
-        if (validation.requiresAcceptance && this.toolUse?.name === 'executeBash') {
+        if (validation.requiresAcceptance && isReadorList) {
+            this.messenger.sendDirectiveMessage(
+                tabID,
+                triggerID,
+                i18n('AWS.amazonq.chat.directive.permission.readAndList')
+            )
+        } else if (validation.requiresAcceptance && this.toolUse?.name === 'executeBash') {
             this.messenger.sendDirectiveMessage(
                 tabID,
                 triggerID,
diff --git a/packages/core/src/codewhispererChat/tools/fsRead.ts b/packages/core/src/codewhispererChat/tools/fsRead.ts
@@ -8,6 +8,7 @@ import fs from '../../shared/fs/fs'
 import { Writable } from 'stream'
 import { InvokeOutput, OutputKind, sanitizePath, CommandValidation, fsReadToolResponseSize } from './toolShared'
 import { isInDirectory } from '../../shared/filesystemUtilities'
+import path from 'path'
 
 export interface FsReadParams {
     path: string
@@ -47,8 +48,28 @@ export class FsRead {
         this.logger.debug(`Validation succeeded for path: ${this.fsPath}`)
     }
 
-    public queueDescription(updates: Writable): void {
-        updates.write('')
+    public queueDescription(updates: Writable, requiresAcceptance: boolean): void {
+        if (requiresAcceptance) {
+            const fileName = path.basename(this.fsPath)
+            const fileUri = vscode.Uri.file(this.fsPath)
+            updates.write(`Reading file: [${fileName}](${fileUri}), `)
+
+            const [start, end] = this.readRange ?? []
+
+            if (start && end) {
+                updates.write(`from line ${start} to ${end}`)
+            } else if (start) {
+                if (start > 0) {
+                    updates.write(`from line ${start} to end of file`)
+                } else {
+                    updates.write(`${start} line from the end of file to end of file`)
+                }
+            } else {
+                updates.write('all lines')
+            }
+        } else {
+            updates.write('')
+        }
         updates.end()
     }
 
diff --git a/packages/core/src/codewhispererChat/tools/listDirectory.ts b/packages/core/src/codewhispererChat/tools/listDirectory.ts
@@ -49,16 +49,21 @@ export class ListDirectory {
         }
     }
 
-    public queueDescription(updates: Writable): void {
-        const fileName = path.basename(this.fsPath)
-        if (this.maxDepth === undefined) {
-            updates.write(`Analyzing directories recursively: ${fileName}`)
-        } else if (this.maxDepth === 0) {
-            updates.write(`Analyzing directory: ${fileName}`)
+    public queueDescription(updates: Writable, requiresAcceptance: boolean): void {
+        if (requiresAcceptance) {
+            const fileName = path.basename(this.fsPath)
+            if (this.maxDepth === undefined) {
+                updates.write(`Analyzing directories recursively: ${fileName}`)
+            } else if (this.maxDepth === 0) {
+                updates.write(`Analyzing directory: ${fileName}`)
+            } else {
+                const level = this.maxDepth > 1 ? 'levels' : 'level'
+                updates.write(`Analyzing directory: ${fileName} limited to ${this.maxDepth} subfolder ${level}`)
+            }
         } else {
-            const level = this.maxDepth > 1 ? 'levels' : 'level'
-            updates.write(`Analyzing directory: ${fileName} limited to ${this.maxDepth} subfolder ${level}`)
+            updates.write('')
         }
+
         updates.end()
     }
 
diff --git a/packages/core/src/codewhispererChat/tools/toolUtils.ts b/packages/core/src/codewhispererChat/tools/toolUtils.ts
@@ -2,6 +2,7 @@
  * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
  * SPDX-License-Identifier: Apache-2.0
  */
+import * as vscode from 'vscode'
 import { Writable } from 'stream'
 import { FsRead, FsReadParams } from './fsRead'
 import { FsWrite, FsWriteParams } from './fsWrite'
@@ -14,7 +15,6 @@ import {
     fsReadToolResponseSize,
 } from './toolShared'
 import { ListDirectory, ListDirectoryParams } from './listDirectory'
-import * as vscode from 'vscode'
 
 export enum ToolType {
     FsRead = 'fsRead',
@@ -95,10 +95,10 @@ export class ToolUtils {
         }
     }
 
-    static async queueDescription(tool: Tool, updates: Writable): Promise<void> {
+    static async queueDescription(tool: Tool, updates: Writable, requiresAcceptance: boolean): Promise<void> {
         switch (tool.type) {
             case ToolType.FsRead:
-                tool.tool.queueDescription(updates)
+                tool.tool.queueDescription(updates, requiresAcceptance)
                 break
             case ToolType.FsWrite:
                 await tool.tool.queueDescription(updates)
@@ -107,7 +107,7 @@ export class ToolUtils {
                 tool.tool.queueDescription(updates)
                 break
             case ToolType.ListDirectory:
-                tool.tool.queueDescription(updates)
+                tool.tool.queueDescription(updates, requiresAcceptance)
                 break
         }
     }
diff --git a/packages/core/src/test/codewhispererChat/tools/toolShared.test.ts b/packages/core/src/test/codewhispererChat/tools/toolShared.test.ts
@@ -234,30 +234,30 @@ describe('ToolUtils', function () {
         // TODO: Adding "void" to the following tests for the current implementation but in the next followup PR I will fix this issue.
         it('delegates to FsRead tool queueDescription method', function () {
             const tool: Tool = { type: ToolType.FsRead, tool: mockFsRead as unknown as FsRead }
-            void ToolUtils.queueDescription(tool, mockWritable as unknown as Writable)
+            void ToolUtils.queueDescription(tool, mockWritable as unknown as Writable, false)
 
-            assert(mockFsRead.queueDescription.calledOnceWith(mockWritable))
+            assert(mockFsRead.queueDescription.calledOnceWith(mockWritable, false))
         })
 
         it('delegates to FsWrite tool queueDescription method', function () {
             const tool: Tool = { type: ToolType.FsWrite, tool: mockFsWrite as unknown as FsWrite }
-            void ToolUtils.queueDescription(tool, mockWritable as unknown as Writable)
+            void ToolUtils.queueDescription(tool, mockWritable as unknown as Writable, false)
 
             assert(mockFsWrite.queueDescription.calledOnceWith(mockWritable))
         })
 
         it('delegates to ExecuteBash tool queueDescription method', function () {
             const tool: Tool = { type: ToolType.ExecuteBash, tool: mockExecuteBash as unknown as ExecuteBash }
-            void ToolUtils.queueDescription(tool, mockWritable as unknown as Writable)
+            void ToolUtils.queueDescription(tool, mockWritable as unknown as Writable, false)
 
             assert(mockExecuteBash.queueDescription.calledOnceWith(mockWritable))
         })
 
         it('delegates to ListDirectory tool queueDescription method', function () {
             const tool: Tool = { type: ToolType.ListDirectory, tool: mockListDirectory as unknown as ListDirectory }
-            void ToolUtils.queueDescription(tool, mockWritable as unknown as Writable)
+            void ToolUtils.queueDescription(tool, mockWritable as unknown as Writable, false)
 
-            assert(mockListDirectory.queueDescription.calledOnceWith(mockWritable))
+            assert(mockListDirectory.queueDescription.calledOnceWith(mockWritable, false))
         })
     })
 
