Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Feb 12, 2026

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)

    • Validates ${{ }} brace balance and nesting
    • Checks parentheses and quote pairing (single, double, backtick)
    • Detects malformed logical operators (&&, ||, !)
    • Leverages existing expression parser for complex boolean logic
  • Compiler integration (compiler.go)

    • Validates both workflow-level and engine-level concurrency during validateWorkflowData()
    • Handles string format (concurrency: "group") and object format (concurrency: {group: "group"})

Example

Invalid expression:

concurrency: workflow-${{ github.ref

Compile error:

✗ workflow-level concurrency validation failed: unclosed expression braces
  found opening '${{' at position 9 without matching closing '}}'
  Suggestion: Add the missing closing braces.

Valid expressions pass through:

concurrency: workflow-${{ github.ref }}
concurrency: gh-aw-${{ github.workflow }}-${{ github.event.pr.number || github.ref }}
Original prompt

Add compiler support to validate the syntax of custom group expressions as thoroughly as possible and detect obvious syntactic errors at compile time.


💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 4 commits February 12, 2026 03:51
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Add compiler support for validating custom group expressions Add compile-time syntax validation for concurrency group expressions Feb 12, 2026
Copilot AI requested a review from pelikhan February 12, 2026 04:05
@pelikhan
Copy link
Contributor

At @copilot, add more tests.

@pelikhan pelikhan marked this pull request as ready for review February 12, 2026 04:13
Copilot AI review requested due to automatic review settings February 12, 2026 04:13
@pelikhan pelikhan merged commit 77f63b8 into main Feb 12, 2026
3 of 4 checks passed
@pelikhan pelikhan deleted the copilot/add-compiler-support-syntax-validation branch February 12, 2026 04:13
Copilot stopped work on behalf of pelikhan due to an error February 12, 2026 04:13
Copy link
Contributor

Copilot AI left a 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.

Comment on lines +127 to +128
// Pattern to extract content between ${{ and }}
expressionPattern := regexp.MustCompile(`\$\{\{([^}]*)\}\}`)
Copy link

Copilot AI Feb 12, 2026

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.

Suggested change
// 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(`\$\{\{(.*?)\}\}`)

Copilot uses AI. Check for mistakes.
Comment on lines +157 to +174
// 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.",
)
}
}
}
Copy link

Copilot AI Feb 12, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +212 to +267
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
Copy link

Copilot AI Feb 12, 2026

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.

Copilot uses AI. Check for mistakes.
Comment on lines +190 to +201
// 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),
)
}
}

Copy link

Copilot AI Feb 12, 2026

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).

Suggested change
// 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),
)
}

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants