-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix: properly quote Windows .cmd/.bat paths in spawn() calls #889
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
Changes from 1 commit
c0431dd
4ac77d3
0b95d67
b2865a3
55b2d4e
24d8fdc
fe7a02f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import { describe, expect, it, beforeEach, afterEach } from 'vitest'; | ||
| import { shouldUseShell, getSpawnOptions } from '../env-utils'; | ||
| import { shouldUseShell, getSpawnOptions, getSpawnCommand } from '../env-utils'; | ||
|
|
||
| describe('shouldUseShell', () => { | ||
| const originalPlatform = process.platform; | ||
|
|
@@ -198,3 +198,78 @@ describe('getSpawnOptions', () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('getSpawnCommand', () => { | ||
| const originalPlatform = process.platform; | ||
|
|
||
| afterEach(() => { | ||
| // Restore original platform after each test | ||
| Object.defineProperty(process, 'platform', { | ||
| value: originalPlatform, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| describe('Windows platform', () => { | ||
| beforeEach(() => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'win32', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); | ||
|
|
||
| it('should quote .cmd files with spaces', () => { | ||
| const cmd = getSpawnCommand('C:\\Users\\OXFAM MONS\\AppData\\Roaming\\npm\\claude.cmd'); | ||
| expect(cmd).toBe('"C:\\Users\\OXFAM MONS\\AppData\\Roaming\\npm\\claude.cmd"'); | ||
| }); | ||
|
|
||
| it('should quote .cmd files without spaces too (idempotent)', () => { | ||
| const cmd = getSpawnCommand('C:\\Users\\admin\\AppData\\Roaming\\npm\\claude.cmd'); | ||
| expect(cmd).toBe('"C:\\Users\\admin\\AppData\\Roaming\\npm\\claude.cmd"'); | ||
| }); | ||
|
|
||
| it('should quote .bat files with spaces', () => { | ||
| const cmd = getSpawnCommand('D:\\Program Files (x86)\\scripts\\setup.bat'); | ||
| expect(cmd).toBe('"D:\\Program Files (x86)\\scripts\\setup.bat"'); | ||
| }); | ||
|
|
||
| it('should NOT quote .exe files', () => { | ||
| const cmd = getSpawnCommand('C:\\Program Files\\Git\\cmd\\git.exe'); | ||
| expect(cmd).toBe('C:\\Program Files\\Git\\cmd\\git.exe'); | ||
| }); | ||
|
|
||
| it('should NOT quote extensionless files', () => { | ||
| const cmd = getSpawnCommand('D:\\Git\\bin\\bash'); | ||
| expect(cmd).toBe('D:\\Git\\bin\\bash'); | ||
| }); | ||
|
|
||
| it('should handle uppercase .CMD and .BAT extensions', () => { | ||
| expect(getSpawnCommand('D:\\Tools\\CLAUDE.CMD')).toBe('"D:\\Tools\\CLAUDE.CMD"'); | ||
| expect(getSpawnCommand('C:\\Scripts\\SETUP.BAT')).toBe('"C:\\Scripts\\SETUP.BAT"'); | ||
| }); | ||
|
Comment on lines
+248
to
+251
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To ensure the robustness of the it('should handle uppercase .CMD and .BAT extensions', () => {
expect(getSpawnCommand('D:\\Tools\\CLAUDE.CMD')).toBe('"D:\\Tools\\CLAUDE.CMD"');
expect(getSpawnCommand('C:\\Scripts\\SETUP.BAT')).toBe('"C:\\Scripts\\SETUP.BAT"');
});
it('should NOT double-quote an already quoted .cmd file', () => {
const cmd = getSpawnCommand('"C:\\Users\\Test User\\app.cmd"');
expect(cmd).toBe('"C:\\Users\\Test User\\app.cmd"');
}); |
||
| }); | ||
|
|
||
| describe('Non-Windows platforms', () => { | ||
| it('should NOT quote commands on macOS', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'darwin', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| expect(getSpawnCommand('/usr/local/bin/claude')).toBe('/usr/local/bin/claude'); | ||
| expect(getSpawnCommand('/opt/homebrew/bin/claude.cmd')).toBe('/opt/homebrew/bin/claude.cmd'); | ||
| }); | ||
|
|
||
| it('should NOT quote commands on Linux', () => { | ||
| Object.defineProperty(process, 'platform', { | ||
| value: 'linux', | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| expect(getSpawnCommand('/usr/bin/claude')).toBe('/usr/bin/claude'); | ||
| expect(getSpawnCommand('/home/user/.local/bin/claude.bat')).toBe('/home/user/.local/bin/claude.bat'); | ||
| }); | ||
| }); | ||
|
Comment on lines
296
to
336
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Add test coverage for whitespace handling on non-Windows platforms. The tests verify that commands aren't quoted on macOS/Linux, but don't test whitespace trimming behavior. Adding a test case would catch the whitespace inconsistency identified in the implementation: it('should trim whitespace on non-Windows platforms', () => {
Object.defineProperty(process, 'platform', {
value: 'darwin',
writable: true,
configurable: true,
});
expect(getSpawnCommand(' /usr/local/bin/claude ')).toBe('/usr/local/bin/claude');
});This test would currently fail due to the inconsistent whitespace handling in 🤖 Prompt for AI Agents |
||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -473,14 +473,17 @@ export function shouldUseShell(command: string): boolean { | |||||||||||||
| * Provides a consistent way to create spawn options that work across platforms. | ||||||||||||||
| * Handles the shell requirement for Windows .cmd/.bat files automatically. | ||||||||||||||
| * | ||||||||||||||
| * For .cmd/.bat files on Windows, returns options that tell the caller to use | ||||||||||||||
| * proper quoting for paths with spaces. | ||||||||||||||
| * | ||||||||||||||
| * @param command - The command path to execute | ||||||||||||||
| * @param baseOptions - Base spawn options to merge with (optional) | ||||||||||||||
| * @returns Spawn options with correct shell setting | ||||||||||||||
| * | ||||||||||||||
| * @example | ||||||||||||||
| * ```typescript | ||||||||||||||
| * const opts = getSpawnOptions(claudeCmd, { cwd: '/project', env: {...} }); | ||||||||||||||
| * spawn(claudeCmd, ['--version'], opts); | ||||||||||||||
| * spawn(getSpawnCommand(claudeCmd), ['--version'], opts); | ||||||||||||||
| * ``` | ||||||||||||||
| */ | ||||||||||||||
| export function getSpawnOptions( | ||||||||||||||
|
|
@@ -505,3 +508,28 @@ export function getSpawnOptions( | |||||||||||||
| shell: shouldUseShell(command), | ||||||||||||||
| }; | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Get the properly quoted command for use with spawn() | ||||||||||||||
| * | ||||||||||||||
| * For .cmd/.bat files on Windows with shell:true, the command path must be | ||||||||||||||
| * quoted to handle paths containing spaces correctly (e.g., C:\Users\OXFAM MONS\...). | ||||||||||||||
| * | ||||||||||||||
| * @param command - The command path to execute | ||||||||||||||
| * @returns The command (quoted if needed for .cmd/.bat files on Windows) | ||||||||||||||
| * | ||||||||||||||
| * @example | ||||||||||||||
| * ```typescript | ||||||||||||||
| * const cmd = getSpawnCommand(claudeCmd); // "C:\Users\OXFAM MONS\...\claude.cmd" | ||||||||||||||
| * const opts = getSpawnOptions(claudeCmd, { cwd: '/project', env: {...} }); | ||||||||||||||
| * spawn(cmd, ['--version'], opts); | ||||||||||||||
| * ``` | ||||||||||||||
| */ | ||||||||||||||
| export function getSpawnCommand(command: string): string { | ||||||||||||||
| // For .cmd/.bat files on Windows, quote the command to handle spaces | ||||||||||||||
| // The shell will parse the quoted path correctly | ||||||||||||||
| if (shouldUseShell(command)) { | ||||||||||||||
| return `"${command}"`; | ||||||||||||||
| } | ||||||||||||||
|
||||||||||||||
| if (shouldUseShell(command)) { | |
| return `"${command}"`; | |
| } | |
| if (shouldUseShell(command)) { | |
| return (command.startsWith('"') && command.endsWith('"')) ? command : `"${command}"`; | |
| } |
Uh oh!
There was an error while loading. Please reload this page.