diff --git a/pkg/cli/workflows/test-copilot-max-patch-size.lock.yml b/pkg/cli/workflows/test-copilot-max-patch-size.lock.yml index 4a9a5a999ce..0334f5cfa42 100644 --- a/pkg/cli/workflows/test-copilot-max-patch-size.lock.yml +++ b/pkg/cli/workflows/test-copilot-max-patch-size.lock.yml @@ -766,7 +766,7 @@ jobs: INSTRUCTION=$(cat /tmp/aw-prompts/prompt.txt) # Run copilot CLI with log capture - copilot --add-dir /tmp/ --log-level debug --log-dir /tmp/.copilot/logs/ --allow-tool safe_outputs --allow-tool shell(git add:*) --allow-tool shell(git branch:*) --allow-tool shell(git checkout:*) --allow-tool shell(git commit:*) --allow-tool shell(git merge:*) --allow-tool shell(git rm:*) --allow-tool shell(git switch:*) --allow-tool write --prompt "$INSTRUCTION" 2>&1 | tee /tmp/test-copilot-patch-size-validation.log + copilot --add-dir /tmp/ --log-level debug --log-dir /tmp/.copilot/logs/ --allow-tool safe_outputs --allow-tool 'shell(git add:*)' --allow-tool 'shell(git branch:*)' --allow-tool 'shell(git checkout:*)' --allow-tool 'shell(git commit:*)' --allow-tool 'shell(git merge:*)' --allow-tool 'shell(git rm:*)' --allow-tool 'shell(git switch:*)' --allow-tool write --prompt "$INSTRUCTION" 2>&1 | tee /tmp/test-copilot-patch-size-validation.log env: GITHUB_AW_SAFE_OUTPUTS: ${{ env.GITHUB_AW_SAFE_OUTPUTS }} GITHUB_STEP_SUMMARY: ${{ env.GITHUB_STEP_SUMMARY }} diff --git a/pkg/cli/workflows/test-copilot-patch-size-exceeded.lock.yml b/pkg/cli/workflows/test-copilot-patch-size-exceeded.lock.yml index 909a8ce459a..32e4152655f 100644 --- a/pkg/cli/workflows/test-copilot-patch-size-exceeded.lock.yml +++ b/pkg/cli/workflows/test-copilot-patch-size-exceeded.lock.yml @@ -768,7 +768,7 @@ jobs: INSTRUCTION=$(cat /tmp/aw-prompts/prompt.txt) # Run copilot CLI with log capture - copilot --add-dir /tmp/ --log-level debug --log-dir /tmp/.copilot/logs/ --allow-tool safe_outputs --allow-tool shell(git add:*) --allow-tool shell(git branch:*) --allow-tool shell(git checkout:*) --allow-tool shell(git commit:*) --allow-tool shell(git merge:*) --allow-tool shell(git rm:*) --allow-tool shell(git switch:*) --allow-tool write --prompt "$INSTRUCTION" 2>&1 | tee /tmp/test-copilot-patch-size-limit-exceeded.log + copilot --add-dir /tmp/ --log-level debug --log-dir /tmp/.copilot/logs/ --allow-tool safe_outputs --allow-tool 'shell(git add:*)' --allow-tool 'shell(git branch:*)' --allow-tool 'shell(git checkout:*)' --allow-tool 'shell(git commit:*)' --allow-tool 'shell(git merge:*)' --allow-tool 'shell(git rm:*)' --allow-tool 'shell(git switch:*)' --allow-tool write --prompt "$INSTRUCTION" 2>&1 | tee /tmp/test-copilot-patch-size-limit-exceeded.log env: GITHUB_AW_SAFE_OUTPUTS: ${{ env.GITHUB_AW_SAFE_OUTPUTS }} GITHUB_STEP_SUMMARY: ${{ env.GITHUB_STEP_SUMMARY }} diff --git a/pkg/workflow/copilot_engine.go b/pkg/workflow/copilot_engine.go index fa8623f6267..bcdc2377736 100644 --- a/pkg/workflow/copilot_engine.go +++ b/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 diff --git a/pkg/workflow/copilot_engine_test.go b/pkg/workflow/copilot_engine_test.go index 4080499c251..c5d28be33da 100644 --- a/pkg/workflow/copilot_engine_test.go +++ b/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) } } diff --git a/pkg/workflow/shell.go b/pkg/workflow/shell.go new file mode 100644 index 00000000000..4d880eb5687 --- /dev/null +++ b/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 +} diff --git a/pkg/workflow/shell_test.go b/pkg/workflow/shell_test.go new file mode 100644 index 00000000000..f56c1f3aa25 --- /dev/null +++ b/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) + } + }) + } +}