Fix copilot engine shell argument escaping for special characters

pkg/workflow/copilot_engine.go

@@ -119,7 +119,7 @@ func (e *CopilotEngine) GetExecutionSteps(workflowData *WorkflowData, logFile st
 INSTRUCTION=$(cat /tmp/aw-prompts/prompt.txt)
 
 # Run copilot CLI with log capture
-copilot %s 2>&1 | tee %s`, strings.Join(copilotArgs, " "), logFile)
+copilot %s 2>&1 | tee %s`, shellJoinArgs(copilotArgs), logFile)
 
 	env := map[string]string{
 		"XDG_CONFIG_HOME":     tempFolder, // copilot help environment

pkg/workflow/copilot_engine_test.go

@@ -353,41 +353,89 @@ func TestCopilotEngineExecutionStepsWithToolArguments(t *testing.T) {
 	}
 }
 
-func TestCopilotEngineUploadConfigStep(t *testing.T) {
+func TestCopilotEngineShellEscaping(t *testing.T) {
 	engine := NewCopilotEngine()
 	workflowData := &WorkflowData{
 		Name: "test-workflow",
+		Tools: map[string]any{
+			"bash": []any{"git add:*", "git commit:*"},
+		},
 	}
 	steps := engine.GetExecutionSteps(workflowData, "/tmp/test.log")
 
 	if len(steps) != 3 {
-		t.Fatalf("Expected 3 steps (upload config + copilot execution + log capture), got %d", len(steps))
+		t.Fatalf("Expected 3 steps, got %d", len(steps))
 	}
 
-	// Check the upload config step is present and correct
-	uploadStepContent := strings.Join([]string(steps[0]), "\n")
+	// Get the full command from the execution step (step 1 is the copilot execution)
+	stepContent := strings.Join([]string(steps[1]), "\n")
 
-	if !strings.Contains(uploadStepContent, "name: Upload config") {
-		t.Errorf("Expected upload config step name in:\n%s", uploadStepContent)
+	// Find the line that contains the copilot command
+	lines := strings.Split(stepContent, "\n")
+	var copilotCommand string
+	for _, line := range lines {
+		if strings.Contains(line, "copilot ") && strings.Contains(line, "--allow-tool") {
+			copilotCommand = strings.TrimSpace(line)
+			break
+		}
 	}
 
-	if !strings.Contains(uploadStepContent, "if: always()") {
-		t.Errorf("Expected upload config step to have 'if: always()' condition in:\n%s", uploadStepContent)
+	if copilotCommand == "" {
+		t.Fatalf("Could not find copilot command in step content:\n%s", stepContent)
 	}
 
-	if !strings.Contains(uploadStepContent, "uses: actions/upload-artifact@v4") {
-		t.Errorf("Expected upload config step to use 'actions/upload-artifact@v4' in:\n%s", uploadStepContent)
+	// Verify that arguments with special characters are properly quoted
+	// This test should fail initially, showing the need for escaping
+	t.Logf("Generated command: %s", copilotCommand)
+
+	// The command should contain properly escaped arguments with single quotes
+	if !strings.Contains(copilotCommand, "'shell(git add:*)'") {
+		t.Errorf("Expected 'shell(git add:*)' to be single-quoted in command: %s", copilotCommand)
 	}
 
-	if !strings.Contains(uploadStepContent, "name: config") {
-		t.Errorf("Expected artifact name 'config' in:\n%s", uploadStepContent)
+	if !strings.Contains(copilotCommand, "'shell(git commit:*)'") {
+		t.Errorf("Expected 'shell(git commit:*)' to be single-quoted in command: %s", copilotCommand)
 	}
+}
 
-	if !strings.Contains(uploadStepContent, "path: /tmp/.copilot/") {
-		t.Errorf("Expected artifact path '/tmp/.copilot/' in:\n%s", uploadStepContent)
+func TestCopilotEngineInstructionPromptNotEscaped(t *testing.T) {
+	engine := NewCopilotEngine()
+	workflowData := &WorkflowData{
+		Name: "test-workflow",
+		Tools: map[string]any{
+			"bash": []any{"git status"},
+		},
+	}
+	steps := engine.GetExecutionSteps(workflowData, "/tmp/test.log")
+
+	if len(steps) != 3 {
+		t.Fatalf("Expected 3 steps, got %d", len(steps))
+	}
+
+	// Get the full command from the execution step (step 1 is the copilot execution)
+	stepContent := strings.Join([]string(steps[1]), "\n")
+
+	// Find the line that contains the copilot command
+	lines := strings.Split(stepContent, "\n")
+	var copilotCommand string
+	for _, line := range lines {
+		if strings.Contains(line, "copilot ") && strings.Contains(line, "--prompt") {
+			copilotCommand = strings.TrimSpace(line)
+			break
+		}
+	}
+
+	if copilotCommand == "" {
+		t.Fatalf("Could not find copilot command in step content:\n%s", stepContent)
+	}
+
+	// The $INSTRUCTION should NOT be wrapped in additional single quotes
+	if strings.Contains(copilotCommand, `'"$INSTRUCTION"'`) {
+		t.Errorf("$INSTRUCTION should not be wrapped in single quotes: %s", copilotCommand)
 	}
 
-	if !strings.Contains(uploadStepContent, "if-no-files-found: ignore") {
-		t.Errorf("Expected 'if-no-files-found: ignore' in:\n%s", uploadStepContent)
+	// The $INSTRUCTION should remain double-quoted for variable expansion
+	if !strings.Contains(copilotCommand, `"$INSTRUCTION"`) {
+		t.Errorf("$INSTRUCTION should remain double-quoted: %s", copilotCommand)
 	}
 }

pkg/workflow/shell.go

@@ -0,0 +1,35 @@
+package workflow
+
+import "strings"
+
+// shellJoinArgs joins command arguments with proper shell escaping
+// Arguments containing special characters are wrapped in single quotes
+func shellJoinArgs(args []string) string {
+	var escapedArgs []string
+	for _, arg := range args {
+		escapedArgs = append(escapedArgs, shellEscapeArg(arg))
+	}
+	return strings.Join(escapedArgs, " ")
+}
+
+// shellEscapeArg escapes a single argument for safe use in shell commands
+// Arguments containing special characters are wrapped in single quotes
+func shellEscapeArg(arg string) string {
+	// If the argument is already properly quoted with double quotes, leave it as-is
+	if len(arg) >= 2 && arg[0] == '"' && arg[len(arg)-1] == '"' {
+		return arg
+	}
+
+	// If the argument is already properly quoted with single quotes, leave it as-is
+	if len(arg) >= 2 && arg[0] == '\'' && arg[len(arg)-1] == '\'' {
+		return arg
+	}
+
+	// Check if the argument contains special shell characters that need escaping
+	if strings.ContainsAny(arg, "()[]{}*?$`\"'\\|&;<> \t\n") {
+		// Handle single quotes in the argument by escaping them
+		escaped := strings.ReplaceAll(arg, "'", "'\"'\"'")
+		return "'" + escaped + "'"
+	}
+	return arg
+}

pkg/workflow/shell_test.go

@@ -0,0 +1,119 @@
+package workflow
+
+import "testing"
+
+func TestShellEscapeArg(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "simple argument without special characters",
+			input:    "hello",
+			expected: "hello",
+		},
+		{
+			name:     "argument with parentheses",
+			input:    "shell(git add:*)",
+			expected: "'shell(git add:*)'",
+		},
+		{
+			name:     "argument with brackets",
+			input:    "pattern[abc]",
+			expected: "'pattern[abc]'",
+		},
+		{
+			name:     "argument with spaces",
+			input:    "hello world",
+			expected: "'hello world'",
+		},
+		{
+			name:     "argument with single quote",
+			input:    "don't",
+			expected: "'don'\"'\"'t'",
+		},
+		{
+			name:     "argument with asterisk",
+			input:    "*.txt",
+			expected: "'*.txt'",
+		},
+		{
+			name:     "argument with dollar sign",
+			input:    "$HOME",
+			expected: "'$HOME'",
+		},
+		{
+			name:     "simple flag",
+			input:    "--allow-tool",
+			expected: "--allow-tool",
+		},
+		{
+			name:     "already double-quoted argument should not be escaped",
+			input:    "\"$INSTRUCTION\"",
+			expected: "\"$INSTRUCTION\"",
+		},
+		{
+			name:     "already single-quoted argument should not be escaped",
+			input:    "'hello world'",
+			expected: "'hello world'",
+		},
+		{
+			name:     "partial double quote should be escaped",
+			input:    "hello\"world",
+			expected: "'hello\"world'",
+		},
+		{
+			name:     "empty double quotes should not be escaped",
+			input:    "\"\"",
+			expected: "\"\"",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := shellEscapeArg(tt.input)
+			if result != tt.expected {
+				t.Errorf("shellEscapeArg(%q) = %q, expected %q", tt.input, result, tt.expected)
+			}
+		})
+	}
+}
+
+func TestShellJoinArgs(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    []string
+		expected string
+	}{
+		{
+			name:     "simple arguments",
+			input:    []string{"git", "status"},
+			expected: "git status",
+		},
+		{
+			name:     "arguments with special characters",
+			input:    []string{"--allow-tool", "shell(git add:*)", "--allow-tool", "shell(git commit:*)"},
+			expected: "--allow-tool 'shell(git add:*)' --allow-tool 'shell(git commit:*)'",
+		},
+		{
+			name:     "mixed arguments",
+			input:    []string{"copilot", "--add-dir", "/tmp/", "--allow-tool", "shell(*.txt)"},
+			expected: "copilot --add-dir /tmp/ --allow-tool 'shell(*.txt)'",
+		},
+		{
+			name:     "prompt with pre-quoted instruction should not be escaped",
+			input:    []string{"copilot", "--add-dir", "/tmp/", "--prompt", "\"$INSTRUCTION\""},
+			expected: "copilot --add-dir /tmp/ --prompt \"$INSTRUCTION\"",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := shellJoinArgs(tt.input)
+			if result != tt.expected {
+				t.Errorf("shellJoinArgs(%q) = %q, expected %q", tt.input, result, tt.expected)
+			}
+		})
+	}
+}