diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 85b716b600..060f2b9258 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -198,6 +198,32 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath return formatCompilerError(markdownPath, "error", err.Error(), err) } + // Validate workflow-level concurrency group expression + log.Printf("Validating workflow-level concurrency configuration") + if workflowData.Concurrency != "" { + // Extract the group expression from the concurrency YAML + // The Concurrency field contains the full YAML (e.g., "concurrency:\n group: \"...\"") + // We need to extract just the group value + groupExpr := extractConcurrencyGroupFromYAML(workflowData.Concurrency) + if groupExpr != "" { + if err := validateConcurrencyGroupExpression(groupExpr); err != nil { + return formatCompilerError(markdownPath, "error", fmt.Sprintf("workflow-level concurrency validation failed: %s", err.Error()), err) + } + } + } + + // Validate engine-level concurrency group expression + log.Printf("Validating engine-level concurrency configuration") + if workflowData.EngineConfig != nil && workflowData.EngineConfig.Concurrency != "" { + // Extract the group expression from the engine concurrency YAML + groupExpr := extractConcurrencyGroupFromYAML(workflowData.EngineConfig.Concurrency) + if groupExpr != "" { + if err := validateConcurrencyGroupExpression(groupExpr); err != nil { + return formatCompilerError(markdownPath, "error", fmt.Sprintf("engine.concurrency validation failed: %s", err.Error()), err) + } + } + } + // Emit experimental warning for sandbox-runtime feature if isSRTEnabled(workflowData) { fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: sandbox-runtime firewall")) diff --git a/pkg/workflow/concurrency_validation.go b/pkg/workflow/concurrency_validation.go new file mode 100644 index 0000000000..4fcf80b010 --- /dev/null +++ b/pkg/workflow/concurrency_validation.go @@ -0,0 +1,346 @@ +// This file provides validation for custom concurrency group expressions. +// +// # Concurrency Group Expression Validation +// +// This file validates that custom concurrency group expressions specified by users +// have correct syntax and can be safely compiled into GitHub Actions workflows. +// +// # Validation Functions +// +// - validateConcurrencyGroupExpression() - Validates syntax of a single group expression +// - extractGroupExpression() - Extracts group value from concurrency configuration +// +// # Validation Coverage +// +// The validation detects common syntactic errors at compile time: +// - Unbalanced ${{ }} braces +// - Missing closing braces +// - Malformed GitHub Actions expressions +// - Invalid logical operators placement +// - Unclosed parentheses or quotes +// +// # When to Add Validation Here +// +// Add validation to this file when: +// - Adding new concurrency group syntax checks +// - Detecting new types of expression syntax errors +// - Improving error messages for concurrency configuration + +package workflow + +import ( + "fmt" + "regexp" + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var concurrencyValidationLog = logger.New("workflow:concurrency_validation") + +// validateConcurrencyGroupExpression validates the syntax of a custom concurrency group expression. +// It checks for common syntactic errors that would cause runtime failures: +// - Unbalanced ${{ }} braces +// - Missing closing braces +// - Malformed GitHub Actions expressions +// - Invalid operator placement +// +// Returns an error if validation fails, nil otherwise. +func validateConcurrencyGroupExpression(group string) error { + if strings.TrimSpace(group) == "" { + return NewValidationError( + "concurrency", + "empty concurrency group expression", + "the concurrency group expression is empty or contains only whitespace", + "Provide a non-empty concurrency group name or expression. Example: 'my-workflow-${{ github.ref }}'", + ) + } + + concurrencyValidationLog.Printf("Validating concurrency group expression: %s", group) + + // Check for balanced ${{ }} braces + if err := validateBalancedBraces(group); err != nil { + return err + } + + // Extract and validate each GitHub Actions expression within ${{ }} + if err := validateExpressionSyntax(group); err != nil { + return err + } + + concurrencyValidationLog.Print("Concurrency group expression validation passed") + return nil +} + +// validateBalancedBraces checks that all ${{ }} braces are balanced and properly closed +func validateBalancedBraces(group string) error { + openCount := 0 + i := 0 + positions := []int{} // Track positions of opening braces for error reporting + + for i < len(group) { + // Check for opening ${{ + if i+2 < len(group) && group[i:i+3] == "${{" { + openCount++ + positions = append(positions, i) + i += 3 + continue + } + + // Check for closing }} + if i+1 < len(group) && group[i:i+2] == "}}" { + if openCount == 0 { + return NewValidationError( + "concurrency", + "unbalanced closing braces", + fmt.Sprintf("found '}}' at position %d without matching opening '${{' in expression: %s", i, group), + "Ensure all '}}' have a corresponding opening '${{'. Check for typos or missing opening braces.", + ) + } + openCount-- + if len(positions) > 0 { + positions = positions[:len(positions)-1] + } + i += 2 + continue + } + + i++ + } + + if openCount > 0 { + // Find the position of the first unclosed opening brace + pos := positions[0] + return NewValidationError( + "concurrency", + "unclosed expression braces", + fmt.Sprintf("found opening '${{' at position %d without matching closing '}}' in expression: %s", pos, group), + "Ensure all '${{' have a corresponding closing '}}'. Add the missing closing braces.", + ) + } + + return nil +} + +// validateExpressionSyntax validates the syntax of expressions within ${{ }} +func validateExpressionSyntax(group string) error { + // Pattern to extract content between ${{ and }} + expressionPattern := regexp.MustCompile(`\$\{\{([^}]*)\}\}`) + matches := expressionPattern.FindAllStringSubmatch(group, -1) + + for _, match := range matches { + if len(match) < 2 { + continue + } + + exprContent := strings.TrimSpace(match[1]) + if exprContent == "" { + return NewValidationError( + "concurrency", + "empty expression content", + fmt.Sprintf("found empty expression '${{ }}' in concurrency group: %s", group), + "Provide a valid GitHub Actions expression inside '${{ }}'. Example: '${{ github.ref }}'", + ) + } + + // Check for common syntax errors + if err := validateExpressionContent(exprContent, group); err != nil { + return err + } + } + + return nil +} + +// validateExpressionContent validates the content inside ${{ }} +func validateExpressionContent(expr string, fullGroup string) error { + // Check for unbalanced parentheses + parenCount := 0 + for i, ch := range expr { + switch ch { + case '(': + parenCount++ + case ')': + parenCount-- + if parenCount < 0 { + return NewValidationError( + "concurrency", + "unbalanced parentheses in expression", + fmt.Sprintf("found closing ')' without matching opening '(' at position %d in expression: %s", i, expr), + "Ensure all parentheses are properly balanced in your concurrency group expression.", + ) + } + } + } + + if parenCount > 0 { + return NewValidationError( + "concurrency", + "unclosed parentheses in expression", + fmt.Sprintf("found %d unclosed opening '(' in expression: %s", parenCount, expr), + "Add the missing closing ')' to balance parentheses in your expression.", + ) + } + + // Check for unbalanced quotes (single, double, backtick) + if err := validateBalancedQuotes(expr); err != nil { + return err + } + + // Try to parse complex expressions with logical operators + if containsLogicalOperators(expr) { + if _, err := ParseExpression(expr); err != nil { + return NewValidationError( + "concurrency", + "invalid expression syntax", + fmt.Sprintf("failed to parse expression in concurrency group: %s", err.Error()), + fmt.Sprintf("Fix the syntax error in your concurrency group expression. Full expression: %s", fullGroup), + ) + } + } + + return nil +} + +// validateBalancedQuotes checks for balanced quotes in an expression +func validateBalancedQuotes(expr string) error { + inSingleQuote := false + inDoubleQuote := false + inBacktick := false + escaped := false + + for i, ch := range expr { + if escaped { + escaped = false + continue + } + + if ch == '\\' { + escaped = true + continue + } + + switch ch { + case '\'': + if !inDoubleQuote && !inBacktick { + inSingleQuote = !inSingleQuote + } + case '"': + if !inSingleQuote && !inBacktick { + inDoubleQuote = !inDoubleQuote + } + case '`': + if !inSingleQuote && !inDoubleQuote { + inBacktick = !inBacktick + } + } + + // Check if we reached end of string with unclosed quote + if i == len(expr)-1 { + if inSingleQuote { + return NewValidationError( + "concurrency", + "unclosed single quote", + fmt.Sprintf("found unclosed single quote in expression: %s", expr), + "Add the missing closing single quote (') to your expression.", + ) + } + if inDoubleQuote { + return NewValidationError( + "concurrency", + "unclosed double quote", + fmt.Sprintf("found unclosed double quote in expression: %s", expr), + "Add the missing closing double quote (\") to your expression.", + ) + } + if inBacktick { + return NewValidationError( + "concurrency", + "unclosed backtick", + fmt.Sprintf("found unclosed backtick in expression: %s", expr), + "Add the missing closing backtick (`) to your expression.", + ) + } + } + } + + return nil +} + +// containsLogicalOperators checks if an expression contains logical operators (&&, ||, !) +// Note: This is a simple string-based check that may return true for expressions containing +// '!=' (not equals) since it includes the '!' character. This is acceptable because the +// function is used to decide whether to parse the expression with the expression parser, +// and expressions with '!=' will be successfully parsed by the parser. +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: +// +// concurrency: "group-name" # string format +// +// or +// +// concurrency: +// group: "group-name" # object format +// cancel-in-progress: true # optional +// +// Returns the group value string or empty string if not found. +// +// Note: This function uses a regex pattern that stops at the first quote or newline character. +// Group values containing embedded quotes or newlines will be truncated at that point. However, +// such values are rare in concurrency group expressions, and any resulting syntax errors will be +// caught by the subsequent expression validation. +func extractConcurrencyGroupFromYAML(concurrencyYAML string) string { + // First, check if it's object format with explicit "group:" field + // Pattern: group: "value" or group: 'value' or group: value (at start of line or after spaces) + groupPattern := regexp.MustCompile(`(?m)^\s*group:\s*["']?([^"'\n]+?)["']?\s*$`) + matches := groupPattern.FindStringSubmatch(concurrencyYAML) + if len(matches) > 1 { + return strings.TrimSpace(matches[1]) + } + + // If no explicit "group:" field, it might be string format: concurrency: "value" + // Pattern: concurrency: "value" or concurrency: 'value' or concurrency: value + // Must be on the first line (not indented, not preceded by other content) + lines := strings.Split(concurrencyYAML, "\n") + if len(lines) > 0 { + firstLine := strings.TrimSpace(lines[0]) + // Only match if it starts with "concurrency:" + if strings.HasPrefix(firstLine, "concurrency:") { + value := strings.TrimSpace(strings.TrimPrefix(firstLine, "concurrency:")) + // Remove quotes if present + value = strings.Trim(value, `"'`) + return value + } + } + + return "" +} diff --git a/pkg/workflow/concurrency_validation_integration_test.go b/pkg/workflow/concurrency_validation_integration_test.go new file mode 100644 index 0000000000..4cc8e62232 --- /dev/null +++ b/pkg/workflow/concurrency_validation_integration_test.go @@ -0,0 +1,339 @@ +//go:build integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConcurrencyGroupValidationIntegration(t *testing.T) { + tmpDir := testutil.TempDir(t, "concurrency-validation-integration") + compiler := NewCompiler() + + tests := []struct { + name string + frontmatter string + content string + expectError bool + errorSubstr string // Expected substring in error message + description string + }{ + // Valid workflow-level concurrency + { + name: "valid workflow-level concurrency string", + frontmatter: `--- +on: push +concurrency: "my-workflow-${{ github.ref }}" +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with valid workflow-level concurrency.", + expectError: false, + description: "Valid workflow-level concurrency string should compile", + }, + { + name: "valid workflow-level concurrency object", + frontmatter: `--- +on: pull_request +concurrency: + group: "pr-${{ github.event.pull_request.number || github.ref }}" + cancel-in-progress: true +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with valid workflow-level concurrency object.", + expectError: false, + description: "Valid workflow-level concurrency object should compile", + }, + + // Valid engine-level concurrency + { + name: "valid engine-level concurrency string", + frontmatter: `--- +on: push +engine: + id: copilot + concurrency: "copilot-${{ github.workflow }}" +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with valid engine-level concurrency string.", + expectError: false, + description: "Valid engine-level concurrency string should compile", + }, + { + name: "valid engine-level concurrency object", + frontmatter: `--- +on: push +engine: + id: copilot + concurrency: + group: "copilot-${{ github.workflow }}-${{ github.ref }}" +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with valid engine-level concurrency object.", + expectError: false, + description: "Valid engine-level concurrency object should compile", + }, + + // Invalid workflow-level concurrency + { + name: "invalid workflow-level concurrency - unclosed braces", + frontmatter: `--- +on: push +concurrency: workflow-${{ github.ref +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with invalid workflow-level concurrency.", + expectError: true, + errorSubstr: "unclosed expression braces", + description: "Unclosed braces in workflow-level concurrency should fail", + }, + { + name: "invalid workflow-level concurrency - empty expression", + frontmatter: `--- +on: push +concurrency: workflow-${{}} +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with empty expression in workflow-level concurrency.", + expectError: true, + errorSubstr: "empty expression content", + description: "Empty expression in workflow-level concurrency should fail", + }, + { + name: "invalid workflow-level concurrency - unbalanced parentheses", + frontmatter: `--- +on: push +concurrency: + group: workflow-${{ (github.workflow }} +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with unbalanced parentheses.", + expectError: true, + errorSubstr: "unclosed parentheses", + description: "Unbalanced parentheses in workflow-level concurrency should fail", + }, + + // Invalid engine-level concurrency + { + name: "invalid engine-level concurrency - unclosed braces", + frontmatter: `--- +on: push +engine: + id: copilot + concurrency: copilot-${{ github.workflow +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with invalid engine-level concurrency.", + expectError: true, + errorSubstr: "unclosed expression braces", + description: "Unclosed braces in engine-level concurrency should fail", + }, + { + name: "invalid engine-level concurrency - malformed operators", + frontmatter: `--- +on: push +engine: + id: copilot + concurrency: + group: copilot-${{ github.workflow && && github.ref }} +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with malformed operators in engine-level concurrency.", + expectError: true, + errorSubstr: "invalid expression syntax", + description: "Malformed operators in engine-level concurrency should fail", + }, + + // Both levels with mixed validity + { + name: "valid workflow-level, invalid engine-level", + frontmatter: `--- +on: push +concurrency: workflow-${{ github.ref }} +engine: + id: copilot + concurrency: copilot-${{ github.workflow +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with valid workflow-level but invalid engine-level concurrency.", + expectError: true, + errorSubstr: "engine.concurrency validation failed", + description: "Should fail on invalid engine-level concurrency even if workflow-level is valid", + }, + { + name: "invalid workflow-level, valid engine-level", + frontmatter: `--- +on: push +concurrency: workflow-${{ github.ref +engine: + id: copilot + concurrency: copilot-${{ github.workflow }} +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with invalid workflow-level but valid engine-level concurrency.", + expectError: true, + errorSubstr: "workflow-level concurrency validation failed", + description: "Should fail on invalid workflow-level concurrency even if engine-level is valid", + }, + + // Edge cases + { + name: "complex valid expression", + frontmatter: `--- +on: push +concurrency: + group: workflow-${{ (github.workflow || github.ref) && github.repository }} +tools: + github: + allowed: [list_issues] +---`, + content: "Test workflow with complex valid expression.", + expectError: false, + description: "Complex valid expression should compile", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Create markdown file + markdown := tt.frontmatter + "\n" + tt.content + mdPath := filepath.Join(tmpDir, tt.name+".md") + err := os.WriteFile(mdPath, []byte(markdown), 0644) + require.NoError(t, err, "Failed to write test markdown file") + + // Try to compile + err = compiler.CompileWorkflow(mdPath) + + if tt.expectError { + assert.Error(t, err, "Expected error for: %s", tt.description) + if tt.errorSubstr != "" { + assert.Contains(t, err.Error(), tt.errorSubstr, + "Error should contain expected substring for: %s", tt.description) + } + } else { + assert.NoError(t, err, "Expected no error for: %s. Got: %v", tt.description, err) + } + + // Clean up lock file if created + lockPath := mdPath[:len(mdPath)-3] + ".lock.yml" + _ = os.Remove(lockPath) + }) + } +} + +// TestConcurrencyGroupValidationWithRealWorkflows tests validation with real workflow patterns +func TestConcurrencyGroupValidationWithRealWorkflows(t *testing.T) { + tmpDir := testutil.TempDir(t, "concurrency-validation-real-workflows") + compiler := NewCompiler() + + // Test with real concurrency patterns used in the codebase + realPatterns := []struct { + name string + frontmatter string + description string + }{ + { + name: "pr-workflow-pattern", + frontmatter: `--- +on: + pull_request: + types: [opened, synchronize] +concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}" + cancel-in-progress: true +tools: + github: + allowed: [list_issues] +---`, + description: "Real PR workflow concurrency pattern", + }, + { + name: "issue-workflow-pattern", + frontmatter: `--- +on: + issues: + types: [opened] +concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}" +tools: + github: + allowed: [list_issues] +---`, + description: "Real issue workflow concurrency pattern", + }, + { + name: "command-workflow-pattern", + frontmatter: `--- +on: + command: + name: test-bot +concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}" +tools: + github: + allowed: [list_issues] +---`, + description: "Real command workflow concurrency pattern", + }, + { + name: "engine-level-pattern", + frontmatter: `--- +on: + workflow_dispatch: +engine: + id: copilot + concurrency: + group: "gh-aw-copilot-${{ github.workflow }}" +tools: + github: + allowed: [list_issues] +---`, + description: "Real engine-level concurrency pattern", + }, + } + + for _, pattern := range realPatterns { + t.Run(pattern.name, func(t *testing.T) { + // Create markdown file + markdown := pattern.frontmatter + "\nTest workflow content." + mdPath := filepath.Join(tmpDir, pattern.name+".md") + err := os.WriteFile(mdPath, []byte(markdown), 0644) + require.NoError(t, err, "Failed to write test markdown file") + + // Should compile successfully + err = compiler.CompileWorkflow(mdPath) + assert.NoError(t, err, "Real workflow pattern should compile: %s. Got: %v", pattern.description, err) + + // Clean up + lockPath := mdPath[:len(mdPath)-3] + ".lock.yml" + _ = os.Remove(lockPath) + }) + } +} diff --git a/pkg/workflow/concurrency_validation_test.go b/pkg/workflow/concurrency_validation_test.go new file mode 100644 index 0000000000..98d40c6394 --- /dev/null +++ b/pkg/workflow/concurrency_validation_test.go @@ -0,0 +1,818 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestValidateConcurrencyGroupExpression(t *testing.T) { + tests := []struct { + name string + group string + wantErr bool + errorInMsg string // expected substring in error message + description string + }{ + // Valid expressions + { + name: "simple static group", + group: "my-workflow", + wantErr: false, + description: "Simple string without expressions should be valid", + }, + { + name: "group with github.ref", + group: "workflow-${{ github.ref }}", + wantErr: false, + description: "Valid GitHub expression should pass", + }, + { + name: "group with github.workflow", + group: "gh-aw-${{ github.workflow }}", + wantErr: false, + description: "Valid GitHub workflow expression should pass", + }, + { + name: "group with event number", + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}", + wantErr: false, + description: "Multiple expressions should be valid", + }, + { + name: "group with OR operator", + group: "gh-aw-${{ github.event.pull_request.number || github.ref }}", + wantErr: false, + description: "Expression with OR operator should parse correctly", + }, + { + name: "group with complex expression", + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}", + wantErr: false, + description: "Complex expression with multiple OR operators should be valid", + }, + { + name: "group with AND operator", + group: "test-${{ github.workflow && github.repository }}", + wantErr: false, + description: "Expression with AND operator should parse correctly", + }, + { + name: "group with NOT operator", + group: "test-${{ !github.event.issue }}", + wantErr: false, + description: "Expression with NOT operator should parse correctly", + }, + { + name: "group with parentheses", + group: "test-${{ (github.workflow || github.ref) && github.repository }}", + wantErr: false, + description: "Expression with parentheses for precedence should be valid", + }, + + // Invalid expressions - empty/whitespace + { + name: "empty string", + group: "", + wantErr: true, + errorInMsg: "empty concurrency group expression", + description: "Empty string should be rejected", + }, + { + name: "only whitespace", + group: " ", + wantErr: true, + errorInMsg: "empty concurrency group expression", + description: "Whitespace-only string should be rejected", + }, + + // Invalid expressions - unbalanced braces + { + name: "missing closing braces", + group: "workflow-${{ github.ref ", + wantErr: true, + errorInMsg: "unclosed expression braces", + description: "Missing closing }} should be caught", + }, + { + name: "missing opening braces", + group: "workflow-github.ref }}", + wantErr: true, + errorInMsg: "unbalanced closing braces", + description: "Missing opening ${{ should be caught", + }, + { + name: "unbalanced nested braces", + group: "workflow-${{ github.ref }} extra }}", + wantErr: true, + errorInMsg: "unbalanced closing braces", + description: "Extra closing braces should be caught", + }, + { + name: "multiple unclosed braces", + group: "workflow-${{ github.ref }}-${{ github.workflow ", + wantErr: true, + errorInMsg: "unclosed expression braces", + description: "Multiple unclosed expressions should be caught", + }, + + // Invalid expressions - empty expression content + { + name: "empty expression", + group: "workflow-${{ }}", + wantErr: true, + errorInMsg: "empty expression content", + description: "Empty expression content should be rejected", + }, + { + name: "whitespace expression", + group: "workflow-${{ }}", + wantErr: true, + errorInMsg: "empty expression content", + description: "Whitespace-only expression should be rejected", + }, + + // Invalid expressions - unbalanced parentheses + { + name: "unclosed opening parenthesis", + group: "test-${{ (github.workflow }}", + wantErr: true, + errorInMsg: "unclosed parentheses", + description: "Unclosed parenthesis should be caught", + }, + { + name: "extra closing parenthesis", + group: "test-${{ github.workflow) }}", + wantErr: true, + errorInMsg: "unbalanced parentheses", + description: "Extra closing parenthesis should be caught", + }, + { + name: "mismatched parentheses", + group: "test-${{ ((github.workflow) }}", + wantErr: true, + errorInMsg: "unclosed parentheses", + description: "Mismatched parentheses count should be caught", + }, + + // Invalid expressions - unbalanced quotes + { + name: "unclosed single quote", + group: "test-${{ 'unclosed }}", + wantErr: true, + errorInMsg: "unclosed single quote", + description: "Unclosed single quote should be caught", + }, + { + name: "unclosed double quote", + group: "test-${{ \"unclosed }}", + wantErr: true, + errorInMsg: "unclosed double quote", + description: "Unclosed double quote should be caught", + }, + { + name: "unclosed backtick", + group: "test-${{ `unclosed }}", + wantErr: true, + errorInMsg: "unclosed backtick", + description: "Unclosed backtick should be caught", + }, + + // Invalid expressions - malformed logical operators + { + name: "consecutive AND operators", + group: "test-${{ github.workflow && && github.ref }}", + wantErr: true, + errorInMsg: "invalid expression syntax", + description: "Consecutive && operators should be caught", + }, + { + name: "consecutive OR operators", + group: "test-${{ github.workflow || || github.ref }}", + wantErr: true, + errorInMsg: "invalid expression syntax", + description: "Consecutive || operators should be caught", + }, + { + name: "operator at end", + group: "test-${{ github.workflow && }}", + wantErr: true, + errorInMsg: "invalid expression syntax", + description: "Operator at end should be caught", + }, + { + name: "operator at start", + group: "test-${{ && github.workflow }}", + wantErr: true, + errorInMsg: "invalid expression syntax", + description: "Operator at start should be caught", + }, + + // Edge cases + { + name: "multiple valid expressions", + group: "prefix-${{ github.workflow }}-middle-${{ github.ref }}-suffix", + wantErr: false, + description: "Multiple valid expressions with text between should pass", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateConcurrencyGroupExpression(tt.group) + + if tt.wantErr { + require.Error(t, err, "Test case: %s - Expected error but got nil", tt.description) + if tt.errorInMsg != "" { + assert.Contains(t, err.Error(), tt.errorInMsg, + "Error message should contain expected substring for: %s", tt.description) + } + } else { + assert.NoError(t, err, "Test case: %s - Expected no error but got: %v", tt.description, err) + } + }) + } +} + +func TestValidateBalancedBraces(t *testing.T) { + tests := []struct { + name string + input string + wantErr bool + errorInMsg string + }{ + { + name: "balanced single expression", + input: "test-${{ github.ref }}", + wantErr: false, + }, + { + name: "balanced multiple expressions", + input: "test-${{ github.ref }}-${{ github.workflow }}", + wantErr: false, + }, + { + name: "no expressions", + input: "simple-group-name", + wantErr: false, + }, + { + name: "missing closing braces", + input: "test-${{ github.ref", + wantErr: true, + errorInMsg: "unclosed expression braces", + }, + { + name: "extra closing braces", + input: "test-github.ref }}", + wantErr: true, + errorInMsg: "unbalanced closing braces", + }, + { + name: "nested incomplete expression", + input: "test-${{ github.ref }}-${{ incomplete", + wantErr: true, + errorInMsg: "unclosed expression braces", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateBalancedBraces(tt.input) + + if tt.wantErr { + require.Error(t, err, "Expected error for input: %s", tt.input) + if tt.errorInMsg != "" { + assert.Contains(t, err.Error(), tt.errorInMsg) + } + } else { + assert.NoError(t, err, "Expected no error for input: %s", tt.input) + } + }) + } +} + +func TestValidateExpressionContent(t *testing.T) { + tests := []struct { + name string + expr string + fullGroup string + wantErr bool + errorInMsg string + }{ + { + name: "simple property access", + expr: "github.ref", + fullGroup: "test-${{ github.ref }}", + wantErr: false, + }, + { + name: "OR expression", + expr: "github.event.issue.number || github.ref", + fullGroup: "test-${{ github.event.issue.number || github.ref }}", + wantErr: false, + }, + { + name: "expression with parentheses", + expr: "(github.workflow || github.ref) && github.repository", + fullGroup: "test-${{ (github.workflow || github.ref) && github.repository }}", + wantErr: false, + }, + { + name: "unclosed parenthesis", + expr: "(github.workflow", + fullGroup: "test-${{ (github.workflow }}", + wantErr: true, + errorInMsg: "unclosed parentheses", + }, + { + name: "extra closing parenthesis", + expr: "github.workflow)", + fullGroup: "test-${{ github.workflow) }}", + wantErr: true, + errorInMsg: "unbalanced parentheses", + }, + { + name: "unclosed single quote", + expr: "github.workflow == 'test", + fullGroup: "test-${{ github.workflow == 'test }}", + wantErr: true, + errorInMsg: "unclosed single quote", + }, + { + name: "consecutive operators", + expr: "github.workflow && && github.ref", + fullGroup: "test-${{ github.workflow && && github.ref }}", + wantErr: true, + errorInMsg: "invalid expression syntax", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateExpressionContent(tt.expr, tt.fullGroup) + + if tt.wantErr { + require.Error(t, err, "Expected error for expression: %s", tt.expr) + if tt.errorInMsg != "" { + assert.Contains(t, err.Error(), tt.errorInMsg) + } + } else { + assert.NoError(t, err, "Expected no error for expression: %s", tt.expr) + } + }) + } +} + +func TestValidateBalancedQuotes(t *testing.T) { + tests := []struct { + name string + expr string + wantErr bool + errorInMsg string + }{ + { + name: "no quotes", + expr: "github.workflow", + wantErr: false, + }, + { + name: "balanced single quotes", + expr: "github.workflow == 'test'", + wantErr: false, + }, + { + name: "balanced double quotes", + expr: "github.workflow == \"test\"", + wantErr: false, + }, + { + name: "balanced backticks", + expr: "github.workflow == `test`", + wantErr: false, + }, + { + name: "mixed balanced quotes", + expr: "github.workflow == 'test' || github.ref == \"value\"", + wantErr: false, + }, + { + name: "escaped quote inside string", + expr: "github.workflow == 'test\\'s'", + wantErr: false, + }, + { + name: "unclosed single quote", + expr: "github.workflow == 'test", + wantErr: true, + errorInMsg: "unclosed single quote", + }, + { + name: "unclosed double quote", + expr: "github.workflow == \"test", + wantErr: true, + errorInMsg: "unclosed double quote", + }, + { + name: "unclosed backtick", + expr: "github.workflow == `test", + wantErr: true, + errorInMsg: "unclosed backtick", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateBalancedQuotes(tt.expr) + + if tt.wantErr { + require.Error(t, err, "Expected error for expression: %s", tt.expr) + if tt.errorInMsg != "" { + assert.Contains(t, err.Error(), tt.errorInMsg) + } + } else { + assert.NoError(t, err, "Expected no error for expression: %s", tt.expr) + } + }) + } +} + +func TestContainsLogicalOperators(t *testing.T) { + tests := []struct { + name string + expr string + expected bool + }{ + { + name: "no operators", + expr: "github.workflow", + expected: false, + }, + { + name: "has AND operator", + expr: "github.workflow && github.ref", + expected: true, + }, + { + name: "has OR operator", + expr: "github.workflow || github.ref", + expected: true, + }, + { + name: "has NOT operator", + expr: "!github.workflow", + expected: true, + }, + { + name: "has multiple operators", + expr: "!github.workflow && github.ref || github.repository", + expected: true, + }, + { + name: "has != comparison (triggers due to ! character)", + expr: "github.workflow != 'test'", + expected: true, // The function detects '!' even when part of '!='; parser will handle correctly + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := containsLogicalOperators(tt.expr) + assert.Equal(t, tt.expected, result, "containsLogicalOperators(%s) = %v, want %v", tt.expr, result, tt.expected) + }) + } +} + +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 + realWorldExpressions := []struct { + name string + group string + description string + }{ + { + name: "workflow-level PR concurrency", + group: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}", + description: "Pattern used for PR workflows with fallback", + }, + { + name: "workflow-level issue concurrency", + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}", + description: "Pattern used for issue workflows", + }, + { + name: "workflow-level command concurrency", + group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}", + description: "Pattern used for command workflows", + }, + { + name: "workflow-level discussion concurrency", + group: "gh-aw-${{ github.workflow }}-${{ github.event.discussion.number }}", + description: "Pattern used for discussion workflows", + }, + { + name: "workflow-level push concurrency", + group: "gh-aw-${{ github.workflow }}-${{ github.ref }}", + description: "Pattern used for push workflows", + }, + { + name: "engine-level concurrency", + group: "gh-aw-copilot-${{ github.workflow }}", + description: "Pattern used for engine-level concurrency", + }, + { + name: "simple static group", + group: "production", + description: "Simple static concurrency group", + }, + } + + for _, tt := range realWorldExpressions { + t.Run(tt.name, func(t *testing.T) { + err := validateConcurrencyGroupExpression(tt.group) + assert.NoError(t, err, "Real-world pattern should be valid: %s - %s", tt.group, tt.description) + }) + } +} + +// TestValidateConcurrencyGroupExpressionErrorMessages validates error message quality +func TestValidateConcurrencyGroupExpressionErrorMessages(t *testing.T) { + tests := []struct { + name string + group string + expectedErrorParts []string // Parts that should be in the error message + description string + }{ + { + name: "missing closing brace error message", + group: "workflow-${{ github.ref", + expectedErrorParts: []string{ + "unclosed expression braces", + "without matching closing", + "Add the missing closing braces", + }, + description: "Error should be actionable and clear", + }, + { + name: "unbalanced parenthesis error message", + group: "test-${{ (github.workflow }}", + expectedErrorParts: []string{ + "unclosed parentheses", + "opening '('", + "Add the missing closing ')'", + }, + description: "Error should indicate the specific problem", + }, + { + name: "empty expression error message", + group: "test-${{ }}", + expectedErrorParts: []string{ + "empty expression content", + "found empty expression", + "Provide a valid GitHub Actions expression", + }, + description: "Error should suggest a fix", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateConcurrencyGroupExpression(tt.group) + require.Error(t, err, tt.description) + + errorMsg := err.Error() + for _, part := range tt.expectedErrorParts { + assert.Contains(t, errorMsg, part, + "Error message should contain '%s' for: %s", part, tt.description) + } + }) + } +} + +// TestValidateConcurrencyGroupExpressionWithComplexExpressions tests complex valid expressions +func TestValidateConcurrencyGroupExpressionWithComplexExpressions(t *testing.T) { + complexExpressions := []struct { + name string + group string + description string + }{ + { + name: "nested parentheses", + group: "test-${{ ((github.workflow || github.ref) && github.repository) }}", + description: "Deeply nested parentheses should be valid", + }, + { + name: "multiple NOT operators", + group: "test-${{ !!github.workflow }}", + description: "Double negation should parse correctly", + }, + { + name: "complex boolean expression", + group: "test-${{ (github.workflow && github.ref) || (!github.repository && github.actor) }}", + description: "Complex boolean logic should be valid", + }, + { + name: "comparison expressions", + group: "test-${{ github.workflow == 'test' && github.repository != 'other' }}", + description: "Comparison operators should be valid", + }, + } + + for _, tt := range complexExpressions { + t.Run(tt.name, func(t *testing.T) { + err := validateConcurrencyGroupExpression(tt.group) + assert.NoError(t, err, "Complex expression should be valid: %s - %s", tt.group, tt.description) + }) + } +} + +// Benchmark tests +func BenchmarkValidateConcurrencyGroupExpression(b *testing.B) { + testCases := []string{ + "simple-group", + "gh-aw-${{ github.workflow }}", + "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}", + "test-${{ (github.workflow || github.ref) && github.repository }}", + } + + for _, tc := range testCases { + b.Run(tc, func(b *testing.B) { + for i := 0; i < b.N; i++ { + _ = validateConcurrencyGroupExpression(tc) + } + }) + } +} + +func BenchmarkValidateBalancedBraces(b *testing.B) { + input := "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}" + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = validateBalancedBraces(input) + } +} + +func BenchmarkValidateExpressionContent(b *testing.B) { + expr := "(github.workflow || github.ref) && github.repository" + fullGroup := "test-${{ (github.workflow || github.ref) && github.repository }}" + b.ResetTimer() + for i := 0; i < b.N; i++ { + _ = validateExpressionContent(expr, fullGroup) + } +} + +func TestExtractConcurrencyGroupFromYAML(t *testing.T) { + tests := []struct { + name string + yaml string + expected string + description string + }{ + { + name: "simple group with double quotes", + yaml: `concurrency: + group: "my-workflow-group"`, + expected: "my-workflow-group", + description: "Should extract group value from YAML with double quotes", + }, + { + name: "simple group with single quotes", + yaml: `concurrency: + group: 'my-workflow-group'`, + expected: "my-workflow-group", + description: "Should extract group value from YAML with single quotes", + }, + { + name: "group with expression", + yaml: `concurrency: + group: "gh-aw-${{ github.workflow }}"`, + expected: "gh-aw-${{ github.workflow }}", + description: "Should extract group with GitHub Actions expression", + }, + { + name: "group with cancel-in-progress", + yaml: `concurrency: + group: "my-workflow-group" + cancel-in-progress: true`, + expected: "my-workflow-group", + description: "Should extract group ignoring cancel-in-progress", + }, + { + name: "complex group expression", + yaml: `concurrency: + group: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}" + cancel-in-progress: true`, + expected: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}", + description: "Should extract complex group expression", + }, + { + name: "group without quotes", + yaml: `concurrency: + group: simple-group`, + expected: "simple-group", + description: "Should extract group without quotes", + }, + { + name: "string format with double quotes", + yaml: `concurrency: "my-workflow-group"`, + expected: "my-workflow-group", + description: "Should extract string format concurrency", + }, + { + name: "string format with expression", + yaml: `concurrency: workflow-${{ github.ref }}`, + expected: "workflow-${{ github.ref }}", + description: "Should extract string format with expression", + }, + { + name: "string format with unclosed expression", + yaml: `concurrency: workflow-${{ github.ref`, + expected: "workflow-${{ github.ref", + description: "Should extract string even with malformed expression (validation will catch it)", + }, + { + name: "no group field", + yaml: `concurrency: + cancel-in-progress: true`, + expected: "", + description: "Should return empty string when no group field", + }, + { + name: "empty yaml", + yaml: "", + expected: "", + description: "Should return empty string for empty input", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := extractConcurrencyGroupFromYAML(tt.yaml) + assert.Equal(t, tt.expected, result, tt.description) + }) + } +}