fix(mcp): use shell:true for .cmd/.bat executables on Windows#122
Open
mrdailey99 wants to merge 3 commits intodevelopfrom
Open
fix(mcp): use shell:true for .cmd/.bat executables on Windows#122mrdailey99 wants to merge 3 commits intodevelopfrom
mrdailey99 wants to merge 3 commits intodevelopfrom
Conversation
spawnSync with shell:false cannot execute Windows batch files (.cmd/.bat) or extensionless commands like 'sf' that map to sf.cmd on disk. Added needsWindowsShell() to detect these cases and pass shell:true accordingly. Includes unit tests for the helper and for shell option propagation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the MCP Provar automation tooling to correctly execute Salesforce CLI shims on Windows by enabling spawnSync(..., { shell: true }) for .cmd/.bat and certain extensionless executable cases, and adds unit tests for the detection helper and spawn option propagation.
Changes:
- Add
needsWindowsShell()helper to decide when Windows shell execution is required. - Use
needsWindowsShell()in thesf --versionprobe and inrunSfCommand()spawn options. - Add unit tests for
needsWindowsShell()and forshelloption propagation intospawnSync.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/mcp/tools/automationTools.ts |
Adds Windows shell detection and applies it to spawnSync calls for sf execution. |
test/unit/mcp/automationTools.test.ts |
Adds tests for needsWindowsShell() and validates shell option behavior passed to spawnSync. |
Comments suppressed due to low confidence (1)
src/mcp/tools/automationTools.ts:132
- Switching to
shell: needsWindowsShell(executable)changes not-found behavior on Windows: whenshell:trueand the executable/path is invalid,spawnSyncmay return a non-zero exit code without populatingresult.error(because the shell started successfully). That means missingsf/sf_pathcan surface as anAUTOMATION_*_FAILEDinstead ofSF_NOT_FOUND. To keep error semantics consistent, consider (a) pre-validating an explicitsf_pathwithfs.existsSyncwhen it's an absolute path, and (b) whenshell:true, mapping common "command not found"/"system cannot find the path" stderr patterns (or non-zero exit with empty stdout/stderr) toSfNotFoundError.
const result = sfSpawnHelper.spawnSync(executable, args, {
encoding: 'utf-8',
shell: needsWindowsShell(executable),
maxBuffer: MAX_BUFFER,
});
if (result.error) {
const err = result.error as NodeJS.ErrnoException;
if (err.code === 'ENOENT') {
throw new SfNotFoundError(sfPath);
}
throw result.error;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+560
to
+590
| describe('Windows shell option in spawnSync', () => { | ||
| it('passes shell: true when sf_path is a .cmd file', () => { | ||
| spawnStub.returns(makeSpawnResult('ok', '', 0)); | ||
| server.call('provar.automation.compile', { flags: [], sf_path: 'C:\\npm\\sf.cmd' }); | ||
| const opts = spawnStub.firstCall.args[2] as { shell: boolean }; | ||
| assert.ok(opts.shell === true, 'shell should be true for a .cmd executable'); | ||
| }); | ||
|
|
||
| it('passes shell: false when sf_path is a .exe binary', () => { | ||
| // .exe has an explicit extension that is neither .cmd nor .bat, so no | ||
| // shell is required even on Windows. | ||
| spawnStub.returns(makeSpawnResult('ok', '', 0)); | ||
| server.call('provar.automation.compile', { flags: [], sf_path: 'C:\\Program Files\\sf.exe' }); | ||
| const opts = spawnStub.firstCall.args[2] as { shell: boolean }; | ||
| assert.ok(opts.shell === false, 'shell should be false for a .exe executable'); | ||
| }); | ||
|
|
||
| it('passes shell: false for a .js node script path', () => { | ||
| spawnStub.returns(makeSpawnResult('ok', '', 0)); | ||
| server.call('provar.automation.testrun', { flags: [], sf_path: 'C:\\npm\\sf.js' }); | ||
| const opts = spawnStub.firstCall.args[2] as { shell: boolean }; | ||
| assert.ok(opts.shell === false, 'shell should be false for a .js script'); | ||
| }); | ||
|
|
||
| it('passes shell: true when sf_path is a .bat file', () => { | ||
| spawnStub.returns(makeSpawnResult('ok', '', 0)); | ||
| server.call('provar.automation.testrun', { flags: [], sf_path: 'C:\\tools\\sf.bat' }); | ||
| const opts = spawnStub.firstCall.args[2] as { shell: boolean }; | ||
| assert.ok(opts.shell === true, 'shell should be true for a .bat executable'); | ||
| }); | ||
| }); |
Comment on lines
+120
to
+124
| const result = sfSpawnHelper.spawnSync(executable, args, { | ||
| encoding: 'utf-8', | ||
| shell: needsWindowsShell(executable), | ||
| maxBuffer: MAX_BUFFER, | ||
| }); |
src/mcp/tools/automationTools.ts
Outdated
| shell: needsWindowsShell('sf'), | ||
| maxBuffer: 1024 * 1024, | ||
| }); | ||
| if (!probe.error) { |
- setSfPathCacheForTesting now accepts undefined to reset the probe cache - setSfPlatformForTesting added to allow cross-OS testing of Windows shell logic - resolveSfExecutable uses two-phase probe (shell:false first, shell:true retry on win32 ENOENT) - runSfCommand uses _sfPlatform override and calls assertShellSafePath for shell:true + user sf_path - Tests: platform override in Windows shell tests; probe phase tests; injection hardening tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
spawnSync with shell:false cannot execute Windows batch files (.cmd/.bat) or extensionless commands like 'sf' that map to sf.cmd on disk. Added needsWindowsShell() to detect these cases and pass shell:true accordingly. Includes unit tests for the helper and for shell option propagation.