-
Notifications
You must be signed in to change notification settings - Fork 132
Add compile-time syntax validation for concurrency group expressions #15082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add compile-time syntax validation for concurrency group expressions #15082
Conversation
Co-authored-by: pelikhan <[email protected]>
Co-authored-by: pelikhan <[email protected]>
Co-authored-by: pelikhan <[email protected]>
Co-authored-by: pelikhan <[email protected]>
|
At @copilot, add more tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds compile-time validation for GitHub Actions-style ${{ }} expressions used in custom concurrency group strings, surfacing common syntax issues during compilation instead of at runtime.
Changes:
- Introduces a new validation module for concurrency group expressions (braces, parentheses/quotes, and parsing for boolean logic).
- Integrates validation into the compiler for both workflow-level and engine-level concurrency configuration.
- Adds unit + integration tests (and benchmarks) covering common valid/invalid patterns and error message quality.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/workflow/concurrency_validation.go | Implements concurrency group expression extraction + syntax validation logic. |
| pkg/workflow/compiler.go | Calls the new validator during compile-time workflow data validation for workflow/engine concurrency. |
| pkg/workflow/concurrency_validation_test.go | Adds unit tests and benchmarks for the new validation helpers and extraction logic. |
| pkg/workflow/concurrency_validation_integration_test.go | Adds integration tests asserting compile-time failures/successes for concurrency syntax. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Pattern to extract content between ${{ and }} | ||
| expressionPattern := regexp.MustCompile(`\$\{\{([^}]*)\}\}`) |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extraction regex \\$\\{\\{([^}]*)\\}\\} fails to match valid expressions that contain a } character inside the expression (e.g., string literals or function calls producing/containing braces). In that case, matches becomes empty and the validator silently skips validating expression content (parentheses/quotes/parser), allowing syntax errors to pass. Consider replacing this regex-based extraction with a small scanner that finds ${{ and the corresponding }} using index searching (and/or reusing the brace-balance traversal) so expressions are always extracted and validated correctly.
| // Pattern to extract content between ${{ and }} | |
| expressionPattern := regexp.MustCompile(`\$\{\{([^}]*)\}\}`) | |
| // Pattern to extract content between ${{ and }}. | |
| // Use a non-greedy wildcard so that '}' characters inside the expression are allowed. | |
| expressionPattern := regexp.MustCompile(`\$\{\{(.*?)\}\}`) |
| // 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.", | ||
| ) | ||
| } | ||
| } | ||
| } |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parentheses are counted without considering whether they occur inside quoted strings/backticks. This can produce false positives for expressions like github.ref == '(' or format('(%)', ...), where parentheses inside quotes should not affect balance. A tangible fix is to perform the parentheses scan while tracking quote state (single/double/backtick + escapes), skipping ( and ) when inside any quote context.
| 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 |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unclosed quotes can be missed when the final character is a backslash. In that case the loop hits ch == '\\\\', sets escaped = true, continues, and never runs the i == len(expr)-1 unclosed-quote checks. A concrete fix is to move 'unclosed quote' checks after the loop (based on inSingleQuote/inDoubleQuote/inBacktick), rather than relying on the last-iteration branch inside the loop.
| // 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), | ||
| ) | ||
| } | ||
| } | ||
|
|
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only parsing expressions when containsLogicalOperators(expr) is true leaves a gap where syntactically invalid expressions without &&, ||, or ! are accepted (e.g., github.ref == or other malformed comparisons/calls). If ParseExpression is intended to be the source of truth for syntax, it should be invoked for all extracted ${{ ... }} expressions after basic brace/quote checks (or at least for a broader set of operator tokens than just logical ones).
| // 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), | |
| ) | |
| } | |
| } | |
| // Parse all non-empty expressions to validate syntax | |
| trimmedExpr := strings.TrimSpace(expr) | |
| if trimmedExpr == "" { | |
| return nil | |
| } | |
| if _, err := ParseExpression(trimmedExpr); 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), | |
| ) | |
| } |
Custom concurrency group expressions can contain GitHub Actions expressions like
${{ github.ref }}. Previously, syntax errors (unbalanced braces, unclosed quotes, malformed operators) were only caught at runtime.Changes
New validation module (
concurrency_validation.go)${{ }}brace balance and nesting&&,||,!)Compiler integration (
compiler.go)validateWorkflowData()concurrency: "group") and object format (concurrency: {group: "group"})Example
Invalid expression:
Compile error:
Valid expressions pass through:
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.