Fix bash tool parser to merge default commands with custom arrays (#1094)

* Initial plan

* Initial analysis: Understand bash tool permissions issue

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Add make:* commands to DefaultBashTools

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Final validation: Code review passed, all tests successful

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Fix parser to merge default bash tools with custom commands

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

* Apply code formatting (gofmt)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>

Author: Copilot

.github/workflows/tidy.lock.yml

@@ -65,6 +65,15 @@ func TestBashDefaultsConsistency(t *testing.T) {
 			},
 			safeOutputs: nil,
 		},
+		{
+			name: "bash with make array and create-pull-request (tidy.md config)",
+			tools: map[string]any{
+				"bash": []any{"make:*"},
+			},
+			safeOutputs: &SafeOutputsConfig{
+				CreatePullRequests: &CreatePullRequestsConfig{},
+			},
+		},
 	}
 
 	for _, tt := range tests {

pkg/workflow/bash_defaults_consistency_test.go

@@ -0,0 +1,98 @@
+package workflow
+
+import (
+	"testing"
+)
+
+// TestBashToolsMergeCustomWithDefaults tests that custom bash tools get merged with defaults
+func TestBashToolsMergeCustomWithDefaults(t *testing.T) {
+	compiler := NewCompiler(false, "", "test")
+
+	tests := []struct {
+		name        string
+		tools       map[string]any
+		safeOutputs *SafeOutputsConfig
+		expected    []string
+	}{
+		{
+			name: "bash with make commands should include defaults + make",
+			tools: map[string]any{
+				"bash": []any{"make:*"},
+			},
+			safeOutputs: nil,
+			expected:    []string{"echo", "ls", "pwd", "cat", "head", "tail", "grep", "wc", "sort", "uniq", "date", "make:*"},
+		},
+		{
+			name: "bash with multiple commands should include defaults + custom",
+			tools: map[string]any{
+				"bash": []any{"make:*", "npm:*"},
+			},
+			safeOutputs: nil,
+			expected:    []string{"echo", "ls", "pwd", "cat", "head", "tail", "grep", "wc", "sort", "uniq", "date", "make:*", "npm:*"},
+		},
+		{
+			name: "bash with empty array should remain empty",
+			tools: map[string]any{
+				"bash": []any{},
+			},
+			safeOutputs: nil,
+			expected:    []string{},
+		},
+		{
+			name: "bash with make commands and safe outputs should include defaults + make + git",
+			tools: map[string]any{
+				"bash": []any{"make:*"},
+			},
+			safeOutputs: &SafeOutputsConfig{
+				CreatePullRequests: &CreatePullRequestsConfig{},
+			},
+			expected: []string{"echo", "ls", "pwd", "cat", "head", "tail", "grep", "wc", "sort", "uniq", "date", "make:*", "git checkout:*", "git branch:*", "git switch:*", "git add:*", "git rm:*", "git commit:*", "git merge:*"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Apply default tools
+			result := compiler.applyDefaultTools(tt.tools, tt.safeOutputs)
+
+			// Check the bash tools
+			bashTools, exists := result["bash"]
+			if !exists {
+				t.Fatalf("Expected bash tools to exist")
+			}
+
+			bashArray, ok := bashTools.([]any)
+			if !ok {
+				t.Fatalf("Expected bash tools to be an array, got %T", bashTools)
+			}
+
+			// Convert to string slice
+			actual := make([]string, len(bashArray))
+			for i, tool := range bashArray {
+				actual[i] = tool.(string)
+			}
+
+			// Debug: print actual tools
+			t.Logf("Actual tools: %v", actual)
+			t.Logf("Expected tools: %v", tt.expected)
+
+			// Check length
+			if len(actual) != len(tt.expected) {
+				t.Errorf("Expected %d tools, got %d. Expected: %v, Actual: %v", len(tt.expected), len(actual), tt.expected, actual)
+				return
+			}
+
+			// Check all expected tools are present
+			actualMap := make(map[string]bool)
+			for _, tool := range actual {
+				actualMap[tool] = true
+			}
+
+			for _, expected := range tt.expected {
+				if !actualMap[expected] {
+					t.Errorf("Expected tool '%s' not found in actual tools: %v", expected, actual)
+				}
+			}
+		})
+	}
+}

pkg/workflow/bash_merge_test.go

@@ -1069,7 +1069,7 @@ func (c *Compiler) applyDefaultTools(tools map[string]any, safeOutputs *SafeOutp
 	// Behavior:
 	//   - bash: true or bash: nil → Add default commands
 	//   - bash: [] → No commands (empty array means no tools allowed)
-	//   - bash: ["cmd1", "cmd2"] → Keep specific commands as-is
+	//   - bash: ["cmd1", "cmd2"] → Add default commands + specific commands
 	if bashTool, exists := tools["bash"]; exists {
 		// Check if bash was left as nil or true after git processing
 		if bashTool == nil {
@@ -1089,8 +1089,31 @@ func (c *Compiler) applyDefaultTools(tools map[string]any, safeOutputs *SafeOutp
 				defaultCommands[i] = cmd
 			}
 			tools["bash"] = defaultCommands
+		} else if bashArray, ok := bashTool.([]any); ok {
+			// bash is an array - merge default commands with custom commands
+			if len(bashArray) > 0 {
+				// Create a set to track existing commands to avoid duplicates
+				existingCommands := make(map[string]bool)
+				for _, cmd := range bashArray {
+					if cmdStr, ok := cmd.(string); ok {
+						existingCommands[cmdStr] = true
+					}
+				}
+
+				// Start with default commands
+				mergedCommands := make([]any, 0, len(constants.DefaultBashTools)+len(bashArray))
+				for _, cmd := range constants.DefaultBashTools {
+					if !existingCommands[cmd] {
+						mergedCommands = append(mergedCommands, cmd)
+					}
+				}
+
+				// Add the custom commands
+				mergedCommands = append(mergedCommands, bashArray...)
+				tools["bash"] = mergedCommands
+			}
+			// Note: bash with empty array (bash: []) means "no bash tools allowed" and is left as-is
 		}
-		// Note: bash with empty array (bash: []) means "no bash tools allowed" and is left as-is
 	}
 
 	return tools

pkg/workflow/compiler.go

@@ -151,10 +151,10 @@ Some normal log content
 
     // Should handle invalid patterns gracefully, not throw
     const hasErrors = validateErrors(logContent, patterns);
-    
+
     // Should return false since no valid patterns matched
     expect(hasErrors).toBe(false);
-    
+
     // Should log an error about the invalid pattern
     expect(global.core.error).toHaveBeenCalledWith("invalid error regex pattern: [invalid regex");
   });