feat: add proper windows support for executeBash and remove mocks in tests. (#934)

* test: avoid mocks in tests

* feat: switch to posix version

* test: switch back to which

* test: implement defaulting strategy for windows

* refactor: remove fallback on non-windows

* docs: update error message to reflect command that is run

Author: Hweinstock

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/executeBash.test.ts

@@ -1,41 +1,26 @@
-/*!
- * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
- * SPDX-License-Identifier: Apache-2.0
- */
-
 import { strict as assert } from 'assert'
+import * as mockfs from 'mock-fs'
 import * as sinon from 'sinon'
 import { ExecuteBash } from './executeBash'
-import { processUtils } from '@aws/lsp-core'
 import { Logging } from '@aws/language-server-runtimes/server-interface'
 import { TestFeatures } from '@aws/language-server-runtimes/testing'
 
 describe('ExecuteBash Tool', () => {
-    let runStub: sinon.SinonStub
-    let invokeStub: sinon.SinonStub
     let logging: Logging
 
     before(function () {
         logging = new TestFeatures().logging
     })
 
     beforeEach(() => {
-        runStub = sinon.stub(processUtils.ChildProcess.prototype, 'run')
-        invokeStub = sinon.stub(ExecuteBash.prototype, 'invoke')
+        mockfs.restore()
     })
 
     afterEach(() => {
         sinon.restore()
     })
 
     it('pass validation for a safe command (read-only)', async () => {
-        runStub.resolves({
-            exitCode: 0,
-            stdout: '/bin/ls',
-            stderr: '',
-            error: undefined,
-            signal: undefined,
-        })
         const execBash = new ExecuteBash(logging)
         await execBash.validate(logging, 'ls')
     })
@@ -62,14 +47,6 @@ describe('ExecuteBash Tool', () => {
     })
 
     it('whichCommand cannot find the first arg', async () => {
-        runStub.resolves({
-            exitCode: 1,
-            stdout: '',
-            stderr: '',
-            error: undefined,
-            signal: undefined,
-        })
-
         const execBash = new ExecuteBash(logging)
         await assert.rejects(
             execBash.validate(logging, 'noSuchCmd'),
@@ -78,46 +55,18 @@ describe('ExecuteBash Tool', () => {
         )
     })
 
-    it('whichCommand sees first arg on PATH', async () => {
-        runStub.resolves({
-            exitCode: 0,
-            stdout: '/usr/bin/noSuchCmd\n',
-            stderr: '',
-            error: undefined,
-            signal: undefined,
-        })
-
+    it('validate and invokes the command', async () => {
         const execBash = new ExecuteBash(logging)
-        await execBash.validate(logging, 'noSuchCmd')
-    })
 
-    it('stub invoke() call', async () => {
-        invokeStub.resolves({
-            output: {
-                kind: 'json',
-                content: {
-                    exitStatus: '0',
-                    stdout: 'mocked stdout lines',
-                    stderr: '',
-                },
-            },
-        })
-
-        const execBash = new ExecuteBash(logging)
-
-        const dummyWritable = { write: () => {} } as any
-        const result = await execBash.invoke(dummyWritable)
+        const writable = new WritableStream()
+        const result = await execBash.invoke({ command: 'ls' }, writable)
 
         assert.strictEqual(result.output.kind, 'json')
-        const out = result.output.content as unknown as {
-            exitStatus: string
-            stdout: string
-            stderr: string
-        }
-        assert.strictEqual(out.exitStatus, '0')
-        assert.strictEqual(out.stdout, 'mocked stdout lines')
-        assert.strictEqual(out.stderr, '')
-
-        assert.strictEqual(invokeStub.callCount, 1)
+        assert.ok('exitStatus' in result.output.content && result.output.content.exitStatus === '0')
+        assert.ok(
+            'stdout' in result.output.content &&
+                typeof result.output.content.stdout === 'string' &&
+                result.output.content.stdout.length > 0
+        )
     })
 })

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/executeBash.ts

@@ -220,6 +220,7 @@ export class ExecuteBash {
 
             let firstChunk = true
             let firstStderrChunk = true
+            const writer = updates?.getWriter()
             const childProcessOptions: processUtils.ChildProcessOptions = {
                 spawnOptions: {
                     cwd: cwd,
@@ -228,11 +229,11 @@ export class ExecuteBash {
                 collect: false,
                 waitForStreams: true,
                 onStdout: (chunk: string) => {
-                    ExecuteBash.handleChunk(firstChunk ? '```console\n' + chunk : chunk, stdoutBuffer, updates)
+                    ExecuteBash.handleChunk(firstChunk ? '```console\n' + chunk : chunk, stdoutBuffer, writer)
                     firstChunk = false
                 },
                 onStderr: (chunk: string) => {
-                    ExecuteBash.handleChunk(firstStderrChunk ? '```console\n' + chunk : chunk, stderrBuffer, updates)
+                    ExecuteBash.handleChunk(firstStderrChunk ? '```console\n' + chunk : chunk, stderrBuffer, writer)
                     firstStderrChunk = false
                 },
             }
@@ -273,13 +274,15 @@ export class ExecuteBash {
             } catch (err: any) {
                 this.logger.error(`Failed to execute bash command '${params.command}': ${err.message}`)
                 reject(new Error(`Failed to execute command: ${err.message}`))
+            } finally {
+                await writer?.close()
+                writer?.releaseLock()
             }
         })
     }
 
-    private static handleChunk(chunk: string, buffer: string[], updates?: WritableStream) {
+    private static handleChunk(chunk: string, buffer: string[], writer?: WritableStreamDefaultWriter<any>) {
         try {
-            const writer = updates?.getWriter()
             void writer?.write(chunk)
             const lines = chunk.split(/\r?\n/)
             for (const line of lines) {
@@ -302,7 +305,11 @@ export class ExecuteBash {
     }
 
     private static async whichCommand(logger: Logging, cmd: string): Promise<string> {
-        const cp = new processUtils.ChildProcess(logger, 'which', [cmd], {
+        const isWindows = process.platform === 'win32'
+        const { command, args } = isWindows
+            ? { command: 'where', args: [cmd] }
+            : { command: 'sh', args: ['-c', `command -v ${cmd}`] }
+        const cp = new processUtils.ChildProcess(logger, command, args, {
             collect: true,
             waitForStreams: true,
         })
@@ -314,7 +321,7 @@ export class ExecuteBash {
 
         const output = result.stdout.trim()
         if (!output) {
-            throw new Error(`Command '${cmd}' found but 'which' returned empty output.`)
+            throw new Error(`Command '${cmd}' found but '${command} ${args.join(' ')}' returned empty output.`)
         }
         return output
     }