mirror of
https://github.com/jlengrand/Maestro.git
synced 2026-03-10 08:31:19 +00:00
fix: address PR review issues for Windows compatibility
- Fix CLI storage path handling: use path.join instead of path.posix.join for proper Windows path construction on local filesystem operations - Fix streamJsonBuilder format: restore correct Claude Code stream-json format with type: 'user' and nested message structure (was incorrectly changed to type: 'user_message' which causes Claude CLI errors) - Add shellEscape utility: create proper shell argument escaping module with documentation and comprehensive tests for cmd.exe and PowerShell - Refactor ChildProcessSpawner: use new shellEscape utility instead of inline escaping logic for better maintainability - Fix type errors: correct ToolType usage (remove invalid 'claude' value) and add explicit Record<string, AgentSshRemoteConfig> typing
This commit is contained in:
140
src/__tests__/main/process-manager/utils/shellEscape.test.ts
Normal file
140
src/__tests__/main/process-manager/utils/shellEscape.test.ts
Normal file
@@ -0,0 +1,140 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import {
|
||||
escapeCmdArg,
|
||||
escapePowerShellArg,
|
||||
escapeCmdArgs,
|
||||
escapePowerShellArgs,
|
||||
isPowerShellShell,
|
||||
escapeArgsForShell,
|
||||
} from '../../../../main/process-manager/utils/shellEscape';
|
||||
|
||||
describe('shellEscape', () => {
|
||||
describe('escapeCmdArg', () => {
|
||||
it('should not escape simple arguments', () => {
|
||||
expect(escapeCmdArg('hello')).toBe('hello');
|
||||
expect(escapeCmdArg('world123')).toBe('world123');
|
||||
expect(escapeCmdArg('file.txt')).toBe('file.txt');
|
||||
});
|
||||
|
||||
it('should escape arguments with spaces', () => {
|
||||
expect(escapeCmdArg('hello world')).toBe('"hello world"');
|
||||
expect(escapeCmdArg('path with spaces')).toBe('"path with spaces"');
|
||||
});
|
||||
|
||||
it('should escape arguments with special characters', () => {
|
||||
expect(escapeCmdArg('foo&bar')).toBe('"foo&bar"');
|
||||
expect(escapeCmdArg('foo|bar')).toBe('"foo|bar"');
|
||||
expect(escapeCmdArg('foo<bar')).toBe('"foo<bar"');
|
||||
expect(escapeCmdArg('foo>bar')).toBe('"foo>bar"');
|
||||
});
|
||||
|
||||
it('should escape double quotes by doubling them', () => {
|
||||
expect(escapeCmdArg('say "hello"')).toBe('"say ""hello"""');
|
||||
});
|
||||
|
||||
it('should escape carets by doubling them', () => {
|
||||
expect(escapeCmdArg('foo^bar')).toBe('"foo^^bar"');
|
||||
});
|
||||
|
||||
it('should escape arguments with newlines', () => {
|
||||
expect(escapeCmdArg('line1\nline2')).toBe('"line1\nline2"');
|
||||
expect(escapeCmdArg('line1\r\nline2')).toBe('"line1\r\nline2"');
|
||||
});
|
||||
|
||||
it('should escape long arguments', () => {
|
||||
const longArg = 'a'.repeat(150);
|
||||
expect(escapeCmdArg(longArg)).toBe(`"${longArg}"`);
|
||||
});
|
||||
});
|
||||
|
||||
describe('escapePowerShellArg', () => {
|
||||
it('should not escape simple arguments', () => {
|
||||
expect(escapePowerShellArg('hello')).toBe('hello');
|
||||
expect(escapePowerShellArg('world123')).toBe('world123');
|
||||
expect(escapePowerShellArg('file.txt')).toBe('file.txt');
|
||||
});
|
||||
|
||||
it('should escape arguments with spaces', () => {
|
||||
expect(escapePowerShellArg('hello world')).toBe("'hello world'");
|
||||
});
|
||||
|
||||
it('should escape arguments with special characters', () => {
|
||||
expect(escapePowerShellArg('foo&bar')).toBe("'foo&bar'");
|
||||
expect(escapePowerShellArg('foo$bar')).toBe("'foo$bar'");
|
||||
expect(escapePowerShellArg('foo`bar')).toBe("'foo`bar'");
|
||||
});
|
||||
|
||||
it('should escape single quotes by doubling them', () => {
|
||||
expect(escapePowerShellArg("it's")).toBe("'it''s'");
|
||||
expect(escapePowerShellArg("say 'hello'")).toBe("'say ''hello'''");
|
||||
});
|
||||
|
||||
it('should escape arguments with PowerShell-specific characters', () => {
|
||||
expect(escapePowerShellArg('foo@bar')).toBe("'foo@bar'");
|
||||
expect(escapePowerShellArg('foo{bar}')).toBe("'foo{bar}'");
|
||||
expect(escapePowerShellArg('foo[bar]')).toBe("'foo[bar]'");
|
||||
});
|
||||
|
||||
it('should escape long arguments', () => {
|
||||
const longArg = 'a'.repeat(150);
|
||||
expect(escapePowerShellArg(longArg)).toBe(`'${longArg}'`);
|
||||
});
|
||||
});
|
||||
|
||||
describe('escapeCmdArgs', () => {
|
||||
it('should escape multiple arguments', () => {
|
||||
const result = escapeCmdArgs(['simple', 'with space', 'say "hi"']);
|
||||
expect(result).toEqual(['simple', '"with space"', '"say ""hi"""']);
|
||||
});
|
||||
});
|
||||
|
||||
describe('escapePowerShellArgs', () => {
|
||||
it('should escape multiple arguments', () => {
|
||||
const result = escapePowerShellArgs(['simple', 'with space', "it's"]);
|
||||
expect(result).toEqual(['simple', "'with space'", "'it''s'"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isPowerShellShell', () => {
|
||||
it('should detect Windows PowerShell', () => {
|
||||
expect(isPowerShellShell('powershell.exe')).toBe(true);
|
||||
expect(isPowerShellShell('C:\\Windows\\System32\\WindowsPowerShell\\v1.0\\powershell.exe')).toBe(
|
||||
true
|
||||
);
|
||||
expect(isPowerShellShell('PowerShell.exe')).toBe(true);
|
||||
});
|
||||
|
||||
it('should detect PowerShell Core (pwsh)', () => {
|
||||
expect(isPowerShellShell('pwsh')).toBe(true);
|
||||
expect(isPowerShellShell('pwsh.exe')).toBe(true);
|
||||
expect(isPowerShellShell('C:\\Program Files\\PowerShell\\7\\pwsh.exe')).toBe(true);
|
||||
});
|
||||
|
||||
it('should not detect cmd.exe as PowerShell', () => {
|
||||
expect(isPowerShellShell('cmd.exe')).toBe(false);
|
||||
expect(isPowerShellShell('C:\\Windows\\System32\\cmd.exe')).toBe(false);
|
||||
});
|
||||
|
||||
it('should handle undefined and empty strings', () => {
|
||||
expect(isPowerShellShell(undefined)).toBe(false);
|
||||
expect(isPowerShellShell('')).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('escapeArgsForShell', () => {
|
||||
it('should use PowerShell escaping for PowerShell shells', () => {
|
||||
const result = escapeArgsForShell(['with space', "it's"], 'powershell.exe');
|
||||
expect(result).toEqual(["'with space'", "'it''s'"]);
|
||||
});
|
||||
|
||||
it('should use cmd.exe escaping for cmd.exe shells', () => {
|
||||
const result = escapeArgsForShell(['with space', 'say "hi"'], 'cmd.exe');
|
||||
expect(result).toEqual(['"with space"', '"say ""hi"""']);
|
||||
});
|
||||
|
||||
it('should default to cmd.exe escaping when no shell specified', () => {
|
||||
const result = escapeArgsForShell(['with space', 'say "hi"']);
|
||||
expect(result).toEqual(['"with space"', '"say ""hi"""']);
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -22,16 +22,16 @@ function getConfigDir(): string {
|
||||
const home = os.homedir();
|
||||
|
||||
if (platform === 'darwin') {
|
||||
return path.posix.join(home, 'Library', 'Application Support', 'Maestro');
|
||||
return path.join(home, 'Library', 'Application Support', 'Maestro');
|
||||
} else if (platform === 'win32') {
|
||||
return path.posix.join(
|
||||
process.env.APPDATA || path.posix.join(home, 'AppData', 'Roaming'),
|
||||
return path.join(
|
||||
process.env.APPDATA || path.join(home, 'AppData', 'Roaming'),
|
||||
'Maestro'
|
||||
);
|
||||
} else {
|
||||
// Linux and others
|
||||
return path.posix.join(
|
||||
process.env.XDG_CONFIG_HOME || path.posix.join(home, '.config'),
|
||||
return path.join(
|
||||
process.env.XDG_CONFIG_HOME || path.join(home, '.config'),
|
||||
'Maestro'
|
||||
);
|
||||
}
|
||||
@@ -42,7 +42,7 @@ function getConfigDir(): string {
|
||||
* Returns undefined if file doesn't exist
|
||||
*/
|
||||
function readStoreFile<T>(filename: string): T | undefined {
|
||||
const filePath = path.posix.join(getConfigDir(), filename);
|
||||
const filePath = path.join(getConfigDir(), filename);
|
||||
|
||||
try {
|
||||
const content = fs.readFileSync(filePath, 'utf-8');
|
||||
@@ -101,7 +101,7 @@ export function readGroups(): Group[] {
|
||||
* Check if migration to per-session history format has been completed
|
||||
*/
|
||||
function hasMigrated(): boolean {
|
||||
const markerPath = path.posix.join(getConfigDir(), 'history-migrated.json');
|
||||
const markerPath = path.join(getConfigDir(), 'history-migrated.json');
|
||||
return fs.existsSync(markerPath);
|
||||
}
|
||||
|
||||
@@ -109,7 +109,7 @@ function hasMigrated(): boolean {
|
||||
* Get the history directory path
|
||||
*/
|
||||
function getHistoryDir(): string {
|
||||
return path.posix.join(getConfigDir(), 'history');
|
||||
return path.join(getConfigDir(), 'history');
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -117,7 +117,7 @@ function getHistoryDir(): string {
|
||||
*/
|
||||
function getSessionHistoryPath(sessionId: string): string {
|
||||
const safeId = sanitizeSessionId(sessionId);
|
||||
return path.posix.join(getHistoryDir(), `${safeId}.json`);
|
||||
return path.join(getHistoryDir(), `${safeId}.json`);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -14,6 +14,7 @@ import { ExitHandler } from '../handlers/ExitHandler';
|
||||
import { buildChildProcessEnv } from '../utils/envBuilder';
|
||||
import { saveImageToTempFile } from '../utils/imageUtils';
|
||||
import { buildStreamJsonMessage } from '../utils/streamJsonBuilder';
|
||||
import { escapeArgsForShell, isPowerShellShell } from '../utils/shellEscape';
|
||||
|
||||
/**
|
||||
* Handles spawning of child processes (non-PTY).
|
||||
@@ -178,46 +179,18 @@ export class ChildProcessSpawner {
|
||||
{ command: spawnCommand }
|
||||
);
|
||||
|
||||
// Check if we're using PowerShell (for SSH commands to avoid cmd.exe 8191 char limit)
|
||||
const isPowerShell =
|
||||
typeof config.shell === 'string' && config.shell.toLowerCase().includes('powershell');
|
||||
// Use the shell escape utility for proper argument escaping
|
||||
const shellPath = typeof config.shell === 'string' ? config.shell : undefined;
|
||||
spawnArgs = escapeArgsForShell(finalArgs, shellPath);
|
||||
|
||||
if (isPowerShell) {
|
||||
// Escape arguments for PowerShell (supports longer command lines than cmd.exe)
|
||||
spawnArgs = finalArgs.map((arg) => {
|
||||
const needsQuoting = /[ &|<>^%!()"\n\r#?*`$]/.test(arg) || arg.length > 100;
|
||||
if (needsQuoting) {
|
||||
// PowerShell escaping: wrap in single quotes, escape single quotes by doubling
|
||||
const escaped = arg.replace(/'/g, "''");
|
||||
return `'${escaped}'`;
|
||||
}
|
||||
return arg;
|
||||
});
|
||||
logger.info('[ProcessManager] Escaped args for PowerShell', 'ProcessManager', {
|
||||
originalArgsCount: finalArgs.length,
|
||||
escapedArgsCount: spawnArgs.length,
|
||||
escapedPromptArgLength: spawnArgs[spawnArgs.length - 1]?.length,
|
||||
escapedPromptArgPreview: spawnArgs[spawnArgs.length - 1]?.substring(0, 200),
|
||||
argsModified: finalArgs.some((arg, i) => arg !== spawnArgs[i]),
|
||||
});
|
||||
} else {
|
||||
// Escape arguments for cmd.exe when using shell
|
||||
spawnArgs = finalArgs.map((arg) => {
|
||||
const needsQuoting = /[ &|<>^%!()"\n\r#?*]/.test(arg) || arg.length > 100;
|
||||
if (needsQuoting) {
|
||||
const escaped = arg.replace(/"/g, '""').replace(/\^/g, '^^');
|
||||
return `"${escaped}"`;
|
||||
}
|
||||
return arg;
|
||||
});
|
||||
logger.info('[ProcessManager] Escaped args for Windows shell', 'ProcessManager', {
|
||||
originalArgsCount: finalArgs.length,
|
||||
escapedArgsCount: spawnArgs.length,
|
||||
escapedPromptArgLength: spawnArgs[spawnArgs.length - 1]?.length,
|
||||
escapedPromptArgPreview: spawnArgs[spawnArgs.length - 1]?.substring(0, 200),
|
||||
argsModified: finalArgs.some((arg, i) => arg !== spawnArgs[i]),
|
||||
});
|
||||
}
|
||||
const shellType = isPowerShellShell(shellPath) ? 'PowerShell' : 'cmd.exe';
|
||||
logger.info(`[ProcessManager] Escaped args for ${shellType}`, 'ProcessManager', {
|
||||
originalArgsCount: finalArgs.length,
|
||||
escapedArgsCount: spawnArgs.length,
|
||||
escapedPromptArgLength: spawnArgs[spawnArgs.length - 1]?.length,
|
||||
escapedPromptArgPreview: spawnArgs[spawnArgs.length - 1]?.substring(0, 200),
|
||||
argsModified: finalArgs.some((arg, i) => arg !== spawnArgs[i]),
|
||||
});
|
||||
}
|
||||
|
||||
// Determine shell option to pass to child_process.spawn.
|
||||
|
||||
121
src/main/process-manager/utils/shellEscape.ts
Normal file
121
src/main/process-manager/utils/shellEscape.ts
Normal file
@@ -0,0 +1,121 @@
|
||||
/**
|
||||
* Shell argument escaping utilities for Windows cmd.exe and PowerShell.
|
||||
*
|
||||
* These functions handle escaping command line arguments to prevent injection
|
||||
* and ensure proper argument passing when spawning processes via shell.
|
||||
*
|
||||
* References:
|
||||
* - cmd.exe escaping: https://docs.microsoft.com/en-us/windows-server/administration/windows-commands/cmd
|
||||
* - PowerShell escaping: https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules
|
||||
*
|
||||
* For production use, consider using the 'shescape' npm package which provides
|
||||
* more comprehensive cross-platform shell escaping with extensive testing.
|
||||
*/
|
||||
|
||||
/**
|
||||
* Characters that require quoting in cmd.exe.
|
||||
* Based on cmd.exe special characters: https://ss64.com/nt/syntax-esc.html
|
||||
*/
|
||||
const CMD_SPECIAL_CHARS = /[ &|<>^%!()"\n\r#?*]/;
|
||||
|
||||
/**
|
||||
* Characters that require quoting in PowerShell.
|
||||
* Based on PowerShell special characters: https://ss64.com/ps/syntax-esc.html
|
||||
*/
|
||||
const POWERSHELL_SPECIAL_CHARS = /[ &|<>^%!()"\n\r#?*`$@{}[\]';,]/;
|
||||
|
||||
/**
|
||||
* Escape a single argument for use in cmd.exe.
|
||||
*
|
||||
* Strategy:
|
||||
* 1. If the argument contains special characters or is long, wrap in double quotes
|
||||
* 2. Escape existing double quotes by doubling them
|
||||
* 3. Escape carets (^) as they're the escape character in cmd.exe
|
||||
*
|
||||
* @param arg - The argument to escape
|
||||
* @returns The escaped argument safe for cmd.exe
|
||||
*/
|
||||
export function escapeCmdArg(arg: string): string {
|
||||
// If no special characters and not too long, return as-is
|
||||
if (!CMD_SPECIAL_CHARS.test(arg) && arg.length <= 100) {
|
||||
return arg;
|
||||
}
|
||||
|
||||
// Escape double quotes by doubling them, and carets by doubling
|
||||
const escaped = arg.replace(/"/g, '""').replace(/\^/g, '^^');
|
||||
|
||||
// Wrap in double quotes
|
||||
return `"${escaped}"`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Escape a single argument for use in PowerShell.
|
||||
*
|
||||
* Strategy:
|
||||
* 1. If the argument contains special characters or is long, wrap in single quotes
|
||||
* 2. Escape existing single quotes by doubling them (PowerShell's single-quote escape)
|
||||
*
|
||||
* Single quotes in PowerShell treat the content as a literal string, which is
|
||||
* safer than double quotes (which allow variable expansion).
|
||||
*
|
||||
* @param arg - The argument to escape
|
||||
* @returns The escaped argument safe for PowerShell
|
||||
*/
|
||||
export function escapePowerShellArg(arg: string): string {
|
||||
// If no special characters and not too long, return as-is
|
||||
if (!POWERSHELL_SPECIAL_CHARS.test(arg) && arg.length <= 100) {
|
||||
return arg;
|
||||
}
|
||||
|
||||
// Escape single quotes by doubling them (PowerShell's escape mechanism)
|
||||
const escaped = arg.replace(/'/g, "''");
|
||||
|
||||
// Wrap in single quotes (prevents variable expansion)
|
||||
return `'${escaped}'`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Escape an array of arguments for use in cmd.exe.
|
||||
*
|
||||
* @param args - The arguments to escape
|
||||
* @returns The escaped arguments safe for cmd.exe
|
||||
*/
|
||||
export function escapeCmdArgs(args: string[]): string[] {
|
||||
return args.map(escapeCmdArg);
|
||||
}
|
||||
|
||||
/**
|
||||
* Escape an array of arguments for use in PowerShell.
|
||||
*
|
||||
* @param args - The arguments to escape
|
||||
* @returns The escaped arguments safe for PowerShell
|
||||
*/
|
||||
export function escapePowerShellArgs(args: string[]): string[] {
|
||||
return args.map(escapePowerShellArg);
|
||||
}
|
||||
|
||||
/**
|
||||
* Detect if a shell path refers to PowerShell.
|
||||
*
|
||||
* @param shellPath - The shell path to check
|
||||
* @returns True if the shell is PowerShell (either Windows PowerShell or PowerShell Core)
|
||||
*/
|
||||
export function isPowerShellShell(shellPath: string | undefined): boolean {
|
||||
if (!shellPath) return false;
|
||||
const lower = shellPath.toLowerCase();
|
||||
return lower.includes('powershell') || lower.includes('pwsh');
|
||||
}
|
||||
|
||||
/**
|
||||
* Escape arguments based on the target shell.
|
||||
*
|
||||
* @param args - The arguments to escape
|
||||
* @param shell - The shell path or name (optional, defaults to cmd.exe behavior)
|
||||
* @returns The escaped arguments
|
||||
*/
|
||||
export function escapeArgsForShell(args: string[], shell?: string): string[] {
|
||||
if (isPowerShellShell(shell)) {
|
||||
return escapePowerShellArgs(args);
|
||||
}
|
||||
return escapeCmdArgs(args);
|
||||
}
|
||||
@@ -17,20 +17,19 @@ interface TextContent {
|
||||
type MessageContent = ImageContent | TextContent;
|
||||
|
||||
/**
|
||||
* Build a stream-json message for Claude Code with images and text
|
||||
* Build a stream-json message for Claude Code with images and text.
|
||||
*
|
||||
* Claude Code expects this format:
|
||||
* {"type":"user","message":{"role":"user","content":[...]}}
|
||||
*
|
||||
* Content array should have images first (Claude convention), then text.
|
||||
*/
|
||||
export function buildStreamJsonMessage(prompt: string, images: string[]): string {
|
||||
const content: MessageContent[] = [];
|
||||
|
||||
// Add text content first
|
||||
content.push({
|
||||
type: 'text',
|
||||
text: prompt,
|
||||
});
|
||||
|
||||
// Add image content for each image
|
||||
for (const imageDataUrl of images) {
|
||||
const parsed = parseDataUrl(imageDataUrl);
|
||||
// Add images first (Claude convention: images before text)
|
||||
for (const dataUrl of images) {
|
||||
const parsed = parseDataUrl(dataUrl);
|
||||
if (parsed) {
|
||||
content.push({
|
||||
type: 'image',
|
||||
@@ -43,9 +42,18 @@ export function buildStreamJsonMessage(prompt: string, images: string[]): string
|
||||
}
|
||||
}
|
||||
|
||||
// Add text content
|
||||
content.push({
|
||||
type: 'text',
|
||||
text: prompt,
|
||||
});
|
||||
|
||||
const message = {
|
||||
type: 'user_message',
|
||||
content,
|
||||
type: 'user',
|
||||
message: {
|
||||
role: 'user',
|
||||
content,
|
||||
},
|
||||
};
|
||||
|
||||
return JSON.stringify(message);
|
||||
|
||||
@@ -1136,7 +1136,10 @@ export function NewInstanceModal({
|
||||
}
|
||||
onSshRemoteConfigChange={(config) => {
|
||||
setAgentSshRemoteConfigs((prev) => {
|
||||
const newConfigs = { ...prev, _pending_: config };
|
||||
const newConfigs: Record<string, AgentSshRemoteConfig> = {
|
||||
...prev,
|
||||
_pending_: config,
|
||||
};
|
||||
if (selectedAgent) {
|
||||
newConfigs[selectedAgent] = config;
|
||||
}
|
||||
|
||||
@@ -586,7 +586,7 @@ export function useInlineWizard(): UseInlineWizardReturn {
|
||||
|
||||
// Step 4: Initialize conversation session (only for 'new' or 'iterate' modes)
|
||||
// Only allow wizard for agents that support structured output
|
||||
const supportedWizardAgents: ToolType[] = ['claude', 'claude-code', 'codex'];
|
||||
const supportedWizardAgents: ToolType[] = ['claude-code', 'codex'];
|
||||
if (
|
||||
(mode === 'new' || mode === 'iterate') &&
|
||||
agentType &&
|
||||
@@ -699,7 +699,7 @@ export function useInlineWizard(): UseInlineWizardReturn {
|
||||
const sendMessage = useCallback(
|
||||
async (content: string, callbacks?: ConversationCallbacks): Promise<void> => {
|
||||
// Only allow wizard for agents that support structured output
|
||||
const supportedWizardAgents: ToolType[] = ['claude', 'claude-code', 'codex'];
|
||||
const supportedWizardAgents: ToolType[] = ['claude-code', 'codex'];
|
||||
|
||||
// Get the tab ID from the current state, ensure currentTabId is set for visibility
|
||||
const tabId = currentTabId || 'default';
|
||||
|
||||
Reference in New Issue
Block a user