Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Apr 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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')
})
Expand All @@ -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'),
Expand All @@ -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
)
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ export class ExecuteBash {

let firstChunk = true
let firstStderrChunk = true
const writer = updates?.getWriter()
Copy link
Contributor

@kmile kmile Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this writer used? If it's used to communicate updates to the client then we might want to initialize the tool with a more comprehensive abstraction (I'm not saying tossing Features into it, but maybe something more explicit than an any stream?)

Copy link
Contributor Author

@Hweinstock Hweinstock Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it is used to pipe the stdout and stderr streams into the chat from the ChildProcess.

Yes, I agree an abstraction would be helpful. This is the equivalent of that on the VSC side, but this will likely have to be rewritten to properly interface with Flare's chat interface.

const childProcessOptions: processUtils.ChildProcessOptions = {
spawnOptions: {
cwd: cwd,
Expand All @@ -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
},
}
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
})
Expand All @@ -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
}
Expand Down
Loading