-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 2 commits
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,146 @@ 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\\First Last\\AppData\\Roaming\\npm\\claude.cmd'); | ||
| expect(cmd).toBe('"C:\\Users\\First Last\\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"');
}); |
||
|
|
||
| it('should be idempotent - already quoted .cmd files stay quoted', () => { | ||
| const cmd = getSpawnCommand('"C:\\Users\\admin\\AppData\\Roaming\\npm\\claude.cmd"'); | ||
| expect(cmd).toBe('"C:\\Users\\admin\\AppData\\Roaming\\npm\\claude.cmd"'); | ||
| }); | ||
|
|
||
| it('should be idempotent - already quoted .bat files stay quoted', () => { | ||
| const cmd = getSpawnCommand('"D:\\Program Files\\scripts\\setup.bat"'); | ||
| expect(cmd).toBe('"D:\\Program Files\\scripts\\setup.bat"'); | ||
| }); | ||
|
|
||
| it('should be idempotent - double-quoting does not occur', () => { | ||
| const once = getSpawnCommand('C:\\Users\\admin\\npm\\claude.cmd'); | ||
| const twice = getSpawnCommand(once); | ||
| expect(once).toBe(twice); | ||
| expect(once).toBe('"C:\\Users\\admin\\npm\\claude.cmd"'); | ||
| }); | ||
|
|
||
| it('should trim whitespace before processing', () => { | ||
| const cmd = getSpawnCommand(' C:\\Users\\admin\\npm\\claude.cmd '); | ||
| expect(cmd).toBe('"C:\\Users\\admin\\npm\\claude.cmd"'); | ||
| }); | ||
|
|
||
| it('should handle already-quoted .cmd with spaces', () => { | ||
| const cmd = getSpawnCommand('"C:\\Users\\First Last\\npm\\claude.cmd"'); | ||
| expect(cmd).toBe('"C:\\Users\\First Last\\npm\\claude.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 |
||
| }); | ||
|
|
||
| describe('shouldUseShell with quoted paths', () => { | ||
| 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 detect .cmd files in quoted paths', () => { | ||
| expect(shouldUseShell('"C:\\Users\\admin\\npm\\claude.cmd"')).toBe(true); | ||
| expect(shouldUseShell('"D:\\Tools\\CLAUDE.CMD"')).toBe(true); | ||
| }); | ||
|
|
||
| it('should detect .bat files in quoted paths', () => { | ||
| expect(shouldUseShell('"C:\\Scripts\\setup.bat"')).toBe(true); | ||
| expect(shouldUseShell('"D:\\Program Files\\script.BAT"')).toBe(true); | ||
| }); | ||
|
|
||
| it('should NOT detect .exe files in quoted paths', () => { | ||
| expect(shouldUseShell('"C:\\Program Files\\git.exe"')).toBe(false); | ||
| }); | ||
|
|
||
| it('should handle whitespace around quoted paths', () => { | ||
| expect(shouldUseShell(' "C:\\Users\\admin\\npm\\claude.cmd" ')).toBe(true); | ||
| }); | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.