diff --git a/pkg/workflow/concurrency_validation.go b/pkg/workflow/concurrency_validation.go index 779ef07f4cf..10d6602c806 100644 --- a/pkg/workflow/concurrency_validation.go +++ b/pkg/workflow/concurrency_validation.go @@ -8,7 +8,6 @@ // # Validation Functions // // - validateConcurrencyGroupExpression() - Validates syntax of a single group expression -// - extractGroupExpression() - Extracts group value from concurrency configuration // // # Validation Coverage // @@ -283,31 +282,6 @@ func containsLogicalOperators(expr string) bool { return strings.Contains(expr, "&&") || strings.Contains(expr, "||") || strings.Contains(expr, "!") } -// extractGroupExpression extracts the group value from a concurrency configuration. -// Handles both string format ("group-name") and object format ({group: "group-name"}). -// Returns the group expression string or empty string if not found. -func extractGroupExpression(concurrency any) string { - if concurrency == nil { - return "" - } - - // Handle string format (simple group name) - if groupStr, ok := concurrency.(string); ok { - return groupStr - } - - // Handle object format with group field - if concurrencyObj, ok := concurrency.(map[string]any); ok { - if group, hasGroup := concurrencyObj["group"]; hasGroup { - if groupStr, ok := group.(string); ok { - return groupStr - } - } - } - - return "" -} - // extractConcurrencyGroupFromYAML extracts the group value from a YAML-formatted concurrency string. // The input is expected to be in the format generated by the compiler: // diff --git a/pkg/workflow/concurrency_validation_test.go b/pkg/workflow/concurrency_validation_test.go index 2a58cba9940..1fb011c5f58 100644 --- a/pkg/workflow/concurrency_validation_test.go +++ b/pkg/workflow/concurrency_validation_test.go @@ -486,66 +486,6 @@ func TestContainsLogicalOperators(t *testing.T) { } } -func TestExtractGroupExpression(t *testing.T) { - tests := []struct { - name string - concurrency any - expected string - description string - }{ - { - name: "string format", - concurrency: "my-workflow-group", - expected: "my-workflow-group", - description: "Simple string should be returned as-is", - }, - { - name: "object format with group", - concurrency: map[string]any{ - "group": "my-workflow-group", - }, - expected: "my-workflow-group", - description: "Group value should be extracted from object", - }, - { - name: "object format with group and cancel", - concurrency: map[string]any{ - "group": "my-workflow-group", - "cancel-in-progress": true, - }, - expected: "my-workflow-group", - description: "Group value should be extracted ignoring other fields", - }, - { - name: "object format without group", - concurrency: map[string]any{ - "cancel-in-progress": true, - }, - expected: "", - description: "Missing group should return empty string", - }, - { - name: "nil input", - concurrency: nil, - expected: "", - description: "Nil should return empty string", - }, - { - name: "invalid type", - concurrency: 123, - expected: "", - description: "Invalid type should return empty string", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := extractGroupExpression(tt.concurrency) - assert.Equal(t, tt.expected, result, tt.description) - }) - } -} - // TestValidateConcurrencyGroupExpressionRealWorld tests real-world concurrency group patterns func TestValidateConcurrencyGroupExpressionRealWorld(t *testing.T) { // These are actual patterns used in the codebase diff --git a/pkg/workflow/config_helpers.go b/pkg/workflow/config_helpers.go index 98ea4e90181..d00d1f88b42 100644 --- a/pkg/workflow/config_helpers.go +++ b/pkg/workflow/config_helpers.go @@ -20,7 +20,6 @@ // Configuration Array Parsing: // - ParseStringArrayFromConfig() - Generic string array extraction // - parseLabelsFromConfig() - Extract labels array -// - parseParticipantsFromConfig() - Extract participants array // - parseAllowedLabelsFromConfig() - Extract allowed labels array // // Configuration String Parsing: @@ -124,40 +123,6 @@ func parseTargetRepoWithValidation(configMap map[string]any) (string, bool) { return targetRepoSlug, false } -// parseParticipantsFromConfig extracts and validates participants (assignees/reviewers) from a config map. -// Supports both string (single participant) and array (multiple participants) formats. -// Returns a slice of participant usernames, or nil if not present or invalid. -// The participantKey parameter specifies which key to look for (e.g., "assignees" or "reviewers"). -func parseParticipantsFromConfig(configMap map[string]any, participantKey string) []string { - if participants, exists := configMap[participantKey]; exists { - configHelpersLog.Printf("Parsing %s from config", participantKey) - - // Handle single string format - if participantStr, ok := participants.(string); ok { - configHelpersLog.Printf("Parsed single %s: %s", participantKey, participantStr) - return []string{participantStr} - } - - // Handle array format - if participantsArray, ok := participants.([]any); ok { - var participantStrings []string - for _, participant := range participantsArray { - if participantStr, ok := participant.(string); ok { - participantStrings = append(participantStrings, participantStr) - } - } - // Return the slice even if empty (to distinguish from not provided) - if participantStrings == nil { - configHelpersLog.Printf("No valid %s strings found, returning empty array", participantKey) - return []string{} - } - configHelpersLog.Printf("Parsed %d %s from config", len(participantStrings), participantKey) - return participantStrings - } - } - return nil -} - // parseAllowedLabelsFromConfig extracts and validates allowed-labels from a config map. // Returns a slice of label strings, or nil if not present or invalid. func parseAllowedLabelsFromConfig(configMap map[string]any) []string { @@ -210,27 +175,6 @@ func preprocessExpiresField(configData map[string]any, log *logger.Logger) bool return expiresDisabled } -// ParseIntFromConfig is a generic helper that extracts and validates an integer value from a map. -// Supports int, int64, float64, and uint64 types. -// Returns the integer value, or 0 if not present or invalid. -// If log is provided, it will log the extracted value for debugging. -// Note: For uint64 values, returns 0 if the value would overflow int. -func ParseIntFromConfig(m map[string]any, key string, log *logger.Logger) int { - if value, exists := m[key]; exists { - if log != nil { - log.Printf("Parsing %s from config", key) - } - // Use parseIntValue for the actual type conversion - if result, ok := parseIntValue(value); ok { - if log != nil { - log.Printf("Parsed %s from config: %d", key, result) - } - return result - } - } - return 0 -} - // ParseBoolFromConfig is a generic helper that extracts and validates a boolean value from a map. // Returns the boolean value, or false if not present or invalid. // If log is provided, it will log the extracted value for debugging. diff --git a/pkg/workflow/config_parsing_helpers_test.go b/pkg/workflow/config_parsing_helpers_test.go index c34ed7079e9..9100e228712 100644 --- a/pkg/workflow/config_parsing_helpers_test.go +++ b/pkg/workflow/config_parsing_helpers_test.go @@ -519,342 +519,6 @@ func TestParseTargetRepoWithValidation(t *testing.T) { } } -func TestParseParticipantsFromConfig(t *testing.T) { - tests := []struct { - name string - input map[string]any - participantKey string - expected []string - }{ - { - name: "single assignee as string", - input: map[string]any{ - "assignees": "user1", - }, - participantKey: "assignees", - expected: []string{"user1"}, - }, - { - name: "multiple assignees as array", - input: map[string]any{ - "assignees": []any{"user1", "user2", "user3"}, - }, - participantKey: "assignees", - expected: []string{"user1", "user2", "user3"}, - }, - { - name: "empty assignees array", - input: map[string]any{ - "assignees": []any{}, - }, - participantKey: "assignees", - expected: []string{}, // Empty array returns empty slice (consistent with parseLabelsFromConfig) - }, - { - name: "assignees key not present", - input: map[string]any{}, - participantKey: "assignees", - expected: nil, - }, - { - name: "assignees with non-string values (filtered out)", - input: map[string]any{ - "assignees": []any{"user1", 123, "user2", nil, "user3"}, - }, - participantKey: "assignees", - expected: []string{"user1", "user2", "user3"}, - }, - { - name: "assignees array with only non-string values", - input: map[string]any{ - "assignees": []any{123, true, nil}, - }, - participantKey: "assignees", - expected: []string{}, // All filtered out returns empty slice - }, - { - name: "single reviewer as string", - input: map[string]any{ - "reviewers": "reviewer1", - }, - participantKey: "reviewers", - expected: []string{"reviewer1"}, - }, - { - name: "multiple reviewers as array", - input: map[string]any{ - "reviewers": []any{"reviewer1", "reviewer2"}, - }, - participantKey: "reviewers", - expected: []string{"reviewer1", "reviewer2"}, - }, - { - name: "reviewers key not present", - input: map[string]any{}, - participantKey: "reviewers", - expected: nil, - }, - { - name: "assignees is not string or array", - input: map[string]any{ - "assignees": 123, - }, - participantKey: "assignees", - expected: nil, - }, - { - name: "empty string assignee", - input: map[string]any{ - "assignees": "", - }, - participantKey: "assignees", - expected: []string{""}, - }, - { - name: "mixed valid and empty strings in array", - input: map[string]any{ - "assignees": []any{"user1", "", "user2"}, - }, - participantKey: "assignees", - expected: []string{"user1", "", "user2"}, - }, - { - name: "assignees with special characters", - input: map[string]any{ - "assignees": []any{"user-1", "user_2", "user.3"}, - }, - participantKey: "assignees", - expected: []string{"user-1", "user_2", "user.3"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := parseParticipantsFromConfig(tt.input, tt.participantKey) - - // Check nil vs empty array - if tt.expected == nil { - if result != nil { - t.Errorf("expected nil, got %v", result) - } - return - } - - if result == nil { - t.Errorf("expected %v, got nil", tt.expected) - return - } - - if len(result) != len(tt.expected) { - t.Errorf("expected %d participants, got %d", len(tt.expected), len(result)) - return - } - - for i, expectedParticipant := range tt.expected { - if result[i] != expectedParticipant { - t.Errorf("participant %d: expected %q, got %q", i, expectedParticipant, result[i]) - } - } - }) - } -} - -// TestParseParticipantsFromConfigConsistency verifies that assignees and reviewers -// are parsed with identical logic when using the same input data structure -func TestParseParticipantsFromConfigConsistency(t *testing.T) { - testCases := []struct { - name string - input map[string]any - }{ - { - name: "single participant string", - input: map[string]any{ - "assignees": "user1", - "reviewers": "user1", - }, - }, - { - name: "multiple participants array", - input: map[string]any{ - "assignees": []any{"user1", "user2"}, - "reviewers": []any{"user1", "user2"}, - }, - }, - { - name: "empty participants array", - input: map[string]any{ - "assignees": []any{}, - "reviewers": []any{}, - }, - }, - { - name: "participants with filtered non-strings", - input: map[string]any{ - "assignees": []any{"user1", 123, "user2"}, - "reviewers": []any{"user1", 123, "user2"}, - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - assignees := parseParticipantsFromConfig(tc.input, "assignees") - reviewers := parseParticipantsFromConfig(tc.input, "reviewers") - - // Both should return the same results - if (assignees == nil) != (reviewers == nil) { - t.Errorf("nil mismatch: assignees=%v, reviewers=%v", assignees, reviewers) - return - } - - if assignees == nil { - return - } - - if len(assignees) != len(reviewers) { - t.Errorf("length mismatch: assignees=%d, reviewers=%d", len(assignees), len(reviewers)) - return - } - - for i := range assignees { - if assignees[i] != reviewers[i] { - t.Errorf("value mismatch at index %d: assignees=%q, reviewers=%q", i, assignees[i], reviewers[i]) - } - } - }) - } -} - -func TestParseIntFromConfig(t *testing.T) { - tests := []struct { - name string - input map[string]any - key string - expected int - }{ - { - name: "valid int value", - input: map[string]any{ - "my-key": 42, - }, - key: "my-key", - expected: 42, - }, - { - name: "valid int64 value", - input: map[string]any{ - "my-key": int64(100), - }, - key: "my-key", - expected: 100, - }, - { - name: "valid float64 value", - input: map[string]any{ - "my-key": float64(75.5), - }, - key: "my-key", - expected: 75, - }, - { - name: "valid uint64 value", - input: map[string]any{ - "my-key": uint64(200), - }, - key: "my-key", - expected: 200, - }, - { - name: "uint64 overflow - should return 0", - input: map[string]any{ - "my-key": ^uint64(0), // Max uint64 value - }, - key: "my-key", - expected: 0, - }, - { - name: "zero value", - input: map[string]any{ - "my-key": 0, - }, - key: "my-key", - expected: 0, - }, - { - name: "negative value", - input: map[string]any{ - "my-key": -10, - }, - key: "my-key", - expected: -10, - }, - { - name: "missing key", - input: map[string]any{}, - key: "my-key", - expected: 0, - }, - { - name: "non-numeric type (string)", - input: map[string]any{ - "my-key": "123", - }, - key: "my-key", - expected: 0, - }, - { - name: "non-numeric type (bool)", - input: map[string]any{ - "my-key": true, - }, - key: "my-key", - expected: 0, - }, - { - name: "non-numeric type (array)", - input: map[string]any{ - "my-key": []int{1, 2, 3}, - }, - key: "my-key", - expected: 0, - }, - { - name: "nil value", - input: map[string]any{ - "my-key": nil, - }, - key: "my-key", - expected: 0, - }, - { - name: "different keys with different values", - input: map[string]any{ - "key1": 10, - "key2": 20, - }, - key: "key2", - expected: 20, - }, - { - name: "large int value", - input: map[string]any{ - "my-key": 999999, - }, - key: "my-key", - expected: 999999, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := ParseIntFromConfig(tt.input, tt.key, nil) - if result != tt.expected { - t.Errorf("expected %d, got %d", tt.expected, result) - } - }) - } -} - func TestParseBoolFromConfig(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/error_helpers.go b/pkg/workflow/error_helpers.go index 9c93c0e8a56..f124753dde2 100644 --- a/pkg/workflow/error_helpers.go +++ b/pkg/workflow/error_helpers.go @@ -160,40 +160,3 @@ func NewConfigurationError(configKey, value, reason, suggestion string) *Configu Timestamp: time.Now(), } } - -// EnhanceError adds context to an existing error -func EnhanceError(err error, context, suggestion string) error { - if err == nil { - return nil - } - - errorHelpersLog.Printf("Enhancing error with context: context=%s", context) - - timestamp := time.Now().Format(time.RFC3339) - - var b strings.Builder - fmt.Fprintf(&b, "[%s] %s", timestamp, context) - fmt.Fprintf(&b, "\n\nOriginal error: %v", err) - - if suggestion != "" { - fmt.Fprintf(&b, "\nSuggestion: %s", suggestion) - } - - return fmt.Errorf("%s", b.String()) -} - -// WrapErrorWithContext wraps an error with additional context using fmt.Errorf %w -// This preserves error unwrapping while adding context -func WrapErrorWithContext(err error, context, suggestion string) error { - if err == nil { - return nil - } - - timestamp := time.Now().Format(time.RFC3339) - - if suggestion != "" { - return fmt.Errorf("[%s] %s (suggestion: %s): %w", timestamp, context, suggestion, err) - } - - return fmt.Errorf("[%s] %s: %w", timestamp, context, err) -} diff --git a/pkg/workflow/error_helpers_test.go b/pkg/workflow/error_helpers_test.go index 57dea8e9b79..e15b89d197a 100644 --- a/pkg/workflow/error_helpers_test.go +++ b/pkg/workflow/error_helpers_test.go @@ -111,58 +111,3 @@ func TestConfigurationError(t *testing.T) { assert.Contains(t, err.Error(), "...") }) } - -func TestEnhanceError(t *testing.T) { - t.Run("enhance error with context", func(t *testing.T) { - originalErr := errors.New("original message") - enhanced := EnhanceError(originalErr, "processing workflow", "Check your configuration") - - require.Error(t, enhanced) - assert.Contains(t, enhanced.Error(), "processing workflow") - assert.Contains(t, enhanced.Error(), "Original error: original message") - assert.Contains(t, enhanced.Error(), "Suggestion: Check your configuration") - }) - - t.Run("enhance nil error returns nil", func(t *testing.T) { - enhanced := EnhanceError(nil, "context", "suggestion") - assert.NoError(t, enhanced) - }) - - t.Run("enhance error includes timestamp", func(t *testing.T) { - originalErr := errors.New("test") - enhanced := EnhanceError(originalErr, "test context", "") - - require.Error(t, enhanced) - assert.Contains(t, enhanced.Error(), "[") - assert.Contains(t, enhanced.Error(), "T") - }) -} - -func TestWrapErrorWithContext(t *testing.T) { - t.Run("wrap error with context", func(t *testing.T) { - originalErr := errors.New("original") - wrapped := WrapErrorWithContext(originalErr, "failed to compile", "Check syntax") - - require.Error(t, wrapped) - assert.Contains(t, wrapped.Error(), "failed to compile") - assert.Contains(t, wrapped.Error(), "Check syntax") - - // Should be unwrappable - unwrapped := errors.Unwrap(wrapped) - assert.Equal(t, originalErr, unwrapped) - }) - - t.Run("wrap nil error returns nil", func(t *testing.T) { - wrapped := WrapErrorWithContext(nil, "context", "suggestion") - assert.NoError(t, wrapped) - }) - - t.Run("wrap error without suggestion", func(t *testing.T) { - originalErr := errors.New("original") - wrapped := WrapErrorWithContext(originalErr, "processing", "") - - require.Error(t, wrapped) - assert.Contains(t, wrapped.Error(), "processing") - assert.NotContains(t, wrapped.Error(), "suggestion:") - }) -} diff --git a/pkg/workflow/markdown_security_scanner.go b/pkg/workflow/markdown_security_scanner.go index ef01e21557d..2a380996fa6 100644 --- a/pkg/workflow/markdown_security_scanner.go +++ b/pkg/workflow/markdown_security_scanner.go @@ -60,14 +60,6 @@ type SecurityFinding struct { Snippet string // Short excerpt of the problematic content } -// String returns a human-readable description of the finding -func (f SecurityFinding) String() string { - if f.Line > 0 { - return fmt.Sprintf("[%s] line %d: %s", f.Category, f.Line, f.Description) - } - return fmt.Sprintf("[%s] %s", f.Category, f.Description) -} - // countCategories counts unique security finding categories func countCategories(findings []SecurityFinding) int { categories := make(map[SecurityFindingCategory]bool) diff --git a/pkg/workflow/markdown_security_scanner_test.go b/pkg/workflow/markdown_security_scanner_test.go index 69fdade36e7..dc095a72f59 100644 --- a/pkg/workflow/markdown_security_scanner_test.go +++ b/pkg/workflow/markdown_security_scanner_test.go @@ -819,40 +819,6 @@ func TestFormatSecurityFindings_Multiple(t *testing.T) { assert.Contains(t, result, "cannot be added", "should mention rejection") } -func TestSecurityFinding_String(t *testing.T) { - tests := []struct { - name string - finding SecurityFinding - expected string - }{ - { - name: "with line number", - finding: SecurityFinding{ - Category: CategoryUnicodeAbuse, - Description: "test description", - Line: 42, - }, - expected: "[unicode-abuse] line 42: test description", - }, - { - name: "without line number", - finding: SecurityFinding{ - Category: CategoryHTMLAbuse, - Description: "test description", - Line: 0, - }, - expected: "[html-abuse] test description", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := tt.finding.String() - assert.Equal(t, tt.expected, result, "String() output should match") - }) - } -} - func TestTruncateSnippet(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/metrics.go b/pkg/workflow/metrics.go index 2a3044115cf..0c44b198ac1 100644 --- a/pkg/workflow/metrics.go +++ b/pkg/workflow/metrics.go @@ -257,19 +257,6 @@ func PrettifyToolName(toolName string) string { return toolName } -// ExtractMCPServer extracts the MCP server name from a tool name -func ExtractMCPServer(toolName string) string { - // For MCP tools with pattern "mcp__server__method", extract "server" - if strings.HasPrefix(toolName, "mcp__") { - parts := strings.Split(toolName, "__") - if len(parts) >= 2 { - return parts[1] - } - } - // For non-MCP tools, return the tool name as-is - return toolName -} - // FinalizeToolMetricsOptions holds the options for FinalizeToolMetrics type FinalizeToolMetricsOptions struct { Metrics *LogMetrics diff --git a/pkg/workflow/permissions_validation.go b/pkg/workflow/permissions_validation.go index 146739a2c2f..b62d39e49c8 100644 --- a/pkg/workflow/permissions_validation.go +++ b/pkg/workflow/permissions_validation.go @@ -73,16 +73,6 @@ func init() { permissionsValidationLog.Printf("Loaded %d GitHub toolsets from JSON", len(toolsetPermissionsMap)) } -// GetToolsetsData returns the parsed GitHub toolsets data (for use by workflows) -func GetToolsetsData() GitHubToolsetsData { - var data GitHubToolsetsData - if err := json.Unmarshal(githubToolsetsPermissionsJSON, &data); err != nil { - // This should never happen as we validate in init - panic(fmt.Sprintf("failed to parse GitHub toolsets data: %v", err)) - } - return data -} - // ValidatableTool represents a tool configuration that can be validated for permissions // This interface abstracts the tool configuration structure to enable type-safe permission validation type ValidatableTool interface { diff --git a/pkg/workflow/permissions_validator_json_test.go b/pkg/workflow/permissions_validator_json_test.go index 1cdf10ca319..74cafdf9857 100644 --- a/pkg/workflow/permissions_validator_json_test.go +++ b/pkg/workflow/permissions_validator_json_test.go @@ -49,34 +49,3 @@ func TestToolsetPermissionsLoadedFromJSON(t *testing.T) { t.Error("context toolset should have tools listed") } } - -func TestGetToolsetsData(t *testing.T) { - // Test that GetToolsetsData returns valid data - data := GetToolsetsData() - - if data.Version == "" { - t.Error("GetToolsetsData should return data with version") - } - - if len(data.Toolsets) == 0 { - t.Error("GetToolsetsData should return toolsets") - } - - // Test specific toolset has expected structure - repos, exists := data.Toolsets["repos"] - if !exists { - t.Fatal("repos toolset not found in GetToolsetsData") - } - - if repos.Description == "" { - t.Error("repos toolset should have description") - } - - if len(repos.Tools) == 0 { - t.Error("repos toolset should have tools") - } - - if len(repos.ReadPermissions) == 0 { - t.Error("repos toolset should have read permissions") - } -} diff --git a/pkg/workflow/safe_jobs.go b/pkg/workflow/safe_jobs.go index 881d5715af3..11f62072c58 100644 --- a/pkg/workflow/safe_jobs.go +++ b/pkg/workflow/safe_jobs.go @@ -30,11 +30,6 @@ type SafeJobConfig struct { Output string `yaml:"output,omitempty"` } -// HasSafeJobsEnabled checks if any safe-jobs are enabled at the top level -func HasSafeJobsEnabled(safeJobs map[string]*SafeJobConfig) bool { - return len(safeJobs) > 0 -} - // parseSafeJobsConfig parses safe-jobs configuration from a jobs map. // This function expects a map of job configurations directly (from safe-outputs.jobs). // The top-level "safe-jobs" key is NOT supported - only "safe-outputs.jobs" is valid. diff --git a/pkg/workflow/safe_jobs_test.go b/pkg/workflow/safe_jobs_test.go index 8a9793dd54d..c4c5c17dc2e 100644 --- a/pkg/workflow/safe_jobs_test.go +++ b/pkg/workflow/safe_jobs_test.go @@ -143,28 +143,6 @@ func TestParseSafeJobsConfig(t *testing.T) { } } -func TestHasSafeJobsEnabled(t *testing.T) { - // Test that safe-jobs are detected by HasSafeJobsEnabled - safeJobs := map[string]*SafeJobConfig{ - "deploy": { - RunsOn: "ubuntu-latest", - }, - } - - if !HasSafeJobsEnabled(safeJobs) { - t.Error("Expected HasSafeJobsEnabled to return true when safe-jobs are configured") - } - - // Test empty safe-jobs - if HasSafeJobsEnabled(nil) { - t.Error("Expected HasSafeJobsEnabled to return false when safe-jobs are nil") - } - - if HasSafeJobsEnabled(map[string]*SafeJobConfig{}) { - t.Error("Expected HasSafeJobsEnabled to return false when safe-jobs are empty") - } -} - func TestBuildSafeJobs(t *testing.T) { c := NewCompiler() diff --git a/pkg/workflow/tools_validation.go b/pkg/workflow/tools_validation.go index 445e59bbf13..24c2bb7dd17 100644 --- a/pkg/workflow/tools_validation.go +++ b/pkg/workflow/tools_validation.go @@ -27,50 +27,6 @@ func validateBashToolConfig(tools *Tools, workflowName string) error { return nil } -// isGitToolAllowed checks if git commands are allowed in bash tool configuration -func isGitToolAllowed(tools *Tools) bool { - if tools == nil { - // No tools configured - defaults will be applied which include git for PR operations - return true - } - - if tools.Bash == nil { - // No bash tool configured - defaults will be applied which include git for PR operations - return true - } - - // If AllowedCommands is nil or empty, check which case it is: - // - nil AllowedCommands = bash: true (all commands allowed, including git) - // - empty slice = bash: false (explicitly disabled) - if tools.Bash.AllowedCommands == nil { - // bash: true - all commands allowed - return true - } - - if len(tools.Bash.AllowedCommands) == 0 { - // bash: false or bash: [] - explicitly disabled or no commands - return false - } - - // Check if git is in the allowed commands list - for _, cmd := range tools.Bash.AllowedCommands { - if cmd == "*" { - // Wildcard allows all commands - return true - } - if cmd == "git" { - // Exact match for git command - return true - } - // Check for git with wildcards: "git *", "git:*", "git checkout:*", etc. - if strings.HasPrefix(cmd, "git ") || strings.HasPrefix(cmd, "git:") { - return true - } - } - - return false -} - // validateGitHubReadOnly validates that read-only: false is not set for the GitHub tool. // The GitHub MCP server always operates in read-only mode; write access is not permitted. func validateGitHubReadOnly(tools *Tools, workflowName string) error { diff --git a/pkg/workflow/tools_validation_test.go b/pkg/workflow/tools_validation_test.go index bc35f37d13f..432faa366e5 100644 --- a/pkg/workflow/tools_validation_test.go +++ b/pkg/workflow/tools_validation_test.go @@ -154,124 +154,6 @@ func TestNewToolsWithInvalidBash(t *testing.T) { }) } -func TestIsGitToolAllowed(t *testing.T) { - tests := []struct { - name string - toolsMap map[string]any - expectGitOK bool - description string - }{ - { - name: "nil tools - git allowed (defaults will apply)", - toolsMap: nil, - expectGitOK: true, - description: "No tools configuration means defaults will apply which include git for PR operations", - }, - { - name: "no bash tool - git allowed (defaults will apply)", - toolsMap: map[string]any{}, - expectGitOK: true, - description: "No bash tool configured means defaults will apply which include git for PR operations", - }, - { - name: "bash: true - git allowed", - toolsMap: map[string]any{ - "bash": true, - }, - expectGitOK: true, - description: "bash: true allows all commands including git", - }, - { - name: "bash: false - git not allowed", - toolsMap: map[string]any{ - "bash": false, - }, - expectGitOK: false, - description: "bash: false explicitly disables bash", - }, - { - name: "bash with empty list - git not allowed", - toolsMap: map[string]any{ - "bash": []any{}, - }, - expectGitOK: false, - description: "Empty command list means no commands allowed", - }, - { - name: "bash with git command - git allowed", - toolsMap: map[string]any{ - "bash": []any{"git"}, - }, - expectGitOK: true, - description: "git explicitly in allowed commands", - }, - { - name: "bash with git and other commands - git allowed", - toolsMap: map[string]any{ - "bash": []any{"echo", "git", "ls"}, - }, - expectGitOK: true, - description: "git in allowed commands list", - }, - { - name: "bash with wildcard - git allowed", - toolsMap: map[string]any{ - "bash": []any{"*"}, - }, - expectGitOK: true, - description: "Wildcard allows all commands including git", - }, - { - name: "bash with wildcard and other commands - git allowed", - toolsMap: map[string]any{ - "bash": []any{"echo", "*"}, - }, - expectGitOK: true, - description: "Wildcard in list allows all commands", - }, - { - name: "bash with git and space wildcard - git allowed", - toolsMap: map[string]any{ - "bash": []any{"git *"}, - }, - expectGitOK: true, - description: "git * pattern allows all git commands", - }, - { - name: "bash with git colon wildcard - git allowed", - toolsMap: map[string]any{ - "bash": []any{"git:*"}, - }, - expectGitOK: true, - description: "git:* pattern allows all git commands", - }, - { - name: "bash with git subcommand wildcard - git allowed", - toolsMap: map[string]any{ - "bash": []any{"git checkout:*", "git status"}, - }, - expectGitOK: true, - description: "git :* pattern allows git commands", - }, - { - name: "bash with other commands only - git not allowed", - toolsMap: map[string]any{ - "bash": []any{"echo", "ls", "cat"}, - }, - expectGitOK: false, - description: "git not in allowed commands list", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tools := NewTools(tt.toolsMap) - result := isGitToolAllowed(tools) - assert.Equal(t, tt.expectGitOK, result, tt.description) - }) - } -} - // Note: TestValidateGitToolForSafeOutputs was removed because the validation function // was removed. Git commands are automatically injected by the compiler when safe-outputs // needs them (see compiler_safe_outputs.go), so validation was misleading and unnecessary.